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

[Fix #385] Disable Performance/BlockGivenWithExplicitBlock by default #466

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

Earlopain
Copy link
Contributor

See linked issue for benchmark details.

I tested with YJIT and its even worse:

ruby 3.4.0dev (2024-09-05T09:43:46Z master 63cbe3f6ac) +YJIT [x86_64-linux]
Warming up --------------------------------------
               block     1.635M i/100ms
      block w/ block   285.930k i/100ms
        block_given?     1.867M i/100ms
block_given? w/ block
                         1.497M i/100ms
Calculating -------------------------------------
               block     22.611M (±12.7%) i/s   (44.23 ns/i) -    111.180M in   5.033721s
      block w/ block      3.034M (± 8.5%) i/s  (329.59 ns/i) -     15.154M in   5.032609s
        block_given?     24.935M (±10.7%) i/s   (40.10 ns/i) -    123.254M in   5.002983s
block_given? w/ block
                         23.007M (±14.7%) i/s   (43.47 ns/i) -    113.782M in   5.058213s

Comparison:
        block_given?: 24935388.3 i/s
block_given? w/ block: 23006920.5 i/s - same-ish: difference falls within error
               block: 22610939.8 i/s - same-ish: difference falls within error
      block w/ block:  3034047.2 i/s - 8.22x  slower

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

…y default

See linked issue for benchmark details.

I tested with YJIT and its even worse:
```
ruby 3.4.0dev (2024-09-05T09:43:46Z master 63cbe3f6ac) +YJIT [x86_64-linux]
Warming up --------------------------------------
               block     1.635M i/100ms
      block w/ block   285.930k i/100ms
        block_given?     1.867M i/100ms
block_given? w/ block
                         1.497M i/100ms
Calculating -------------------------------------
               block     22.611M (±12.7%) i/s   (44.23 ns/i) -    111.180M in   5.033721s
      block w/ block      3.034M (± 8.5%) i/s  (329.59 ns/i) -     15.154M in   5.032609s
        block_given?     24.935M (±10.7%) i/s   (40.10 ns/i) -    123.254M in   5.002983s
block_given? w/ block
                         23.007M (±14.7%) i/s   (43.47 ns/i) -    113.782M in   5.058213s

Comparison:
        block_given?: 24935388.3 i/s
block_given? w/ block: 23006920.5 i/s - same-ish: difference falls within error
               block: 22610939.8 i/s - same-ish: difference falls within error
      block w/ block:  3034047.2 i/s - 8.22x  slower
```
@koic
Copy link
Member

koic commented Sep 13, 2024

If the speed is reversed, shouldn't the behavior be updated to reverse the bad and good cases instead of disabling the cop?

@Earlopain
Copy link
Contributor Author

I would rather not make that decision. The difference today is smaller than it was when the cop was first introduced and I find it likely to be further optimized in Ruby itself in the future, where the cop can can then again produce worse results.

The results are also only consistently in favor of block_given? when YJIT is enabled, otherwise if block (without actually passing in a block) is still the fastest variant):

               block:  9336250.8 i/s
        block_given?:  8365028.9 i/s - same-ish: difference falls within error
block_given? w/ block:  7620740.6 i/s - 1.23x  slower
      block w/ block:  2334882.5 i/s - 4.00x  slower

Overall "it depends". Per #385 (comment) there is also a difference if the block parameter is defined in the method signature.

@koic
Copy link
Member

koic commented Sep 13, 2024

The difference today is smaller than it was when the cop was first introduced and I find it likely to be further optimized in Ruby itself in the future

I see. That would make sense.

@koic koic merged commit f28d18d into rubocop:master Sep 13, 2024
14 checks passed
@Earlopain Earlopain deleted the block-given-disable branch September 13, 2024 10:32
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.

2 participants