Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmark.ips: show allocated bytes per operation #5522

Merged
merged 1 commit into from Jan 4, 2018
Merged

Benchmark.ips: show allocated bytes per operation #5522

merged 1 commit into from Jan 4, 2018

Conversation

asterite
Copy link
Member

@asterite asterite commented Jan 3, 2018

So for example for this code:

require "benchmark"

def mangle_one(name)
  name.gsub do |char|
    if char == 'o'
      ".#{char.ord.to_s(16)}."
    else
      char
    end
  end
end

def mangle_two(name)
  String.build do |io|
    name.each_char do |char|
      if char == 'o'
        io << '.'
        char.ord.to_s(16, io)
        io << '.'
      else
        io << char
      end
    end
  end
end

def mangle_three(name)
  name.gsub('x', 'y')
end

Benchmark.ips do |x|
  x.report("one") do
    mangle_one("foo bar")
  end
  x.report("two") do
    mangle_two("foo bar")
  end
  x.report("three") do
    mangle_three("foo bar")
  end
end

We now get:

  one   1.33M (753.32ns) (± 7.90%)  624 B/op  104.55× slower
  two   3.62M (276.15ns) (± 2.50%)  209 B/op   38.33× slower
three 138.79M (  7.21ns) (± 9.09%)    0 B/op         fastest

B/op means "Bytes per operation". I initially went with "per second" but it was a really big number so it was harder to understand.

@straight-shoota
Copy link
Member

I've always thought the output format is a bit misleading, as values like 1.33 M can be interpreted as a size in bytes as it is for example used by ls -lh. Now that there is really a memory size included in the IPS output, this feels even more pressing.
As this adds B/op unit, I would suggest to add a unit op/s for the first field.

target = Time.monotonic + @calculation_time

loop do
bytes_before_measure = GC.stats.total_bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought: Maybe this could be extracted as GC.measure_total_bytes { ... }?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to introduce public API, plus it'd used in just one place.

@asterite
Copy link
Member Author

asterite commented Jan 4, 2018

I agree about the numbers being cryptic. I'd like them to be a bit more explicit.

@asterite
Copy link
Member Author

asterite commented Jan 4, 2018

(but changing the format should be discussed and implemented in a separate issue/PR, this is already an improvement)

@RX14 RX14 added this to the Next milestone Jan 4, 2018
@RX14 RX14 merged commit ca555d2 into crystal-lang:master Jan 4, 2018
@asterite asterite deleted the feature/benchmark-ips-allocs branch January 5, 2018 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants