diff --git a/.rubocop.yml b/.rubocop.yml index 55ddda0..6a10c13 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -4,7 +4,7 @@ plugins: rubocop-yard # the assertions. Long lines carrying a `#=>` expectation are allowed. Layout/LineLength: AllowedPatterns: - - '#=>' + - "#=>" # Some(), None(), Ok() and Err() are the library's Rust-style value # constructors; their capitalized names are the point. @@ -19,6 +19,7 @@ Metrics/ClassLength: - lib/errgonomic/option.rb - lib/errgonomic/result.rb - test/**/* + - benchmark/**/* # core_ext vendors ActiveSupport's blank?/present? patches; the reopened core # classes there are explained by the file header, not per class. Test @@ -29,21 +30,36 @@ Style/Documentation: - test/**/* # A test states one behavior end to end, and splitting one to satisfy a size -# metric hides the behavior it was written to name. A cop that names its own -# Exclude replaces this one, so Metrics/ClassLength repeats the path above. +# metric hides the behavior it was written to name. Size cops do not apply +# under test/ or benchmark/. A cop that names its own Exclude replaces this +# one, so those repeat the two paths. Metrics: Exclude: - test/**/* + - benchmark/**/* # The ActiveRecord hooks in the optional concern are one subject — where a -# reader may come from and what leaves it alone — and reading them together is -# the point. The generated reader is a heredoc, which the length cops count as -# if it were code. +# reader may come from and what leaves it alone — and reading them together +# is the point. The generated reader is a heredoc, which the length cops +# count as if it were code. Metrics/BlockLength: Exclude: - lib/errgonomic/rails/active_record_optional.rb - test/**/* + - benchmark/**/* Metrics/MethodLength: Exclude: - lib/errgonomic/rails/active_record_optional.rb - test/**/* + - benchmark/**/* + +# has_one overrides ActiveRecord's association macro; it is not a predicate. +Naming/PredicatePrefix: + AllowedMethods: + - has_one + +# A benchmark prints a table, where the format tokens are positional by +# nature and naming each one only adds noise. +Style/FormatStringToken: + Exclude: + - benchmark/**/* diff --git a/Rakefile b/Rakefile index a72214c..b73f77a 100644 --- a/Rakefile +++ b/Rakefile @@ -18,6 +18,13 @@ end task default: %i[test yard:doctest] +namespace :benchmark do + desc 'Cost of a wrapped attribute read, against the plain reader it replaces' + task :optional_reader do + ruby '-Ilib benchmark/optional_reader.rb' + end +end + namespace :gems4nix do desc 'Regenerate gem-groups.json after Gemfile/Gemfile.lock changes' task :groups do diff --git a/benchmark/optional_reader.rb b/benchmark/optional_reader.rb new file mode 100644 index 0000000..1d624c5 --- /dev/null +++ b/benchmark/optional_reader.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +# What a wrapped attribute read costs, against the plain reader it replaces. +# +# Every converted model pays this on every read of a nullable column, so the +# reader is the one place in the library where a few nanoseconds matter. Two +# figures are reported per shape: wall time per read, and objects allocated +# per read, measured as the marginal cost of a second batch so the harness +# itself cancels out. +# +# rake benchmark:optional_reader + +require 'active_record' +require 'benchmark' +require 'logger' + +require_relative '../lib/errgonomic/rails' + +ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:') +ActiveRecord::Base.logger = Logger.new(File::NULL) + +ActiveRecord::Schema.verbose = false +ActiveRecord::Schema.define do + create_table 'readings', force: :cascade do |t| + t.string :note + end +end + +Errgonomic::Rails.setup_before + +# The reader ActiveRecord would have given us, as the floor. +class PlainReading < ActiveRecord::Base + self.table_name = 'readings' +end + +# The reader the concern generates, guard and all. +class WrappedReading < ActiveRecord::Base + self.table_name = 'readings' + include Errgonomic::Rails::ActiveRecordOptional +end + +# Wrapping with no guard at all, as the ceiling on what the guard may cost: +# the difference between this and WrappedReading is the guard's price. +class UnguardedReading < ActiveRecord::Base + self.table_name = 'readings' + + def note + val = super + val.nil? ? Errgonomic::Option::None.new : Errgonomic::Option::Some.new(val) + end +end + +SHAPES = { 'plain' => PlainReading, 'wrapped' => WrappedReading, 'unguarded' => UnguardedReading }.freeze +READS = 200_000 + +def nanoseconds_per_read(record) + record.note + Benchmark.realtime { READS.times { record.note } } / READS * 1e9 +end + +def allocations_per_read(record) + record.note + before = GC.stat(:total_allocated_objects) + READS.times { record.note } + one_batch = GC.stat(:total_allocated_objects) + (READS * 2).times { record.note } + two_batches = GC.stat(:total_allocated_objects) - one_batch + (two_batches - (one_batch - before)).fdiv(READS) +end + +records = SHAPES.transform_values { |klass| klass.new(note: 'present') } +baseline = nanoseconds_per_read(records.fetch('plain')) + +puts format('%-10s %10s %10s %14s', 'shape', 'ns/read', 'vs plain', 'allocs/read') +records.each do |name, record| + nanoseconds = nanoseconds_per_read(record) + puts format('%-10s %10.0f %9.1fx %14.1f', name, nanoseconds, nanoseconds / baseline, + allocations_per_read(record)) +end diff --git a/lib/errgonomic/rails/active_record_optional.rb b/lib/errgonomic/rails/active_record_optional.rb index d70d80f..ee40ceb 100644 --- a/lib/errgonomic/rails/active_record_optional.rb +++ b/lib/errgonomic/rails/active_record_optional.rb @@ -168,6 +168,11 @@ def errgonomic_unwrap_optionals(*names) end end + # The reader guards against re-entering itself with a flag on the + # record. Bookkeeping shared across records would have to build a key + # per read, and this reader is on the hot path of every wrapped + # attribute. A record read from two threads at once is out of scope, + # as it is for ActiveRecord itself. def errgonomic_wrap_optional(name) name = name.to_s return if errgonomic_optional_off? @@ -176,18 +181,20 @@ def errgonomic_wrap_optional(name) errgonomic_optional_names << name class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{name} - reads = Thread.current[:errgonomic_optional_reads] ||= {} - key = [object_id, :#{name}] - if reads[key] - raise Errgonomic::RecursiveOptionalReadError, - "\#{self.class}##{name} re-entered itself; something beneath this reader reads it again" + if @__errgonomic_reading_#{name} + raise Errgonomic::RecursiveOptionalReadError, <<~MSG + \#{self.class}##{name} re-entered itself; something beneath this reader reads it again. + If this read was not recursive, the record may have been read from two threads at once, + which this guard cannot tell apart. Please report that at + https://github.com/omc/errgonomic/issues + MSG end - reads[key] = true + @__errgonomic_reading_#{name} = true begin val = super ensure - reads.delete(key) + @__errgonomic_reading_#{name} = false end val.nil? ? Errgonomic::Option::None.new : Errgonomic::Option::Some.new(val) end diff --git a/test/rails_test.rb b/test/rails_test.rb index 4d7e334..cbfece4 100644 --- a/test/rails_test.rb +++ b/test/rails_test.rb @@ -305,6 +305,25 @@ def test_optional_attributes_read_on_a_deep_stack deeper(1100) { assert_equal 'writes sci-fi', author.bio.unwrap! } end + # A wrapped read sits on the hot path of every converted model, so the only + # allocation it may make is the Option it returns. + def test_a_wrapped_read_allocates_only_the_option_it_returns + author = Author.create!(name: 'Cixin Liu', bio: 'writes sci-fi') + author.bio + + # Difference two runs so the measurement itself cancels out and only the + # per-read allocation is left. + before = GC.stat(:total_allocated_objects) + 100.times { author.bio } + hundred = GC.stat(:total_allocated_objects) + 200.times { author.bio } + three_hundred = GC.stat(:total_allocated_objects) + + assert_equal 100, (three_hundred - hundred) - (hundred - before) + end + + # A read that re-enters itself still has to say so at the first re-entry, + # naming the reader, rather than unwinding as a stack overflow. def test_recursive_read_raises_a_named_error_at_first_reentry author = LoopyAuthor.create!(name: 'Cixin Liu', bio: 'writes sci-fi') error = assert_raises(Errgonomic::RecursiveOptionalReadError) { author.bio }