Skip to content

Add new Performance/ConditionalDefinition cop #499

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

viralpraxis
Copy link
Contributor

There's a well-known pattern of defining methods conditionally, especially in gems:

def foo
  if RUBY_VERSION < '4.0.'
    ...
  else
    ...
  end
end

If foo is a hotspot, its performance can be improved (see benchmarks below) by moving the conditional logic outside the method body definition:

if RUBY_VERSION < '4.0.'
  def foo
    ...
  end
else
  def foo
    ...
  end
end

I'd like to propose a new Performance/ConditionalDefinition cop that detects conditional method definitions, as described above. We can start by matching constant expressions such as {RUBY_VERSION,RUBY_RELEASE_DATE,...} <op> ..., and generalize the detection logic later.

require 'benchmark/ips'

class ConditionInBody
  def foo
    if RUBY_VERSION >= '3.2'
      1
    else
      2
    end
  end
end

class ConditionalDefinition
  if RUBY_VERSION >= '3.2'
    def foo
      1
    end
  else
    def foo
      2
    end
  end
end

obj1 = ConditionInBody.new
obj2 = ConditionalDefinition.new

Benchmark.ips do |x|
  x.report('condition in body') do
    obj1.foo
  end

  x.report('conditional method definition') do
    obj2.foo
  end

  x.compare!
end
$ ruby bm.rb
ruby 3.4.3 (2025-04-14 revision d0b7e5b6a0) +PRISM [x86_64-linux]
Warming up --------------------------------------
   condition in body     1.170M i/100ms
conditional method definition
                         2.228M i/100ms
Calculating -------------------------------------
   condition in body     11.683M (± 2.6%) i/s   (85.60 ns/i) -     58.485M in   5.009850s
conditional method definition
                         22.148M (± 1.0%) i/s   (45.15 ns/i) -    111.418M in   5.031059s

Comparison:
conditional method definition: 22148111.6 i/s
   condition in body: 11682500.0 i/s - 1.90x  slower

I'm not an expert in YJIT (or any other MRI JIT compiler), but AFAIK even when YJIT determines that RUBY_VERSION <op> ... is constant, it still needs to check a guard clause for every method call. So there's still a performance improvement:

$ ruby --yjit bm.rb
ruby 3.4.3 (2025-04-14 revision d0b7e5b6a0) +YJIT +PRISM [x86_64-linux]
Warming up --------------------------------------
   condition in body     1.853M i/100ms
conditional method definition
                         3.356M i/100ms
Calculating -------------------------------------
   condition in body     22.752M (± 0.4%) i/s   (43.95 ns/i) -    114.907M in   5.050444s
conditional method definition
                         51.223M (± 0.4%) i/s   (19.52 ns/i) -    258.439M in   5.045467s

Comparison:
conditional method definition: 51223091.5 i/s
   condition in body: 22752233.2 i/s - 2.25x  slower

NOTE: This is a WIP, and the cop is not yet ready -- at this point I just want to get feedback on the idea so I can continue working on it.


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.

There's a well-known pattern of defining methods conditionally, especially in gems:

```ruby
def foo
  if RUBY_VERSION < '4.0.'
    ...
  else
    ...
  end
end
```

If `foo` is a hotspot, its performance can be improved (see benchmarks below)
by moving the conditional logic outside the method body definition:

```ruby
if RUBY_VERSION < '4.0.'
  def foo
    ...
  end
else
  def foo
    ...
  end
end
```

I'd like to propose a new `Performance/ConditionalDefinition` cop
that detects conditional method definitions, as described above.
We can start by matching constant expressions such as
`{RUBY_VERSION,RUBY_RELEASE_DATE,...} <op> ...`, and generalize the detection logic later.

```ruby

require 'benchmark/ips'

class ConditionInBody
  def foo
    if RUBY_VERSION >= '3.2'
      1
    else
      2
    end
  end
end

class ConditionalDefinition
  if RUBY_VERSION >= '3.2'
    def foo
      1
    end
  else
    def foo
      2
    end
  end
end

obj1 = ConditionInBody.new
obj2 = ConditionalDefinition.new

Benchmark.ips do |x|
  x.report('condition in body') do
    obj1.foo
  end

  x.report('conditional method definition') do
    obj2.foo
  end

  x.compare!
end
```

```bash
$ ruby bm.rb
ruby 3.4.3 (2025-04-14 revision d0b7e5b6a0) +PRISM [x86_64-linux]
Warming up --------------------------------------
   condition in body     1.170M i/100ms
conditional method definition
                         2.228M i/100ms
Calculating -------------------------------------
   condition in body     11.683M (± 2.6%) i/s   (85.60 ns/i) -     58.485M in   5.009850s
conditional method definition
                         22.148M (± 1.0%) i/s   (45.15 ns/i) -    111.418M in   5.031059s

Comparison:
conditional method definition: 22148111.6 i/s
   condition in body: 11682500.0 i/s - 1.90x  slower
```

I'm not an expert in YJIT (or any other MRI JIT compiler), but AFAIK
even when YJIT determines that `RUBY_VERSION <op> ...` is constant, it still needs to check
a guard clause for every method call. So there's still a performance improvement:

```bash
$ ruby --yjit bm.rb
ruby 3.4.3 (2025-04-14 revision d0b7e5b6a0) +YJIT +PRISM [x86_64-linux]
Warming up --------------------------------------
   condition in body     1.853M i/100ms
conditional method definition
                         3.356M i/100ms
Calculating -------------------------------------
   condition in body     22.752M (± 0.4%) i/s   (43.95 ns/i) -    114.907M in   5.050444s
conditional method definition
                         51.223M (± 0.4%) i/s   (19.52 ns/i) -    258.439M in   5.045467s

Comparison:
conditional method definition: 51223091.5 i/s
   condition in body: 22752233.2 i/s - 2.25x  slower
```

**NOTE**: This is a WIP, and the cop is not yet ready -- at this point
I just want to get feedback on the idea so I can continue working on it.
@viralpraxis viralpraxis force-pushed the add-new-performance-conditional-definition-cop branch from 930e803 to ca46576 Compare May 4, 2025 14:06
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.

1 participant