Improve code template in README.md#140
Conversation
README.md
Outdated
| x.report("addition2") do |times| | ||
| i = 0 | ||
| while i < times | ||
| while (i += 1) <= times |
There was a problem hiding this comment.
Perhaps this is "too clever", but it has the nice effect of not needing benchmark-related code in the loop body.
What's left is purely the user's measured code.
There was a problem hiding this comment.
I think it's too error-prone, e.g. while (i += 1) < times is very subtly incorrect.
IMO this variant should not be shown in the README or not prominently, x.report("addition3", "1 + 2") is simpler if one cares a lot about overhead, there is little value to do the loop manually and it's far more verbose.
There was a problem hiding this comment.
there is little value to do the loop manually
I haven't been able to use the string form much in practice, because it runs in a different context that can't access the local variables you've made outside the eval'ed string.
So I can't use it for anything that requires non-measured setup code, e.g.
#!/usr/bin/ruby
require "benchmark/ips"
RubyVM::YJIT.enable
s = Set.new.compare_by_identity.freeze
Benchmark.ips do |x|
# undefined local variable or method `s' for #<Benchmark::IPS::Job::Entry:0x00000001050ffa68> (NameError)
x.report(".new.compare_by_identity", "s.dup")
x.report(".dup ", "Set.new.compare_by_identity")
x.compare!
endThere was a problem hiding this comment.
One could just use S = Set.new.compare_by_identity.freeze for that.
If wanting to measure something small, it's also best to avoid reading local variables from 2 frames up as you would with the manual loop form and this example.
There was a problem hiding this comment.
Would reading a constant S be faster than a local s? 😲
There was a problem hiding this comment.
Yes, it should be, especially given that s is two blocks away (so at least 3 memory reads, frame->parent->parent[OFFSET_OF_S]).
A JIT can even potentially embed the constant in JITed code and there is no read anymore (with safepoints to deopt if it changes).
The interpreter should be able to make it a single memory read in general.
There was a problem hiding this comment.
If this change is removed or changed to x.report("addition3", "1 + 2"), I will merge.
|
cc @evanphx what do you think of these changes? |
|
Thanks for the bump, forgot about this one |
Some ideas for making the minimal template a bit more readable. See my comments below, would love your thoughts @evanphx