Add record/delegate fast path for OP_RT_GET_PROP in VM#83
Conversation
Dynamic property access via OP_RT_GET_PROP now checks if the receiver is a TSouffleRecord before calling FRuntimeOps.GetProperty. When it is, the record is queried directly, then the delegate chain is walked. This avoids the virtual method call + full runtime GetProperty (type switching, bridge caching) for the common case of accessing properties on objects. Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe VM's property-get opcode (OP_RT_GET_PROP) now tries a TSouffleRecord fast-path: direct Get, then DelegateGet, and only if both fail falls back to FRuntimeOps.GetProperty; non-record bases use the original runtime GetProperty path. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant VM as VM (ExecuteRuntimeOp)
participant Record as TSouffleRecord
participant Delegate as Delegate (owner/parent)
participant Runtime as FRuntimeOps
VM->>Record: If base is record -> Get(PropKey)
alt Get returns value
Record-->>VM: PropVal (assign)
else Get fails
Record->>Delegate: DelegateGet(PropKey)
alt Delegate returns value
Delegate-->>VM: PropVal (assign)
else Delegate fails
VM->>Runtime: GetProperty(base, PropKey)
Runtime-->>VM: PropVal (assign or error)
end
end
alt base not a record
VM->>Runtime: GetProperty(base, PropKey)
Runtime-->>VM: PropVal (assign or error)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Suite Timing
Measured on ubuntu-latest x64. |
Benchmark Results254 benchmarks Interpreted: 🟢 10 improved · 🔴 71 regressed · 173 unchanged · avg -3.8% arraybuffer.js — Interp: 🔴 10, 4 unch. · avg -9.7% · Bytecode: 14 unch. · avg +2.3%
arrays.js — Interp: 🔴 8, 11 unch. · avg -6.5% · Bytecode: 🟢 1, 18 unch. · avg +1.4%
async-await.js — Interp: 🔴 5, 1 unch. · avg -10.5% · Bytecode: 6 unch. · avg +2.3%
classes.js — Interp: 🔴 28, 3 unch. · avg -9.6% · Bytecode: 31 unch. · avg +0.3%
closures.js — Interp: 🔴 1, 10 unch. · avg -5.0% · Bytecode: 11 unch. · avg +0.1%
collections.js — Interp: 🔴 1, 11 unch. · avg -4.4% · Bytecode: 12 unch. · avg -0.0%
destructuring.js — Interp: 🔴 13, 9 unch. · avg -7.3% · Bytecode: 🟢 2, 20 unch. · avg +1.9%
fibonacci.js — Interp: 8 unch. · avg -2.9% · Bytecode: 8 unch. · avg -1.2%
for-of.js — Interp: 7 unch. · avg -0.9% · Bytecode: 7 unch. · avg +0.6%
iterators.js — Interp: 20 unch. · avg -0.4% · Bytecode: 🔴 1, 19 unch. · avg +0.6%
json.js — Interp: 20 unch. · avg -3.2% · Bytecode: 🟢 1, 19 unch. · avg +1.8%
jsx.jsx — Interp: 🔴 1, 20 unch. · avg -4.2% · Bytecode: 21 unch. · avg +0.9%
numbers.js — Interp: 🔴 1, 10 unch. · avg -3.3% · Bytecode: 11 unch. · avg +2.1%
objects.js — Interp: 🟢 1, 6 unch. · avg +1.9% · Bytecode: 7 unch. · avg +1.0%
promises.js — Interp: 🟢 6, 6 unch. · avg +7.7% · Bytecode: 🔴 1, 11 unch. · avg +0.4%
strings.js — Interp: 🟢 1, 10 unch. · avg +4.5% · Bytecode: 🔴 1, 10 unch. · avg -0.7%
typed-arrays.js — Interp: 🟢 2, 🔴 3, 17 unch. · avg -0.6% · Bytecode: 🟢 3, 19 unch. · avg +2.9%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
|
TODO: Let's re-review this after #81 |
- Rewrite optimization-log.md with correct PR numbers for all 22 PRs since #73 (previous version had multiple wrong PR-to-description mappings) - Add missing PRs: #73, #74, #75, #76, #77, #79, #80, #81, #82, #83, #85, #88, #89, plus infrastructure PRs #90, #92, #98 - Fix GC section: Collect-after-each-file is deliberate, not a workaround; any GC revisit needs fresh analysis - Fix minor items: link to actual issues #103 and #104, correct naming convention context (shared units should NOT use Goccia.* prefix) - Create docs/decision-log.md with 1-3 sentence summaries linking to detailed documentation - Link fpc-generics-performance.md and fpc-dispatch-performance.md from AGENTS.md, code-style.md, design-decisions.md, and optimization-log.md Made-with: Cursor
…105) * Convert spike PDFs to Markdown, update VM docs, add optimization log - Convert all 4 PDF spike documents (string performance, hashmap performance, generics performance, dispatch performance) to Markdown for better AI agent parsing - Fix stale TSouffleValue documentation: 26 bytes → 16 bytes, string[23] → string[13] across souffle-vm.md, design-decisions.md - Update test count to 3,501 across all docs (AGENTS.md, README.md, souffle-vm.md, design-decisions.md) - Add svkString to TSouffleValue kinds list in AGENTS.md - Add THashMap optimization details to AGENTS.md component table - Add docs/optimization-log.md with full VM optimization history, experiment results, remaining work, and key insights for fresh context pickup - Add hashmap spike update note documenting TScopeMap removal, TOrderedStringMap refactoring, and THashMap hash optimization Made-with: Cursor * Fix optimization log PR numbers, add decision log, link all spikes - Rewrite optimization-log.md with correct PR numbers for all 22 PRs since #73 (previous version had multiple wrong PR-to-description mappings) - Add missing PRs: #73, #74, #75, #76, #77, #79, #80, #81, #82, #83, #85, #88, #89, plus infrastructure PRs #90, #92, #98 - Fix GC section: Collect-after-each-file is deliberate, not a workaround; any GC revisit needs fresh analysis - Fix minor items: link to actual issues #103 and #104, correct naming convention context (shared units should NOT use Goccia.* prefix) - Create docs/decision-log.md with 1-3 sentence summaries linking to detailed documentation - Link fpc-generics-performance.md and fpc-dispatch-performance.md from AGENTS.md, code-style.md, design-decisions.md, and optimization-log.md Made-with: Cursor * Address PR review comments: fix ratios, bytes/chars, stale refs - Fix generics spike ratio values (arithmetic errors from original PDF) - Change "13 characters" to "13 bytes" in souffle-vm.md (FPC string[N] is byte-counted), add UTF-8 note - Mark TScopeMap as removed/historical in hashmap spike document - Fix Key Insight #4 PR reference (was #88 async/await, not boolean) - Remove stale GOCCIA_EXT_EVAL_CLASS / FPendingClasses / decorator bridge references from AGENTS.md, souffle-vm.md, design-decisions.md (decorators are now compiled natively via BEGIN/APPLY/FINISH opcodes) - Fix "44 second" → "44-second" hyphenation in string spike Made-with: Cursor * Expand TScopeMap history in hashmap spike with full timeline The spike originally recommended TScopeMap for scope bindings based on micro-benchmarks at N≤10. After implementation (PR #66), real-world profiling showed the assumption about scope sizes was incorrect — global and bridged scopes far exceed 20 bindings, causing a 2.7× regression. The update section now tells the complete timeline: original recommendation → implementation → profiling discovery → revert. Made-with: Cursor * Add grill-me interview skill Introduce a new 'grill-me' agent skill that relentlessly interviews users about a plan or design, exploring each branch of the decision tree until a shared understanding is reached and dependencies are resolved. The skill prefers to inspect the codebase when that can answer a question. Updated skills lock to include the new skill.
Mirror PR #83's GET fast path: when the receiver is a TSouffleRecord, call PutChecked() directly instead of bridging to FRuntimeOps.SetProperty(). Non-record types (blueprints, setters, private fields) still fall through to the bridge for full property resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add record fast path for OP_RT_SET_PROP in VM dispatch Mirror PR #83's GET fast path: when the receiver is a TSouffleRecord, call PutChecked() directly instead of bridging to FRuntimeOps.SetProperty(). Non-record types (blueprints, setters, private fields) still fall through to the bridge for full property resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix SET_PROP fast path: skip records with blueprints (setters/frozen) Records with a Blueprint may have setters, non-writable properties, or frozen/sealed state that PutChecked() does not handle. Restrict the fast path to plain records (no blueprint) and fall through to the bridge for class instances. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Revert SET_PROP fast path — PutChecked misses accessor/freeze/seal semantics PutChecked does not handle accessor properties (getters/setters defined via Object.defineProperty), does not throw on non-writable/frozen/sealed violations, and does not cover Object.preventExtensions. The blueprint check alone was insufficient — plain objects modified with defineProperty, freeze, or seal also break. OP_RT_SET_PROP is only emitted for destructuring patterns, not a hot enough path to warrant a partial fast path that breaks correctness. Revert to always delegating to FRuntimeOps.SetProperty which handles the full property descriptor protocol. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Correct SET_PROP fast path: guard all property descriptor cases The fast path now only fires when ALL conditions are met: - Receiver is TSouffleRecord (not blueprint, wrapped value, etc.) - Record has no instance setters (HasSetters = false) - Record has no blueprint (no class setter hierarchy) - Key is not a private property (does not start with #) - PutChecked succeeds (property is writable AND record is extensible) If any condition fails, falls through to FRuntimeOps.SetProperty which handles the full property descriptor protocol: setter invocation, blueprint setter hierarchy, private property flags, TypeError on non-writable/frozen/sealed violations. The previous attempts failed because: - v1: No guards at all — broke setters, frozen, sealed, defineProperty - v2: Blueprint-only guard — missed Object.defineProperty accessor properties, Object.freeze, Object.seal on plain records - v3 (reverted): Gave up entirely This version correctly handles all cases by checking HasSetters, Blueprint, private keys, and PutChecked return value before committing to the fast path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Handle full record SetProperty protocol natively in VM Instead of a partial fast path that falls through to the bridge, handle the complete TSouffleRecord SetProperty protocol in the VM dispatch: 1. Instance setters: check Rec.HasSetters, look up Rec.Setters.Get(key), invoke the setter closure via ExecuteFunction — all Souffle-native 2. Blueprint setter hierarchy: walk Bp.Setters chain via new SetPropertyViaBlueprintSetter helper — all Souffle-native 3. Normal put: Rec.PutChecked handles writable/extensible checks 4. Write violation: on PutChecked failure, call FRuntimeOps.PropertyWriteViolation for the language-level TypeError Add PropertyWriteViolation as a virtual method on the base TSouffleRuntimeOperations contract (default no-op) so the VM can request the error without knowing about Goccia types. The bridge (FRuntimeOps.SetProperty) is now only reached for non-record types (TSouffleBlueprint static fields, TGocciaWrappedValue). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Test plan
Made with Cursor
Summary by CodeRabbit