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

Improve code template in README.md #140

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

amomchilov
Copy link
Contributor

@amomchilov amomchilov commented Jul 10, 2024

Some ideas for making the minimal template a bit more readable. See my comments below, would love your thoughts @evanphx

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
@@ -42,9 +38,8 @@ Benchmark.ips do |x|
# introduces incorrectable errors.
x.report("addition2") do |times|
i = 0
while i < times
while (i += 1) <= times
Copy link
Contributor Author

@amomchilov amomchilov Jul 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!
end

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would reading a constant S be faster than a local s? 😲

Copy link
Contributor

@eregon eregon Jul 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this change is removed or changed to x.report("addition3", "1 + 2"), I will merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nateberkopec Done! :)

Copy link
Contributor

@eregon eregon left a comment

Choose a reason for hiding this comment

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

LGTM

@amomchilov
Copy link
Contributor Author

cc @evanphx what do you think of these changes?

@nateberkopec nateberkopec merged commit bfdf58d into evanphx:master Aug 29, 2024
@nateberkopec
Copy link
Collaborator

Thanks for the bump, forgot about this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants