From ca4657699e3f68b2c32343f55113022c329dc905 Mon Sep 17 00:00:00 2001 From: viralpraxis Date: Sun, 4 May 2025 16:47:14 +0300 Subject: [PATCH] Add new `Performance/ConditionalDefinition` cop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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,...} ...`, 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 ...` 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. --- config/default.yml | 7 ++ .../cop/performance/conditional_definition.rb | 68 +++++++++++++++++++ lib/rubocop/cop/performance_cops.rb | 1 + .../conditional_definition_spec.rb | 53 +++++++++++++++ 4 files changed, 129 insertions(+) create mode 100644 lib/rubocop/cop/performance/conditional_definition.rb create mode 100644 spec/rubocop/cop/performance/conditional_definition_spec.rb diff --git a/config/default.yml b/config/default.yml index 606acb83d..492138f72 100644 --- a/config/default.yml +++ b/config/default.yml @@ -89,6 +89,13 @@ Performance/ConcurrentMonotonicTime: Enabled: pending VersionAdded: '1.12' +Performance/ConditionalDefinition: + Description: >- + Move conditional logic outside of the method body to + improve performance by avoiding repeated evaluation of conditions. + Enabled: pending + VersionAdded: '<>' + Performance/ConstantRegexp: Description: 'Finds regular expressions with dynamic components that are all constants.' Enabled: pending diff --git a/lib/rubocop/cop/performance/conditional_definition.rb b/lib/rubocop/cop/performance/conditional_definition.rb new file mode 100644 index 000000000..edc913223 --- /dev/null +++ b/lib/rubocop/cop/performance/conditional_definition.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # Move conditional logic outside of the method body definition to + # improve performance by avoiding repeated evaluation of constant condition. + # + # @example + # # bad + # def answer + # if RUBY_VERSION < '4.0.' + # 42 + # else + # 239 + # end + # end + # + # # good + # if RUBY_VERSION < '4.0.' + # def answer + # 42 + # end + # else + # def answer + # 239 + # end + # end + class ConditionalDefinition < Base + MSG = 'Move conditional logic outside of the method body definition to ' \ + 'improve performance by avoiding repeated evaluation of constant condition.' + + CONSTANTS = Set[ + :RUBY_VERSION, + :RUBY_RELEASE_DATE, + :RUBY_PLATFORM, + :RUBY_PATCHLEVEL, + :RUBY_REVISION, + :RUBY_COPYRIGHT, + :RUBY_ENGINE, + :RUBY_ENGINE_VERSION, + :RUBY_DESCRIPTION + ].freeze + OPERATORS = Set[:<, :<=, :==, :>=, :>, :=~, :===].freeze + private_constant :CONSTANTS, :OPERATORS + + def on_def(node) + return unless dynamic_definition_in_body?(node) + + dynamic_definition_in_body?(node) do + add_offense(node) + end + end + + # @!method bad_method?(node) + def_node_matcher :dynamic_definition_in_body?, <<~PATTERN + (def _ (...) + (if + { + (send (const nil? CONSTANTS) OPERATORS _) + (send _ OPERATORS (const nil? CONSTANTS)) + } + _ _)) + PATTERN + end + end + end +end diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 8a56be5db..16a3a30b7 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -14,6 +14,7 @@ require_relative 'performance/collection_literal_in_loop' require_relative 'performance/compare_with_block' require_relative 'performance/concurrent_monotonic_time' +require_relative 'performance/conditional_definition' require_relative 'performance/constant_regexp' require_relative 'performance/count' require_relative 'performance/delete_prefix' diff --git a/spec/rubocop/cop/performance/conditional_definition_spec.rb b/spec/rubocop/cop/performance/conditional_definition_spec.rb new file mode 100644 index 000000000..c08202327 --- /dev/null +++ b/spec/rubocop/cop/performance/conditional_definition_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::ConditionalDefinition, :config do + %i[ + RUBY_VERSION + RUBY_PLATFORM + RUBY_PATCHLEVEL + RUBY_ENGINE + RUBY_DESCRIPTION + ].each do |constant| + %i[< <= === >= > =~ ===].each do |operator| + it "registers an offense when using `#{operator}` comparison with `#{constant}` as LHS" do + expect_offense(<<~RUBY) + def foo + ^^^^^^^ Move conditional logic outside of the method body definition to improve performance by avoiding repeated evaluation of constant condition. + if #{constant} #{operator} '3.0' + 1 + else + 2 + end + end + RUBY + end + + it "registers an offense when using `#{operator}` comparison with `#{constant}` as RHS" do + expect_offense(<<~RUBY) + def foo + ^^^^^^^ Move conditional logic outside of the method body definition to improve performance by avoiding repeated evaluation of constant condition. + if '3.0' #{operator} #{constant} + 1 + else + 2 + end + end + RUBY + end + end + end + + it 'does not register an offense when conditional logic is moved outside of method definition' do + expect_no_offenses(<<~RUBY) + if RUBY_VERSION < '4.0.' + def answer + 42 + end + else + def answer + 239 + end + end + RUBY + end +end