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

Provide Benchmark.quick_compare to quickly compare methods on an object #134

Merged
merged 6 commits into from
Sep 8, 2024

Conversation

evanphx
Copy link
Owner

@evanphx evanphx commented Jan 11, 2024

Closes #133

Any feedback welcome, especially from @zenspider

lib/benchmark/ips/quick.rb Outdated Show resolved Hide resolved
lib/benchmark/ips/quick.rb Outdated Show resolved Hide resolved
lib/benchmark/ips/quick.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@zenspider zenspider left a comment

Choose a reason for hiding this comment

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

This is not the quick_compare I was hoping for... but not blocking.

#
# @param warmup How many seconds to warm up the benchmark process
# @param time How many seconds to benchmark each method
def quick_compare(obj, *methods)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that in #133 I said nothing about "quickly comparing methods on an object". Please don't make this take an obj argument that you send on. benchmark-ips is MUCH more than running microbenchmarks on Array to know that reduce is slower.

Some code needs more complex setup.
Some code is comparing that setup.
Or anything else.

This argument limits things and would require me to make a class with my benchmark methods to get around it. THE POINT of this method is to require less structure to get benchmarks up and running and get an easy comparison of anything.

ary =  [Object.new] * 969
ary += [Assertion.new] * 3
ary += [UnexpectedError.new] * 1
ary += [Skip.new] * 27

RESULTS = ary

def report_orig
  aggregate = RESULTS.group_by { |r| r.class }
  aggregate.default = [] # dumb. group_by should provide this

  [aggregate[Assertion].size, aggregate[UnexpectedError].size, aggregate[Skip].size]
end

def report_orig2
  aggregate = RESULTS.group_by { |r| r.class }.transform_values(&:size)
  aggregate.default = 0

  [aggregate[Assertion], aggregate[UnexpectedError], aggregate[Skip]]
end

def report_tally
  aggregate = RESULTS.map { |r| r.class }
  aggregate = if RUBY_VERSION > "3.1" then
                aggregate.tally(Hash.new 0)
              else
                aggregate.tally.tap { |r| r.default = 0 }
              end

  [aggregate[Assertion], aggregate[UnexpectedError], aggregate[Skip]]
end

def report_tally2
  aggregate = RESULTS.map { |r| r.class }
  aggregate = aggregate.tally.tap { |r| r.default = 0 }

  [aggregate[Assertion], aggregate[UnexpectedError], aggregate[Skip]]
end

def report_tally3
  aggregate = RESULTS.map { |r| r.class }.tally
  aggregate.default = 0

  [aggregate[Assertion], aggregate[UnexpectedError], aggregate[Skip]]
end

Copy link
Owner Author

Choose a reason for hiding this comment

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

Correct, you didn't say anything about testing on an object. But without a receiver, who do you want the methods called on? just toplevel methods?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I moved quick_compare to Kernel so it's more flexible, more along the lines of your original version.

Copy link
Contributor

@eregon eregon Jan 13, 2024

Choose a reason for hiding this comment

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

What's the problem with Benchmark::IPS.quick_compare(self, :report_orig, :report_orig2, ...)?
Having the object is useful if you have more state you need to pass in.
Then for instance this state could live in an object instance.
Or the methods would be defined as singleton methods of a module, to have the necessary constant scope, etc.

I am against polluting Kernel.
I think a benchmarking harness should not monkey-patch anything.

Also for 10+ years users of this gem managed just fine without this helper method, so maybe we should not add it.
At least I think we should get multiple opinions to not add something which only works for one person.
The existing API is also the same as stdlib Benchmark, it's pretty simple.

lib/benchmark/ips/quick.rb Outdated Show resolved Hide resolved
lib/benchmark/ips/quick.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Collaborator

I'm back from the dead.

  1. I too don't like polluting Kernel by default. I think it's fine if we don't require 'benchmark/ips/quick by default. But, then you're kind of missing the point of this whole feature ofc because now you have to add another 15 characters at the top of your benchmark. In my head, the interface that makes the most sense is actually Benchmark.ips_quick_compare but I'm neutral and would be fine with Benchmark::IPS.quick_compare.
  2. I agree that benchmark/ips is about more than microbenchmarking, but microbenchmarking is cool too so we should minimize overhead where it's easy to do so.
  3. Supporting 2.3 is rad and doesn't block this feature

If no one objects I'll pick this up and run w/it

#
# @param warmup How many seconds to warm up the benchmark process
# @param time How many seconds to benchmark each method
def quick_compare(*methods)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use regular kwargs here?

Suggested change
def quick_compare(*methods)
def quick_compare(*methods, warmup: nil, time: nil)


methods.each do |name|
x.report(name) do |x|
x.times { __send__ name }
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using one of the faster forms, like the explicit while loop, or string form?

@nateberkopec
Copy link
Collaborator

I've changed it to my take.

  1. I tried @eregon's suggestion to send it directly to reduce overhead BUT that means you can't just define a method on main/Kernel and then quick compare it, because that method you just defined is private. The more important part of this feature is convenience rather than suitability for microbenching, so I decided to keep __send__
  2. It's been moved to Benchmark.ips_quick.
  3. Expanded docs.

LMK about objections, otherwise I'll add a test and merge

Copy link

@MatheusRich MatheusRich left a comment

Choose a reason for hiding this comment

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

I love this!

# def add; 1+1; end
# def sub; 2-1; end
# ips_quick([:add, :sub], warmup: 10)
def ips_quick(methods, receiver = Kernel, **opts)

Choose a reason for hiding this comment

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

I wonder if having quick in the name would make people think that it runs faster than the normal version

README.md Outdated Show resolved Hide resolved
lib/benchmark/ips.rb Outdated Show resolved Hide resolved
lib/benchmark/ips.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Collaborator

Thanks for everyone's feedback. This should be the final form now, incl. a basic test. If no objections, will merge in a few days.

module IPS

# Benchmark-ips Gem version.
VERSION = "2.13.0"
VERSION = "2.13.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just saw the version bump here. Changing to 2.14.

@nateberkopec nateberkopec merged commit f66045d into master Sep 8, 2024
22 checks passed
@nateberkopec nateberkopec deleted the f-quick branch September 8, 2024 06:11
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.

Add Q&D driver method
6 participants