Bypass interpreter bridge for blueprint-backed class construction#87
Conversation
Move the native blueprint instantiation path before PushVMFramesToGoccia so standalone classes and blueprint-inherited classes never cross the interpreter bridge. Three optimizations: 1. Eliminate bridge overhead: the common blueprint path exits early, skipping PushVMFramesToGoccia/PopVMFramesFromGoccia (O(VM_depth) per construction) and the try..except TGocciaThrowValue wrapper. 2. Delegate chain caching: guard the chain walk with `if not Assigned(Bp.Prototype.Delegate)` so subsequent constructions of the same class skip the O(depth) no-op traversal. 3. VMArgs reuse: move SetLength outside the field initializer loop to eliminate redundant dynamic array allocations per iteration. Only blueprint classes without a built-in super (FBlueprintSuperValues) take the fast path; built-in subclasses still use the full bridge. Made-with: Cursor
📝 WalkthroughWalkthroughHandle TSouffleBlueprint constructors early with a dual-path: cached-bridge lookup versus on-demand bridge creation. Allocate the instance record earlier, set Prototype/Method Delegates during prototype walking, guard and run field initializers in reverse, and separate blueprint vs non-blueprint constructor flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Runtime
participant Blueprint
participant BridgeCache
participant Bridge
participant VM
Note right of Runtime: Construct start
Caller->>Runtime: Construct(...)
Runtime->>Blueprint: is TSouffleBlueprint?
alt Blueprint path
Runtime->>Blueprint: ConvertBlueprintToClassValue(...)
Runtime->>BridgeCache: lookup bridge for blueprint
alt cached
BridgeCache-->>Runtime: cached bridge
Runtime->>Bridge: invoke cached bridge with Rec
else not cached
Runtime->>Bridge: create bridge from blueprint
Bridge-->>BridgeCache: write cache entry
Runtime->>Bridge: invoke bridge with Rec
end
else Non-blueprint path
Runtime->>VM: Push frames / prepare context
Runtime->>Bridge: build or locate bridge
Runtime->>Bridge: invoke bridge with Rec
end
Bridge-->>Runtime: constructor result
Runtime-->>Caller: return instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
units/Goccia.Runtime.Operations.pas (1)
3342-3342: UsePROP_CONSTRUCTORinstead of a string literal.This keeps constructor lookups aligned with the rest of the runtime constants.
♻️ Suggested change
- if WalkBp.Methods.Get('constructor', CtorMethod) then + if WalkBp.Methods.Get(PROP_CONSTRUCTOR, CtorMethod) thenAs per coding guidelines, "Use runtime constant units instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names,Goccia.Constants.SymbolNamesfor symbol names,Goccia.Constantsfor literal value strings"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Runtime.Operations.pas` at line 3342, Replace the hardcoded string 'constructor' with the runtime constant PROP_CONSTRUCTOR so constructor lookups use the shared constant; specifically update the call to WalkBp.Methods.Get('constructor', CtorMethod) in units/Goccia.Runtime.Operations.pas (the WalkBp.Methods.Get invocation that fills CtorMethod) to use PROP_CONSTRUCTOR from the constants unit (Goccia.Constants.PropertyNames or the module where PROP_CONSTRUCTOR is defined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 3287-3288: The branch currently uses Assigned(Bp.SuperBlueprint)
which incorrectly treats derived blueprints as roots; change the gate to test
for the root (i.e., not Assigned(Bp.SuperBlueprint)) so the native fast-path
only runs for root blueprints and not for leaves. Update the condition "if
Assigned(Bp.SuperBlueprint) or not FBlueprintSuperValues.ContainsKey(Bp) then"
to "if not Assigned(Bp.SuperBlueprint) or not
FBlueprintSuperValues.ContainsKey(Bp) then" (and make the same change in the
other occurrences around the blocks referencing Bp, FBlueprintSuperValues,
__fields__, and delegate wiring to FVM.RecordDelegate at the noted ranges).
- Around line 3373-3377: The FBlueprintBridgeCache is being reused for two
different bridge conversions (BlueprintToClassValue vs
ConvertBlueprintToClassValue), causing lost behavior when
ConvertBlueprintToClassValue is required; in the branch that currently reads
FBlueprintBridgeCache for Bp (where CachedBridge is assigned for use by
ConvertBlueprintToClassValue), change the logic to avoid reusing entries
produced by the other converter — either bypass the cache and always call
ConvertBlueprintToClassValue for this path, or key into a separate cache (e.g.,
a new FBlueprintClassBridgeCache) so entries created by
UnwrapToGocciaValue/BlueprintToClassValue are not returned here; update the code
paths referencing UnwrapToGocciaValue, ConvertBlueprintToClassValue,
BlueprintToClassValue, FBlueprintBridgeCache, and CachedBridge accordingly so
the bridge used here always includes getters/setters, static fields, private
members and field initializers.
---
Nitpick comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Line 3342: Replace the hardcoded string 'constructor' with the runtime
constant PROP_CONSTRUCTOR so constructor lookups use the shared constant;
specifically update the call to WalkBp.Methods.Get('constructor', CtorMethod) in
units/Goccia.Runtime.Operations.pas (the WalkBp.Methods.Get invocation that
fills CtorMethod) to use PROP_CONSTRUCTOR from the constants unit
(Goccia.Constants.PropertyNames or the module where PROP_CONSTRUCTOR is
defined).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c53d0e42-ca2a-49e0-9ec1-b22fad110d7b
📒 Files selected for processing (1)
units/Goccia.Runtime.Operations.pas
Suite Timing
Measured on ubuntu-latest x64. |
Benchmark Results254 benchmarks Interpreted: 🟢 3 improved · 🔴 2 regressed · 249 unchanged · avg -1.5% arraybuffer.js — Interp: 14 unch. · avg -0.5% · Bytecode: 14 unch. · avg -0.6%
arrays.js — Interp: 19 unch. · avg -1.4% · Bytecode: 🟢 1, 18 unch. · avg +1.5%
async-await.js — Interp: 6 unch. · avg -1.3% · Bytecode: 6 unch. · avg -1.6%
classes.js — Interp: 31 unch. · avg -1.1% · Bytecode: 🔴 1, 30 unch. · avg +0.0%
closures.js — Interp: 11 unch. · avg -2.0% · Bytecode: 11 unch. · avg -0.7%
collections.js — Interp: 🟢 3, 9 unch. · avg +1.4% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg +1.1%
destructuring.js — Interp: 22 unch. · avg -0.9% · Bytecode: 🟢 1, 🔴 2, 19 unch. · avg -0.7%
fibonacci.js — Interp: 8 unch. · avg -1.5% · Bytecode: 8 unch. · avg -2.0%
for-of.js — Interp: 7 unch. · avg -1.9% · Bytecode: 🔴 1, 6 unch. · avg -4.2%
iterators.js — Interp: 20 unch. · avg -2.1% · Bytecode: 🔴 3, 17 unch. · avg -3.8%
json.js — Interp: 20 unch. · avg -2.8% · Bytecode: 🟢 1, 🔴 1, 18 unch. · avg -0.3%
jsx.jsx — Interp: 21 unch. · avg -2.8% · Bytecode: 21 unch. · avg +0.5%
numbers.js — Interp: 🔴 1, 10 unch. · avg -0.5% · Bytecode: 11 unch. · avg +2.7%
objects.js — Interp: 7 unch. · avg -1.9% · Bytecode: 🔴 2, 5 unch. · avg -3.5%
promises.js — Interp: 12 unch. · avg -0.9% · Bytecode: 🔴 3, 9 unch. · avg -4.2%
strings.js — Interp: 🔴 1, 10 unch. · avg -3.5% · Bytecode: 🟢 1, 10 unch. · avg +3.2%
typed-arrays.js — Interp: 22 unch. · avg -1.8% · Bytecode: 🟢 4, 🔴 1, 17 unch. · avg +3.3%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Resolve conflict in Goccia.Runtime.Operations.pas: take the PR's restructured guard (Assigned(Bp.SuperBlueprint) or not in FBlueprintSuperValues) which routes native-pathable blueprints to the fast path. The built-in super class bridge code from main is already handled by the fallthrough to PushVMFramesToGoccia. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
units/Goccia.Runtime.Operations.pas (2)
3381-3385:⚠️ Potential issue | 🔴 CriticalAvoid reusing
FBlueprintBridgeCachefor two different blueprint converters.At Line 3381, this cache can already contain entries created by
BlueprintToClassValue(fromUnwrapToGocciaValue), but this branch requiresConvertBlueprintToClassValue. Reusing the same entry can drop constructor-path behavior.🔧 Minimal safe fix
- if not FBlueprintBridgeCache.TryGetValue(Bp, CachedBridge) then - begin - CachedBridge := ConvertBlueprintToClassValue(Bp, Self); - FBlueprintBridgeCache.Add(Bp, CachedBridge); - end; + CachedBridge := ConvertBlueprintToClassValue(Bp, Self);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Runtime.Operations.pas` around lines 3381 - 3385, FBlueprintBridgeCache is being reused for entries created by two different conversion paths (ConvertBlueprintToClassValue vs BlueprintToClassValue called from UnwrapToGocciaValue), which can cause incorrect constructor behavior; change the caching so entries are distinguished by converter or conversion path — e.g., introduce a separate cache (like FBlueprintConvertBridgeCache) or include the converter id/type in the cache key, and update the lookup/insert in the code around ConvertBlueprintToClassValue and UnwrapToGocciaValue to use the appropriate cache (or key) so ConvertBlueprintToClassValue never reuses entries created by BlueprintToClassValue.
3295-3296:⚠️ Potential issue | 🔴 CriticalGate native construction by the root blueprint mapping, not the leaf.
At Line 3295, this condition still routes derived blueprints into the native path even when their root has a wrapped super mapping, which can bypass wrapped super constructor semantics.
🔧 Minimal safe fix
- if Assigned(Bp.SuperBlueprint) or - not FBlueprintSuperValues.ContainsKey(Bp) then + WalkBp := Bp; + while Assigned(WalkBp.SuperBlueprint) do + WalkBp := WalkBp.SuperBlueprint; + if not FBlueprintSuperValues.ContainsKey(WalkBp) thenBased on learnings: “Classes extending built-in constructors must be compiled natively via
FBlueprintSuperValues… andTGocciaSuperCallHelper.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Runtime.Operations.pas` around lines 3295 - 3296, The condition currently gates native construction using the leaf blueprint (Bp) which allows derived blueprints to take the native path even when their root has a wrapped super mapping; change the check to determine the root-most blueprint first (e.g., walk SuperBlueprint pointers until no further SuperBlueprint exists or call a GetRootBlueprint helper) and then use FBlueprintSuperValues.ContainsKey(rootBlueprint) (and keep the TGocciaSuperCallHelper semantics for classes extending built-ins) so that the decision to use native construction is based on the root blueprint mapping rather than the leaf Bp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 3408-3410: The branch that calls SyncArraysBack(Self,
Context.Scope) then clears FArrayBridgeCache must also reset the corresponding
dirty flag: after FArrayBridgeCache.Clear add logic to set FArrayBridgeDirty to
False (or 0, matching how the sibling branch resets it) so the state is
consistent; update the code near SyncArraysBack, Context.Scope,
FArrayBridgeCache.Clear to mirror the sibling branch's handling of
FArrayBridgeDirty.
---
Duplicate comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 3381-3385: FBlueprintBridgeCache is being reused for entries
created by two different conversion paths (ConvertBlueprintToClassValue vs
BlueprintToClassValue called from UnwrapToGocciaValue), which can cause
incorrect constructor behavior; change the caching so entries are distinguished
by converter or conversion path — e.g., introduce a separate cache (like
FBlueprintConvertBridgeCache) or include the converter id/type in the cache key,
and update the lookup/insert in the code around ConvertBlueprintToClassValue and
UnwrapToGocciaValue to use the appropriate cache (or key) so
ConvertBlueprintToClassValue never reuses entries created by
BlueprintToClassValue.
- Around line 3295-3296: The condition currently gates native construction using
the leaf blueprint (Bp) which allows derived blueprints to take the native path
even when their root has a wrapped super mapping; change the check to determine
the root-most blueprint first (e.g., walk SuperBlueprint pointers until no
further SuperBlueprint exists or call a GetRootBlueprint helper) and then use
FBlueprintSuperValues.ContainsKey(rootBlueprint) (and keep the
TGocciaSuperCallHelper semantics for classes extending built-ins) so that the
decision to use native construction is based on the root blueprint mapping
rather than the leaf Bp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c42a987-72bf-4421-ad90-414a7dafd5b4
📒 Files selected for processing (1)
units/Goccia.Runtime.Operations.pas
- Walk blueprint chain to root before checking FBlueprintSuperValues, so multi-level chains (B extends A extends Array) correctly fall through to the bridge instead of taking the native fast path. - Add missing FArrayBridgeDirty := False after FArrayBridgeCache.Clear in the Construct bridge path, matching every other call site. Made-with: Cursor
Acknowledged. Bytecode benchmarks show avg +1.3% improvement (16 improved, 5 regressed). Interpreted side shows avg -1.6% which is within noise. The typed-array and class construction paths show the expected gains from bypassing the bridge. |
Acknowledged. All 3497 tests pass in both modes. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
units/Goccia.Runtime.Operations.pas (1)
3383-3387:⚠️ Potential issue | 🔴 CriticalDon't share
FBlueprintBridgeCachebetweenBlueprintToClassValueandConvertBlueprintToClassValue.At Line 3383 this can reuse a
BlueprintToClassValueentry inserted byUnwrapToGocciaValue, but this branch needs the richerConvertBlueprintToClassValuebridge. If a wrapped-super blueprint is unwrapped beforenew,InstantiateClassat Line 3407 can silently lose accessors, static members, private members, and__fields__initializers.🔧 Minimal safe fix
- if not FBlueprintBridgeCache.TryGetValue(Bp, CachedBridge) then - begin - CachedBridge := ConvertBlueprintToClassValue(Bp, Self); - FBlueprintBridgeCache.Add(Bp, CachedBridge); - end; + CachedBridge := ConvertBlueprintToClassValue(Bp, Self);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Runtime.Operations.pas` around lines 3383 - 3387, The bug is that FBlueprintBridgeCache can return a lightweight BlueprintToClassValue created by UnwrapToGocciaValue which lacks accessors/static/private members and __fields__ initializers required by ConvertBlueprintToClassValue and InstantiateClass; to fix it, change the lookup/insert logic around FBlueprintBridgeCache in the block with ConvertBlueprintToClassValue so that you either use a separate cache for converted bridges or detect the existing entry's kind (e.g., test if CachedBridge is a BlueprintToClassValue vs a full ConvertBlueprintToClassValue result) and if it's the lightweight form, call ConvertBlueprintToClassValue to produce the richer bridge and replace the cache entry before returning; update the code that uses FBlueprintBridgeCache (the retrieval before calling ConvertBlueprintToClassValue and the add after conversion) so InstantiateClass always receives the full converted bridge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 3383-3387: The bug is that FBlueprintBridgeCache can return a
lightweight BlueprintToClassValue created by UnwrapToGocciaValue which lacks
accessors/static/private members and __fields__ initializers required by
ConvertBlueprintToClassValue and InstantiateClass; to fix it, change the
lookup/insert logic around FBlueprintBridgeCache in the block with
ConvertBlueprintToClassValue so that you either use a separate cache for
converted bridges or detect the existing entry's kind (e.g., test if
CachedBridge is a BlueprintToClassValue vs a full ConvertBlueprintToClassValue
result) and if it's the lightweight form, call ConvertBlueprintToClassValue to
produce the richer bridge and replace the cache entry before returning; update
the code that uses FBlueprintBridgeCache (the retrieval before calling
ConvertBlueprintToClassValue and the add after conversion) so InstantiateClass
always receives the full converted bridge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e7355fe-c649-45bf-95f6-6767e8863f98
📒 Files selected for processing (1)
units/Goccia.Runtime.Operations.pas
Summary
PushVMFramesToGocciaso standalone classes and blueprint-inherited classes never cross the interpreter bridge, eliminating O(VM_depth) frame iteration and thetry..exceptwrapper per constructionSetLength(VMArgs, 1)outside the field initializer loop to eliminate redundant dynamic array allocationsTest plan
Made with Cursor
Summary by CodeRabbit