From ead4efc87e5391ee718f327abafb06dcae21ffcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Wed, 23 Sep 2026 22:56:20 -0600 Subject: [PATCH 1/3] Step 1: Measure select-first-vs-detect with the default timing It was the only benchmark measured for 20 s per report (Benchmark.ips(20)) instead of the default 5 s, since it was added in 87ff8f5 with no reason given. That made it about 4x slower than others in every CI job, and its numbers were measured differently from every other file. At the default timing the result holds, with margins no worse: detect is 3.82x faster than select.first on Ruby 3.4 (3.55x at 20 s) and 4.83x on Ruby 2.1 (4.53x at 20 s). --- code/enumerable/select-first-vs-detect.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/enumerable/select-first-vs-detect.rb b/code/enumerable/select-first-vs-detect.rb index 6b7f0bf9..f535f69e 100644 --- a/code/enumerable/select-first-vs-detect.rb +++ b/code/enumerable/select-first-vs-detect.rb @@ -10,7 +10,7 @@ def fast ARRAY.detect { |x| x.eql?(15) } end -Benchmark.ips(20) do |x| +Benchmark.ips do |x| x.report('Enumerable#select.first') { slow } x.report('Enumerable#detect') { fast } x.compare! From 49efff6fc84e51cdb3706ffbf64cfa119f2cdd0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Wed, 23 Sep 2026 22:56:20 -0600 Subject: [PATCH 2/3] Step 2: Remove concatenation_randomized.rb, which never ran Its three Benchmark.ips blocks are inside methods (bench_100_to_100, bench_100_to_1000, bench_10_to_100) that nothing calls, so running it only built its random strings: no report, no comparison. CI logs show only its `ruby -v` line, and results collection found nothing from it. The results in its comments came from a manual run. concatenation.rb already covers every method it compared (String#+, interpolation, String#concat, String#append). It came from #216 for issue #158 and was never linked from the README. --- code/string/concatenation_randomized.rb | 154 ------------------------ 1 file changed, 154 deletions(-) delete mode 100644 code/string/concatenation_randomized.rb diff --git a/code/string/concatenation_randomized.rb b/code/string/concatenation_randomized.rb deleted file mode 100644 index 8a5e7a9f..00000000 --- a/code/string/concatenation_randomized.rb +++ /dev/null @@ -1,154 +0,0 @@ -require 'benchmark/ips' - -module RandStr - RND_STRINGS_AMOUNT = 1000 - @rand_strs = { - lt100: [], - lt10: [], - lt1000: [], - eq10: [], - eq100: [], - } - - def self.generate_rand_strs - chars = ('A'..'z').to_a * 20 - @rand_strs[:lt10] = Array.new(RND_STRINGS_AMOUNT) { chars.sample(rand(10)).join } - @rand_strs[:lt100] = Array.new(RND_STRINGS_AMOUNT) { chars.sample(rand(100)).join } - @rand_strs[:lt1000] = Array.new(RND_STRINGS_AMOUNT) { chars.sample(rand(1000)).join } - @rand_strs[:eq10] = Array.new(RND_STRINGS_AMOUNT) { chars.sample(10).join } - @rand_strs[:eq100] = Array.new(RND_STRINGS_AMOUNT) { chars.sample(100).join } - end - - self.generate_rand_strs - - def self.rand_str(named_range) - @rand_strs[named_range][rand(RND_STRINGS_AMOUNT)] - end - - def self.method_missing(symbol) - return super unless @rand_strs.keys.include?(symbol) - - define_singleton_method(symbol) { rand_str(symbol) } - return rand_str(symbol) - end - -end - - -# 2 + 1 = 3 object -def fastest_plus(foo, bar) - foo + bar -end - -# 2 + 1 = 3 object -def slow_concat(foo, bar) - foo.concat bar -end - -# 2 + 1 = 3 object -def slow_append(foo, bar) - foo << bar -end - - -def fast_interpolation(foo, bar) - "#{foo}#{bar}" -end - -# bench_100_to_100 -# Rehearsal ----------------------------------------------------------- -# String#+ 1.263725 0.027868 1.291593 ( 1.292498) -# "#{foo}#{bar}" 1.139442 0.022956 1.162398 ( 1.163574) -# String#concat 2.017746 0.014836 2.032582 ( 2.034682) -# String#append 1.320778 0.000000 1.320778 ( 1.321896) -# Collateral actions only 0.713309 0.000000 0.713309 ( 0.714402) -# -------------------------------------------------- total: 6.520660sec -# -# user system total real nomalized ratio -# Collateral actions only 0.703668 0.000000 0.703668 ( 0.705658) -# String#+ 1.014123 0.000000 1.014123 ( 1.015003) 0.30934 -# "#{foo}#{bar}" 1.101751 0.000585 1.102336 ( 1.103558) 0.3979 x 1.3 slower -# String#concat 1.382647 0.000000 1.382647 ( 1.385333) 0.679675 x 2.2 slower -# String#append 1.319974 0.000000 1.319974 ( 1.324772) 0.619114 x 2 slower - -def bench_100_to_100 - Benchmark.ips do |x| - # 1M for rehearsal + 1M for bm - sarr1 = Array.new(2_000_000) { RandStr.eq100.dup } - sarr2 = Array.new(2_000_000) { RandStr.eq100.dup } - - i, j = 0, 0 - # if we want compare apples with apples, we need to measure and exclude "collateral" operations: - # integer += 1, access to an array of randomized strings 100 symbols length, - # then two methods invocation from RandStr module eq100 / lt100. - # - # and only then we can compare string concat methods properly - x.report("Collateral actions only") { k=0; 1_000_000.times { k+=1; RandStr.eq100; sarr2[k]; RandStr.lt100; } } - - x.report("String#+") { k=0; 1_000_000.times { k+=1; sarr1[k]; fastest_plus(RandStr.eq100, RandStr.lt100) } } - x.report('"#{foo}#{bar}"') { k=0; 1_000_000.times { k+=1; sarr2[k]; fast_interpolation(RandStr.eq100, RandStr.lt100) } } - x.report("String#concat") { 1_000_000.times { RandStr.eq100; slow_concat(sarr1[i], RandStr.lt100); i+=1; } } - x.report("String#append") { 1_000_000.times { RandStr.eq100; slow_append(sarr2[j], RandStr.lt100); j+=1; } } - end -end - -# bench_100_to_1000 -# Rehearsal ----------------------------------------------------------- -# Collateral actions only 0.674168 0.000016 0.674184 ( 0.675031) -# String#+ 2.148756 0.032954 2.181710 ( 2.187042) -# "#{foo}#{bar}" 1.570816 0.004948 1.575764 ( 1.579080) -# String#concat 2.223220 0.160917 2.384137 ( 2.387601) -# String#append 2.005056 0.202962 2.208018 ( 2.211476) -# -------------------------------------------------- total: 9.023813sec -# -# user system total real nomalized ratio -# Collateral actions only 0.666190 0.000000 0.666190 ( 0.666398) -# String#+ 1.077629 0.036944 1.114573 ( 1.115465) 0.449067 -# "#{foo}#{bar}" 1.230489 0.001029 1.231518 ( 1.232423) 0.566025 x 1.25 slower -# String#concat 1.881313 0.149949 2.031262 ( 2.033965) 1.367567 x 3.05 slower -# String#append 1.913785 0.177921 2.091706 ( 2.094298) 1.4279 x 3.18 slower - -def bench_100_to_1000 - Benchmark.ips do |x| - sarr1 = Array.new(2_000_000) { RandStr.eq100.dup } - sarr2 = Array.new(2_000_000) { RandStr.eq100.dup } - - i, j = 0, 0 - x.report("Collateral actions only") { k=0; 1_000_000.times { k+=1; RandStr.eq100; sarr2[k]; RandStr.lt1000; } } - - x.report("String#+") { k=0; 1_000_000.times { k+=1; sarr1[k]; fastest_plus(RandStr.eq100, RandStr.lt1000) } } - x.report('"#{foo}#{bar}"') { k=0; 1_000_000.times { k+=1; sarr2[k]; fast_interpolation(RandStr.eq100, RandStr.lt1000) } } - x.report("String#concat") { 1_000_000.times { RandStr.eq100; slow_concat(sarr1[i], RandStr.lt1000); i+=1; } } - x.report("String#append") { 1_000_000.times { RandStr.eq100; slow_append(sarr2[j], RandStr.lt1000); j+=1; } } - end -end - -# bench_10_to_100 -# Rehearsal ----------------------------------------------------------- -# Collateral actions only 0.681273 0.000000 0.681273 ( 0.681611) -# String#+ 1.188326 0.000701 1.189027 ( 1.196455) -# "#{foo}#{bar}" 1.182554 0.003851 1.186405 ( 1.191678) -# String#concat 1.707191 0.006764 1.713955 ( 1.720055) -# String#append 1.177368 0.000831 1.178199 ( 1.184116) -# -------------------------------------------------- total: 5.948859sec -# -# user system total real nomalized ratio -# Collateral actions only 0.682486 0.000000 0.682486 ( 0.682818) -# String#+ 0.914002 0.000000 0.914002 ( 0.917294) 0.234476 -# "#{foo}#{bar}" 1.096633 0.000966 1.097599 ( 1.100782) 0.417964 x 1.78 slower -# String#concat 1.373582 0.000910 1.374492 ( 1.375239) 0.692421 x 2.95 slower -# String#append 1.300632 0.000000 1.300632 ( 1.300807) 0.617989 x 2.63 slower - -def bench_10_to_100 - Benchmark.ips do |x| - sarr1 = Array.new(2_000_000) { RandStr.eq100.dup } - sarr2 = Array.new(2_000_000) { RandStr.eq100.dup } - - i, j = 0, 0 - x.report("Collateral actions only") { k=0; 1_000_000.times { k+=1; RandStr.eq10; sarr2[k]; RandStr.lt100; } } - x.report("String#+") { k=0; 1_000_000.times { k+=1; sarr1[k]; fastest_plus(RandStr.eq10, RandStr.lt100) } } - x.report('"#{foo}#{bar}"') { k=0; 1_000_000.times { k+=1; sarr2[k]; fast_interpolation(RandStr.eq10, RandStr.lt100) } } - x.report("String#concat") { 1_000_000.times { RandStr.eq10; slow_concat(sarr1[i], RandStr.lt100); i+=1; } } - x.report("String#append") { 1_000_000.times { RandStr.eq10; slow_append(sarr2[j], RandStr.lt100); j+=1; } } - end -end From 3194e2cb92ef5b6b2ae82a1884124cb04297b4e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Wed, 23 Sep 2026 22:56:20 -0600 Subject: [PATCH 3/3] Step 3: Lint the shape of every benchmark in CI .github/scripts/lint-benchmarks.rb reads each file under code/ with Prism, without running it, and fails when a Benchmark.ips block: - does not call x.compare! - sets its own timing (Benchmark.ips(20), x.time =, x.warmup =, or x.config with time or warmup in any hash syntax) - sits inside a method that never runs from the top of the file, which is how concatenation_randomized.rb measured nothing for years A new lint job runs it on ruby_4.0. The benchmark jobs wait for it, so a failing lint does not hold 26 jobs of the org's slots, and benchmarks-ok, the required check, fails when it fails. CONTRIBUTING lists the three rules. --- .github/scripts/check-benchmarks.sh | 9 +- .github/scripts/lint-benchmarks.rb | 175 ++++++++++++++++++++++++++++ .github/workflows/benchmarks.yml | 18 ++- CONTRIBUTING.md | 6 + 4 files changed, 201 insertions(+), 7 deletions(-) create mode 100644 .github/scripts/lint-benchmarks.rb diff --git a/.github/scripts/check-benchmarks.sh b/.github/scripts/check-benchmarks.sh index d5bebb80..eff2bccb 100755 --- a/.github/scripts/check-benchmarks.sh +++ b/.github/scripts/check-benchmarks.sh @@ -1,7 +1,8 @@ #!/bin/bash -# The benchmarks-ok check. Passes when every benchmark job passed, or when -# there was nothing to benchmark. Usage: check-benchmarks.sh +# The benchmarks-ok check. +# Passes when the lint passed and every benchmark job passed, or there was nothing to benchmark. +# Usage: check-benchmarks.sh set -e -echo "changes: $1, rake: $2" -[ "$1" = success ] && { [ "$2" = success ] || [ "$2" = skipped ]; } +echo "changes: $1, lint: $2, rake: $3" +[ "$1" = success ] && [ "$2" = success ] && { [ "$3" = success ] || [ "$3" = skipped ]; } diff --git a/.github/scripts/lint-benchmarks.rb b/.github/scripts/lint-benchmarks.rb new file mode 100644 index 00000000..48c166f2 --- /dev/null +++ b/.github/scripts/lint-benchmarks.rb @@ -0,0 +1,175 @@ +# Checks that every benchmark under code/ has the same shape, so they all run, +# print a comparison, and are measured the same way: +# +# - every Benchmark.ips block calls x.compare! +# - no custom timing (Benchmark.ips(20), x.time = 20, x.warmup = 5, x.config(time: 20) in any hash syntax), +# so every file uses the default +# - no Benchmark.ips sits inside a method that never runs from the top of the file, +# which would benchmark nothing at all +# +# Usage: ruby .github/scripts/lint-benchmarks.rb [files...] (needs Ruby 3.3+) +require "prism" + +# Calls named `name` that have a block, not looking inside the ones found. +def find_calls(node, name, found = []) + return found unless node + + if node.is_a?(Prism::CallNode) && node.name == name && node.block + found << node + else + node.compact_child_nodes.each { |child| find_calls(child, name, found) } + end + found +end + +def any_call?(node, &test) + return false unless node + return true if node.is_a?(Prism::CallNode) && test.call(node) + + node.compact_child_nodes.any? { |child| any_call?(child, &test) } +end + +TIMING_KEYS = %w[time warmup].freeze + +# x.time = 20, x.warmup = 5, or x.config with a time or warmup key, +# in any hash syntax (time: 20, :time => 20). +def sets_timing?(node) + any_call?(node) do |call| + next true if %i[time= warmup=].include?(call.name) + next false unless call.name == :config && call.arguments + + call.arguments.arguments.any? do |arg| + next false unless arg.is_a?(Prism::KeywordHashNode) || arg.is_a?(Prism::HashNode) + + arg.elements.any? do |element| + element.is_a?(Prism::AssocNode) && element.key.is_a?(Prism::SymbolNode) && + TIMING_KEYS.include?(element.key.unescaped) + end + end + end +end + +# Every method is known by its name, and by its scope +# ("Foo#run", or "#run" at the top of the file). +# A class method is also known by its full class name ("A::B.run"), +# whether defined as `def self.run` or inside `class << self`, +# so a call like `A.run` does not count as calling `C.run`. +Definition = Struct.new(:keys, :node, :owner) + +def nested_name(owner, node) + [owner, node.constant_path.slice].compact.join("::") +end + +def definitions(node, owner = nil, singleton = false, found = []) + return found unless node + + case node + when Prism::ClassNode, Prism::ModuleNode + owner = nested_name(owner, node) + singleton = false + when Prism::SingletonClassNode + singleton = node.expression.is_a?(Prism::SelfNode) + when Prism::DefNode + class_method = owner && (singleton || node.receiver.is_a?(Prism::SelfNode)) + keys = [node.name.to_s, "#{owner}##{node.name}"] + keys << "#{owner}.#{node.name}" if class_method + found << Definition.new(keys, node, owner) + end + node.compact_child_nodes.each { |child| definitions(child, owner, singleton, found) } + found +end + +# Keys a call can reach: +# - a call without a receiver reaches a method in the same class or at the +# top of the file when the file defines one, otherwise any method of that +# name (one from a parent class or an included module); +# - `Foo.run` reaches "Foo.run" when the file defines it, otherwise any `run`; +# - a call on another object (`Foo.new.run`) reaches any `run`; +# - calls on a variable (x.report, x.compare!) are the benchmark's own API. +def keys_of(call, owner, known) + receiver = call.receiver + case receiver + when Prism::LocalVariableReadNode + [] + when nil, Prism::SelfNode + scoped = ["##{call.name}", *("#{owner}##{call.name}" if owner)] + scoped.intersect?(known) ? scoped : [call.name.to_s] + when Prism::ConstantReadNode, Prism::ConstantPathNode + key = "#{receiver.slice}.#{call.name}" + known.include?(key) ? [key] : [call.name.to_s] + else + [call.name.to_s] + end +end + +# Keys of the methods called anywhere under `node`, from code in class +# `owner`. With `top_level: true`, only calls outside any method: +# the file's entry points. +def called_keys(node, known, owner: nil, top_level: false, keys: []) + return keys unless node + return keys if top_level && node.is_a?(Prism::DefNode) + + owner = nested_name(owner, node) if node.is_a?(Prism::ClassNode) || node.is_a?(Prism::ModuleNode) + keys.concat(keys_of(node, owner, known)) if node.is_a?(Prism::CallNode) + node.compact_child_nodes.each do |child| + called_keys(child, known, owner: owner, top_level: top_level, keys: keys) + end + keys +end + +# Methods that run when the file runs: called from top-level code, or from a method that does. +def reachable_methods(root, methods) + known = methods.flat_map { |m| m.keys.drop(1) } + keys = called_keys(root, known, top_level: true) + reached = [] + loop do + newly = (methods - reached).select { |m| m.keys.intersect?(keys) } + break if newly.empty? + + reached.concat(newly) + newly.each { |m| keys.concat(called_keys(m.node.body, known, owner: m.owner)) } + end + reached +end + +def lint(file) + result = Prism.parse_file(file) + return ["does not parse: #{result.errors.first.message}"] if result.failure? + + blocks = find_calls(result.value, :ips) + return ["no Benchmark.ips block"] if blocks.empty? + + methods = definitions(result.value) + reached = reachable_methods(result.value, methods) + problems = [] + + blocks.each_with_index do |ips, index| + where = blocks.size > 1 ? "block #{index + 1} (line #{ips.location.start_line})" : "Benchmark.ips" + + problems << "#{where}: no x.compare!" unless any_call?(ips.block) { |call| call.name == :compare! } + problems << "#{where}: remove the timing arguments, use the default" if ips.arguments + problems << "#{where}: remove the timing settings, use the default" if sets_timing?(ips.block) + + # The innermost method around this Benchmark.ips, if any. + owner = methods.select do |m| + m.node.location.start_offset <= ips.location.start_offset && ips.location.end_offset <= m.node.location.end_offset + end.min_by { |m| m.node.location.length } + + if owner && !reached.include?(owner) + name = owner.keys.last.delete_prefix("#") + problems << "#{where}: inside `def #{name}`, which never runs from the top of the file" + end + end + problems +end + +files = ARGV.empty? ? Dir["code/**/*.rb"].sort : ARGV +problems = files.flat_map { |file| lint(file).map { |problem| "#{file}: #{problem}" } } + +if problems.empty? + puts "All #{files.size} benchmark files have the expected shape." +else + noun = problems.size == 1 ? "problem" : "problems" + puts problems, "", "#{problems.size} #{noun}, see \"Note on entry\" in CONTRIBUTING.md." + exit 1 +end diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 72bcaf36..20c98b14 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -24,9 +24,21 @@ jobs: # A pull_request checks out a merge commit: HEAD^1 is the base. run: .github/scripts/pick-benchmarks.sh "${{ github.event_name == 'pull_request' && 'HEAD^1' || github.event.before }}" + # Every benchmark has the same shape (see lint-benchmarks.rb). Reads the + # files without running them, on the newest Ruby: the lint needs Prism. + lint: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + - name: Lint benchmarks + run: docker compose run --rm -T --entrypoint ruby ruby_4.0 .github/scripts/lint-benchmarks.rb + + # Waits for the lint: if it fails, the PR cannot merge, so the benchmark + # jobs would only hold the org's job slots for nothing. rake: name: rake (${{ matrix.ruby }}${{ matrix.variant && format('+{0}', matrix.variant) || '' }}) - needs: changes + needs: [changes, lint] if: needs.changes.outputs.run == 'true' runs-on: ubuntu-latest timeout-minutes: 60 @@ -90,11 +102,11 @@ jobs: # The check to require on main. Passes when every benchmark job passed, or # when there was nothing to benchmark. benchmarks-ok: - needs: [changes, rake] + needs: [changes, lint, rake] if: always() runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - name: Check the benchmark jobs - run: .github/scripts/check-benchmarks.sh ${{ needs.changes.result }} ${{ needs.rake.result }} + run: .github/scripts/check-benchmarks.sh ${{ needs.changes.result }} ${{ needs.lint.result }} ${{ needs.rake.result }} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a880650f..3f84750a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,6 +35,12 @@ Benchmark.ips do |x| end ``` +Keep that shape: end every `Benchmark.ips` block with `x.compare!`, keep the +default timing (no `Benchmark.ips(20)`, `x.time = ...` or `x.config(time: ...)`), +so every entry is measured the same way, and make sure +the file actually calls `Benchmark.ips` when it runs +(not only inside a method nothing calls). CI checks all three. + Run your result: ```