diff --git a/.github/scripts/check-benchmarks.sh b/.github/scripts/check-benchmarks.sh index d5bebb8..eff2bcc 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 0000000..48c166f --- /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 72bcaf3..20c98b1 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 a880650..3f84750 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: ``` diff --git a/code/enumerable/select-first-vs-detect.rb b/code/enumerable/select-first-vs-detect.rb index 6b7f0bf..f535f69 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! diff --git a/code/string/concatenation_randomized.rb b/code/string/concatenation_randomized.rb deleted file mode 100644 index 8a5e7a9..0000000 --- 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