From 84eabe75dd913b60f7c7c755de29b964c8229acc Mon Sep 17 00:00:00 2001 From: Nick Zadrozny Date: Wed, 12 Aug 2026 12:31:20 -0500 Subject: [PATCH 1/2] Guard a wrapped read with a flag on the record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recursion guard kept its bookkeeping in a thread-local hash, which meant building an [object_id, name] key on every wrapped attribute read — an allocation and two hash operations to protect against a case that almost never happens, on the hot path of every converted model. Measured by benchmark:optional_reader, a wrapped read cost 511 ns against 116 ns for the plain reader it replaces, where wrapping without any guard costs 212 ns: the guard was more expensive than the work it guarded. A flag on the record needs no key, so the same protection now costs about 30 ns and one allocation per read is the Option itself. A test pins the allocation count by differencing two batches, so the cost cannot creep back unnoticed, and the benchmark stays in the tree to compare against later. The tradeoff: a record read from two threads at the same time could see the other thread's flag. An ActiveRecord instance shared that way is already outside what ActiveRecord supports. Rubocop is relaxed for test/ and benchmark/ in the same pass: the size cops were pushing tests to be split apart, which hides the behavior each one names. --- .rubocop.yml | 28 +++++-- Rakefile | 7 ++ benchmark/optional_reader.rb | 79 +++++++++++++++++++ .../rails/active_record_optional.rb | 13 +-- test/rails_test.rb | 19 +++++ 5 files changed, 135 insertions(+), 11 deletions(-) create mode 100644 benchmark/optional_reader.rb 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..027d055 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,16 @@ 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] + if @__errgonomic_reading_#{name} raise Errgonomic::RecursiveOptionalReadError, "\#{self.class}##{name} re-entered itself; something beneath this reader reads it again" 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 } From a66ead7fa93217e14f46f61e7686ef7cb78935ba Mon Sep 17 00:00:00 2001 From: Nick Zadrozny Date: Wed, 12 Aug 2026 12:36:05 -0500 Subject: [PATCH 2/2] Point a recursion error at a bug report Actual recursion through a wrapped reader is rare, and the flag the guard uses cannot tell it apart from the same record being read by two threads at once. Whoever hits the second case would be reading a message about the first, so the message names that possibility and where to report it. --- lib/errgonomic/rails/active_record_optional.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/errgonomic/rails/active_record_optional.rb b/lib/errgonomic/rails/active_record_optional.rb index 027d055..ee40ceb 100644 --- a/lib/errgonomic/rails/active_record_optional.rb +++ b/lib/errgonomic/rails/active_record_optional.rb @@ -182,8 +182,12 @@ def errgonomic_wrap_optional(name) class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{name} if @__errgonomic_reading_#{name} - raise Errgonomic::RecursiveOptionalReadError, - "\#{self.class}##{name} re-entered itself; something beneath this reader reads it again" + 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 @__errgonomic_reading_#{name} = true