Remove OpenStruct from Context#3
Conversation
c6f4a65 to
276a605
Compare
276a605 to
a320911
Compare
murat-atak
left a comment
murat-atak
left a comment
There was a problem hiding this comment.
Solid perf goal — flagging one blocking behavior change (the OpenStruct accessor-shadowing the docsketch consumer relies on is lost) plus the test gap that hides it, both inline.
…e.g. to_json) read back stored values
|
Pushed a follow-up commit ( The problem. With a plain The fix. Mirror what OpenStruct did: on the first write of a key, define a getter (and setter) on the instance's singleton class so it outranks any inherited method. The Tests added. Verification.
|
Verification resultsVerified the OpenStruct removal (this branch, including the singleton-accessor and Correctness — spec suites
The docsketch runs wire this branch in via a local path and exercise the API layer that reads Behavioral / manual scenarios (run inside the app, ActiveSupport loaded) — all PASSBoth Rubies: method get/set, Performance / stress
Set+get throughput is on par with Threads16 threads × 300 independent contexts → PASS on both Rubies. (Each interactor invocation owns its own IndependenceBecause the change is behavior-preserving and pure Ruby (green on 3.2.8 and 3.3.11), it can ship on the current 3.2.8 production ahead of the Ruby upgrade, isolating the two changes. |
murat-atak
left a comment
murat-atak
left a comment
There was a problem hiding this comment.
Re-reviewed at the latest commit — both points from my earlier review are resolved, and the to_s fix Cristian raised is in too. The to_json regression is handled the OpenStruct way: a per-key accessor defined on first write via define_singleton_method, guarded by method_defined?(key, false) so it outranks Object#to_json, and initialize_copy re-installs it on dup. There's now a spec for the inherited-method-shadowing case (via Kernel#display, so no ActiveSupport dependency), the to_s → inspect alias is back, and the ostruct runtime dependency is gone. I also ran the full Docsketch suite against this gem on both 3.2.8 and 3.3.11 — green on both, and a direct to_json check with ActiveSupport loaded returns the stored value. LGTM, approving.
The previous implementation defined a getter/setter on the instance's singleton class for *every* context key on first write. That had two problems for a production drop-in replacement of OpenStruct: - A key whose name matched a method the Context itself relies on (e.g. `hash`, `to_h`, `==`, `_called`) silently overrode that method for the instance, corrupting the object. `method_missing`/`==` reaching into `other.send(:table)` also broke if a key named `send` was ever stored. - Every key paid for two singleton-method definitions, allocating a per-instance singleton class even for ordinary contexts (the common, short-lived case in interactors), defeating inline caches. Now an accessor is installed only when a key would otherwise be intercepted by an inherited method (the original `to_json` motivation). Plain keys are served by `method_missing` with no singleton methods at all. A frozen `RESERVED_NAMES` set lists the names the class depends on; those are stored in `@table` and remain reachable via `#[]`/`#to_h` but are never shadowed. A spec asserts every Context-defined method is reserved, so future additions can't silently regress. Equality now compares `@table` directly via `instance_variable_get` (zero allocation, no `send`, immune to a stored `send`/`table` key), and `#freeze` freezes the backing table so writes to a frozen context raise FrozenError as they did with OpenStruct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two silent regressions versus the OpenStruct-backed Context that downstream production code can hit: - Marshal: any context with at least one attribute raised "TypeError: singleton can't be dumped" because set keys may define singleton methods. Code that caches contexts or enqueues them (Rails cache, Sidekiq args) would break. marshal_dump now serializes only @table and marshal_load rebuilds accessors, so contexts round-trip; transient state (called list, failure/halted flags) is intentionally reset, matching a freshly built context. - dig: OpenStruct supports `context.dig(:a, :b)`. The custom Context inherited no #dig, so nested lookups silently returned nil. Added a delegation to Hash#dig that normalises the first key to a symbol, as the other accessors do. The accessor-rebuild loop shared by marshal_load and initialize_copy is extracted into a private #rebuild_accessors so the two paths can't drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two small behavioural gaps versus OpenStruct: - to_h ignored a block. OpenStruct#to_h (Ruby 2.6+) yields each key/value pair for transformation; the custom Context silently dropped the block. It now forwards to Hash#to_h(&block), while the no-block path keeps returning @table.dup (Hash#to_h without a block returns self, which would leak the internal table). - A getter invoked with arguments (context.foo(1)) silently returned the value instead of raising. OpenStruct raises ArgumentError, which surfaces caller bugs; method_missing now does the same. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hardening the custom
|
juankisardin
left a comment
juankisardin
left a comment
There was a problem hiding this comment.
Reviewed the swap from OpenStruct to the hand-rolled Context (lib/interactor/context.rb), along with the smaller changes to lib/interactor.rb, the gemspec, and the CI workflow.
This is careful, well-considered work. The accessor strategy — install a per-key method only when a key would otherwise collide with an inherited method, and let everything else fall through to method_missing — is a real improvement over inheriting from OpenStruct, and the edge cases that usually get missed are all handled here: freeze, Marshal round-trips, dup/clone resetting transient state, pattern matching, and keeping to_s readable so failure messages stay legible. The comments explaining why each of those exists are genuinely valuable.
The findings below are minor and all of one kind: a couple of places where the same piece of knowledge is written down twice and will quietly drift apart over time, plus one small bit of code that does nothing.
…ead setter Three points from review feedback on the lazy-accessor implementation: - RESERVED_NAMES duplicated the names of methods defined just below it, so adding or renaming a method meant remembering to edit the list or risk a key shadowing it. The class's own API half is now derived from instance_methods(false) + private_instance_methods(false); only the inherited Object/Kernel protocol (dup, class, send, ...) stays hand-listed as CORE_PROTOCOL, since that genuinely can't be derived. - #== / #eql? reached into the other instance's @table via instance_variable_get. Replaced with a protected attr_reader :table so neither method depends on the other object's ivar layout. - define_accessor also defined a "#{key}=" singleton setter that never ran: setter names end in "=", which no inherited method does, so writes always route through method_missing to #[]=. Only the reader needs a singleton override to outrank the inherited method. Dropped it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
define_accessor? decided whether to install a shadowing accessor with `singleton_class.method_defined?(key, false)` followed by `respond_to?(key, true)`. Two problems, both YJIT-hostile: - Calling `singleton_class` materialises a per-instance singleton class on the first write of ANY key, so every context got its own class. - `respond_to?` is true for any stored key (via respond_to_missing?), so overwriting a plain key installed a needless singleton getter on the second write. Either way ordinary contexts ended up with a unique singleton class, which makes `context.attr` call sites megamorphic and defeats YJIT's inline caches - the opposite of what the lazy-accessor design intended. Decide collision from the class's real method table instead (`self.class.method_defined?` / `private_method_defined?`): it ignores method_missing and never touches the instance's singleton class, so a singleton class is now created only for the rare key that genuinely shadows an inherited method (e.g. to_json). Measured: 2000 plain-key contexts went from 2000 singleton classes to 0; YJIT read throughput +11% on a megamorphic read site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
initialize set only @table; @called/@failure/@halted/@rolled_back were created lazily by _called/fail!/halt!/rollback!. So a fresh, a failed and a halted context each had a different set of instance variables — i.e. a different object shape. Under YJIT that makes even @table reads (every attribute access) and the flag reads in success?/failure?/halted? dispatch on different shape ids, so their inline caches go polymorphic. Assign all five ivars up front in initialize, in a fixed order, via a shared reset_state helper also used by marshal_load; initialize_copy resets the same way and then carries over the called list. fail!/halt!/ rollback! now only mutate existing slots, never fork the shape. Verified: fresh, failed, halted, dup'd and Marshal-loaded contexts all report the same instance_variables in the same order (one shape); a spec locks this in so a future lazy ivar can't silently regress it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
YJIT benchmark: before vs after the two performance commitsFollowing review, two commits make the hand-rolled Commits
Method. Ruby 3.2.8 +YJIT (arm64). 2,000,000 invocations of a realistic path — build a Results
Reading the numbers. Before, every context built a singleton class on first write and had a state-dependent shape, so YJIT's inline caches went megamorphic and it barely helped (+17%). After, ordinary contexts stay on the base Behaviour is unchanged: 170 specs pass, |
YJIT benchmark: whole PR vs
|
| Workload | master (OpenStruct) |
PR (0717dad) |
Δ |
|---|---|---|---|
| Invocations/s — YJIT on | 0.10 M/s (19.47 s) | 0.80 M/s (2.51 s) | ~8.0× faster |
| Invocations/s — YJIT off | 0.11 M/s (18.06 s) | 0.50 M/s (4.01 s) | ~4.5× faster |
| YJIT speedup (on vs off) | ~0.9× (none) | +60% | PR lets YJIT help |
| Singleton classes per 50k contexts | 50,000 | 0 | — |
Reading the numbers. OpenStruct defines getter/setter methods on each instance's singleton class, so every context gets its own dispatch class — and on this path YJIT couldn't recover its overhead, ending up marginally slower than interpreted (0.10 vs 0.11 M/s, within run-to-run noise). The PR keeps ordinary contexts on the base Interactor::Context class with a single object shape, so dispatch and ivar reads stay monomorphic: ~8× throughput under YJIT, and YJIT now contributes +60% instead of nothing. The 50k singleton classes per 50k contexts also disappear, cutting allocation and GC pressure.
Of that ~8×, the OpenStruct removal itself is the larger share; the two YJIT-specific commits (6745725, 0717dad) account for ~3× on top (measured in the previous comment). Behaviour is unchanged: 170 specs pass, standard clean.
Single-machine, best-of-3; absolute throughput will vary by host, but the relative gap is stable across runs.
murat-atak
left a comment
murat-atak
left a comment
There was a problem hiding this comment.
Re-reviewed the two YJIT commits (one object shape + lazy singleton accessor only for shadowing keys) — 170/170 specs, standard clean, no correctness issues. Nothing blocking. 👍
|
|
Understood — treating Confirmed right now:
Going forward: no delete and no force-push of this branch. Any further work would only add commits on top (fast-forward), which keeps Leaving the merge/release/re-pin sequence to the maintainers per your steps 1–4, and not resolving this thread so the warning stays visible until docsketch is re-pinned. Happy to help cut the merge-commit/release tag pointing at 🤖 Addressed by Claude Code |
|
Thanks for protecting the commit 🙏 Production is confirmed healthy on So please go ahead with the merge + release:
Once it's tagged, we'll open the docsketch follow-up to swap the pin |
4 participants
The
Contextclass has been updated to replace the use ofOpenStructwith a custom implementation. This change improves performance and enhances type safety within the codebase.