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

Suggestion from Performance/BlockGivenWithExplicitBlock is often slower #385

Open
jhawthorn opened this issue Nov 20, 2023 · 3 comments
Open

Comments

@jhawthorn
Copy link

jhawthorn commented Nov 20, 2023

Performance/BlockGivenWithExplicitBlock provides bad guidance and should be removed. The benchmark which was given when it was introduced didn't test the case that a block was given, when that happens block is significantly slower than the other option.

For Ruby 3.2+ I made if block faster, but prior to that the advice from this cop was always wrong. Even in 3.2+ I believe it's bad advice as there's only a very narrow usage for which block is (very slightly) faster, and even in those cases it's likely a future refactor the user will accidentally introduce the slow case (ex. moving from if block to if block && something_else)

require 'bundler/inline'

gemfile(true) do
  gem 'benchmark-ips'
end

def if_block(&block)
  !!block
end

def if_block_given(&block)
  !!block_given?
end

Benchmark.ips do |x|
  x.report('block')                  { if_block }
  x.report('block w/ block')         { if_block {} }
  x.report('block_given?')           { if_block_given }
  x.report('block_given? w/ block')  { if_block_given {} }
  x.compare!
end
Warming up --------------------------------------
            block     1.612M i/100ms
   block w/ block   508.391k i/100ms
     block_given?     1.309M i/100ms
block_given? w/ block
                         1.068M i/100ms
Calculating -------------------------------------
            block     15.804M (± 1.8%) i/s -     79.000M in   5.000346s
   block w/ block      5.017M (± 0.5%) i/s -     25.420M in   5.067106s
     block_given?     13.297M (± 1.7%) i/s -     66.746M in   5.021282s
block_given? w/ block
                         10.745M (± 0.6%) i/s -     54.450M in   5.067787s

Comparison:
            block: 15804072.8 i/s
     block_given?: 13296844.8 i/s - 1.19x  slower
block_given? w/ block: 10744860.7 i/s - 1.47x  slower
   block w/ block:  5016692.5 i/s - 3.15x  slower
@casperisfine
Copy link

To be fair, the assumption is that with block_given? you can often avoid declaring &block:

require 'bundler/inline'

gemfile(true) do
  gem 'benchmark-ips'
end

def if_block(&block)
  !!block
end

def if_block_given
  block_given?
end

Benchmark.ips do |x|
  x.report('block')                  { if_block }
  x.report('block w/ block')         { if_block {} }
  x.report('block_given?')           { if_block_given }
  x.report('block_given? w/ block')  { if_block_given {} }
  x.compare!(order: :baseline)
end
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]
Resolving dependencies...
Warming up --------------------------------------
               block     1.343M i/100ms
      block w/ block   513.029k i/100ms
        block_given?     1.465M i/100ms
block_given? w/ block
                         1.457M i/100ms
Calculating -------------------------------------
               block     13.177M (± 2.0%) i/s -     67.134M in   5.096854s
      block w/ block      5.471M (± 4.3%) i/s -     27.704M in   5.074617s
        block_given?     14.664M (± 0.8%) i/s -     74.736M in   5.096843s
block_given? w/ block
                         14.537M (± 0.8%) i/s -     72.851M in   5.011781s

Comparison:
               block: 13176873.3 i/s
        block_given?: 14664219.5 i/s - 1.11x  faster
block_given? w/ block: 14537095.2 i/s - 1.10x  faster
      block w/ block:  5470680.3 i/s - 2.41x  slower

But even then, the difference is too small to be worth in unless you enter the case where the Proc has to be instantiated.

require 'bundler/inline'

gemfile(true) do
  gem 'benchmark-ips'
end

def if_block(&block)
  block ? true : false
end

def if_block_given
  block_given?
end

Benchmark.ips do |x|
  x.report('block')                  { if_block }
  x.report('block w/ block')         { if_block {} }
  x.report('block_given?')           { if_block_given }
  x.report('block_given? w/ block')  { if_block_given {} }
  x.compare!(order: :baseline)
end
Warming up --------------------------------------
               block     1.428M i/100ms
      block w/ block     1.321M i/100ms
        block_given?     1.456M i/100ms
block_given? w/ block
                         1.451M i/100ms
Calculating -------------------------------------
               block     14.424M (± 1.6%) i/s -     72.851M in   5.052055s
      block w/ block     13.159M (± 1.4%) i/s -     66.037M in   5.019221s
        block_given?     14.549M (± 1.4%) i/s -     72.786M in   5.003860s
block_given? w/ block
                         14.404M (± 1.3%) i/s -     72.529M in   5.036180s

Comparison:
               block: 14424088.9 i/s
        block_given?: 14548868.5 i/s - same-ish: difference falls within error
block_given? w/ block: 14404275.0 i/s - same-ish: difference falls within error
      block w/ block: 13159479.3 i/s - 1.10x  slower

@jhawthorn
Copy link
Author

To be fair, the assumption is that with block_given? you can often avoid declaring &block:

That's another point in favour of removing the linter, which tells you to replace block_given? with block.

@byroot
Copy link

byroot commented Nov 20, 2023

ROFL, I just assumed it was advocating for block_given?, I couldn't imagine &block would ever be faster. Same perf at best.

So yeah, definite 👍

joshcooper added a commit to joshcooper/puppet that referenced this issue Dec 6, 2023
This cop flags methods where `&block` is passed to a method, which is a very
common practice. There's also debate[1] as to whether specifying `&block`
explicitly is better than implicitly accepting a block.

[1] rubocop/rubocop-performance#385
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

No branches or pull requests

3 participants