Add Souffle-value-backed Map/Set types and native callback fast paths#109
Add Souffle-value-backed Map/Set types and native callback fast paths#109
Conversation
Introduce TGocciaSouffleMap and TGocciaSouffleSet in the Goccia runtime layer (not Souffle VM core) that store entries as TSouffleValue directly, eliminating per-operation UnwrapToGocciaValue/ToSouffleValue conversion. - Add Goccia.Runtime.Collections.pas with TGocciaSouffleMap and TGocciaSouffleSet backed by TSouffleValue key/value storage - Implement SouffleSameValueZero for key equality (ES SameValueZero semantics at the Souffle value level) - Add fast paths in native Map callbacks (get, set, has, delete, clear, forEach) that check for TGocciaSouffleMap first — zero wrap/unwrap - Add fast paths in native Set callbacks (has, add, delete, clear, forEach) that check for TGocciaSouffleSet first - Add TGocciaSouffleMap/Set handling in GetProperty bridge for .size - Keep TGocciaWrappedValue fallback for interpreter-mode Map/Set instances and operations not yet ported (keys, values, entries, iterators, Set operations like union/intersection) Construction path integration (creating TGocciaSouffleMap/Set instead of TGocciaWrappedValue<TGocciaMapValue>) is deferred to a follow-up — the callback fast paths will activate automatically once instances are created with the new types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded a new unit providing in-memory Souffle-backed map/set types and iterators, plus runtime changes to register iterator delegates, convert Goccia collections to Souffle-native objects, recognize native map/set types for property/iterator access, and provide native fast-paths for map/set methods and iterator next. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
Benchmark Results263 benchmarks Interpreted: 🟢 3 improved · 🔴 95 regressed · 165 unchanged · avg -5.0% arraybuffer.js — Interp: 🔴 12, 2 unch. · avg -11.9% · Bytecode: 14 unch. · avg -0.7%
arrays.js — Interp: 🔴 19 · avg -10.9% · Bytecode: 🟢 1, 18 unch. · avg +3.9%
async-await.js — Interp: 🔴 6 · avg -10.2% · Bytecode: 6 unch. · avg +0.1%
classes.js — Interp: 🔴 30, 1 unch. · avg -10.7% · Bytecode: 🟢 1, 30 unch. · avg +1.2%
closures.js — Interp: 🔴 6, 5 unch. · avg -7.0% · Bytecode: 11 unch. · avg +2.0%
collections.js — Interp: 🔴 1, 11 unch. · avg -4.4% · Bytecode: 12 unch. · avg +1.2%
destructuring.js — Interp: 🔴 14, 8 unch. · avg -8.0% · Bytecode: 🟢 1, 21 unch. · avg +0.7%
fibonacci.js — Interp: 🔴 1, 7 unch. · avg -4.4% · Bytecode: 🟢 1, 🔴 1, 6 unch. · avg +0.9%
for-of.js — Interp: 7 unch. · avg -0.9% · Bytecode: 🟢 1, 6 unch. · avg +0.8%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 20 unch. · avg -1.1% · Bytecode: 🟢 7, 13 unch. · avg +8.3%
json.js — Interp: 🔴 1, 19 unch. · avg -3.2% · Bytecode: 🟢 6, 🔴 2, 12 unch. · avg +3.1%
jsx.jsx — Interp: 21 unch. · avg -3.3% · Bytecode: 🟢 13, 8 unch. · avg +7.7%
modules.js — Interp: 9 unch. · avg -4.1% · Bytecode: 9 unch. · avg -1.6%
numbers.js — Interp: 11 unch. · avg -4.2% · Bytecode: 🟢 2, 9 unch. · avg +1.7%
objects.js — Interp: 7 unch. · avg +0.1% · Bytecode: 🟢 3, 4 unch. · avg +5.5%
promises.js — Interp: 🟢 2, 10 unch. · avg +3.1% · Bytecode: 🟢 7, 5 unch. · avg +11.4%
strings.js — Interp: 🟢 1, 10 unch. · avg +2.7% · Bytecode: 🟢 1, 10 unch. · avg +3.5%
typed-arrays.js — Interp: 🔴 5, 17 unch. · avg -1.5% · Bytecode: 🟢 1, 21 unch. · avg +1.2%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Suite Timing
Measured on ubuntu-latest x64. |
…ratorNext fast paths - Add TGocciaSouffleMapIterator and TGocciaSouffleSetIterator to Goccia.Runtime.Collections with proper GC marking - Add NativeMapIteratorNext/NativeSetIteratorNext callbacks with MAP_ITERATOR_METHODS/SET_ITERATOR_METHODS delegate tables - Add FMapIteratorDelegate/FSetIteratorDelegate fields with GC marking - Wire GetProperty to resolve .next() on native iterators - Wire GetIterator to recognize native Map/Set types and return native iterators (entries for Map, values for Set) - Wire GetIterator to recognize native iterators as self-iterating (Symbol.iterator returns self) - Wire IteratorNext fast paths for native Map/Set iterators - Add native fast paths for keys/values/entries methods returning native iterators instead of bridging to InvokeGocciaMethod Note: ToSouffleValue still wraps Map/Set as TGocciaWrappedValue since the native types don't yet support all operations (getOrInsert, Set operations, subclassing). The callback fast paths check both native and wrapped types so they're ready for future activation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Runtime.Operations.pas (1)
5894-5983:⚠️ Potential issue | 🟠 MajorPass undefined to native Map/Set methods when arguments are omitted.
The native fast paths for
Map.prototype.get,Map.prototype.has,Map.prototype.deleteandSet.prototype.has,Set.prototype.delete,Set.prototype.addcurrently exit early whenAArgCount < 1, but ECMAScript passesundefinedas the key/value. This causes interpreter and native code paths to diverge:m.set(undefined, 1); m.get()will find the entry in interpreter mode but not with a native Map receiver.Treat missing arguments as
undefinedby always passing them through to the underlyingGetEntry,HasKey,DeleteEntry,Contains,Delete, andAddmethods, matching interpreter-mode behavior.🤖 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 5894 - 5983, Native fast-paths (NativeMapGet, NativeMapSet, NativeMapHas, NativeMapDelete) currently early-exit when AArgCount < 1, but ECMAScript semantics treat missing args as undefined; update the SM (GetSouffleMap) branches to synthesize a TSouffleValue = SouffleNilWithFlags(GOCCIA_NIL_UNDEFINED) when arguments are missing and pass that value to SM.GetEntry, SM.HasKey, SM.DeleteEntry (and to SM.SetEntry for missing key/value), and for NativeMapSet ensure zero-arg and one-arg cases call SM.SetEntry with undefined for the missing key/value and still return AReceiver; keep using the existing AArgs^ when present and reference the named symbols GetEntry, HasKey, DeleteEntry and SetEntry to locate changes.
🤖 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 6008-6020: When AReceiver is a native Souffle collection (detected
by GetSouffleMap/GetSouffleSet and SouffleIsReference) do not call
InvokeGocciaMethod; instead validate that the first argument is a callable
function (throw a proper TypeError if missing or not callable), capture the
optional thisArg (second parameter) and pass it into the per-entry invocation
(preserving context) when calling GNativeArrayJoinRuntime.Invoke (or
equivalent), and only call InvokeGocciaMethod for non-native/wrapped instances;
update the Map.prototype.forEach/Set.prototype.forEach branch to perform these
checks and use the native iteration path instead of routing to
InvokeGocciaMethod.
- Around line 6086-6091: Iterator result records created in
NativeMapIteratorNext and NativeSetIteratorNext (where TSouffleRecord.Create is
used to build Rec with 'value' and 'done') are missing the delegate causing
Object.prototype methods to fail in bytecode mode; after creating Rec (and
before allocating/returning it) set Rec.Delegate :=
GNativeArrayJoinRuntime.VM.RecordDelegate and replace literal property names
('value' and 'done') with the appropriate property name constants used elsewhere
in the codebase so the put calls use those constants instead of string literals.
- Around line 2774-2805: GetProperty contains branches for
TGocciaSouffleMap/Set/MapIterator/SetIterator but other bridge methods
(UnwrapToGocciaValue, HasProperty, SetProperty, DeleteProperty,
GetSymbolOnNativeObject) and ToSouffleValue still treat
TGocciaMapValue/TGocciaSetValue as TGocciaWrappedValue, so native heap types are
never returned; either extend those methods to recognize and handle
TGocciaSouffleMap and TGocciaSouffleSet (and the iterator types) the same way
GetProperty does, and call ConvertGocciaMapToSouffle/ConvertGocciaSetToSouffle
from ToSouffleValue when encountering TGocciaMapValue/TGocciaSetValue, or add a
clear TODO comment referencing the tracking issue and ensure all places
consistently wrap rather than return native heap types; update
UnwrapToGocciaValue, HasProperty, SetProperty, DeleteProperty,
GetSymbolOnNativeObject and ToSouffleValue to use the same detection logic as
GetProperty (checking SouffleIsReference and AsReference is
TGocciaSouffleMap/Set/MapIterator/SetIterator) so native collections are fully
supported.
---
Outside diff comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 5894-5983: Native fast-paths (NativeMapGet, NativeMapSet,
NativeMapHas, NativeMapDelete) currently early-exit when AArgCount < 1, but
ECMAScript semantics treat missing args as undefined; update the SM
(GetSouffleMap) branches to synthesize a TSouffleValue =
SouffleNilWithFlags(GOCCIA_NIL_UNDEFINED) when arguments are missing and pass
that value to SM.GetEntry, SM.HasKey, SM.DeleteEntry (and to SM.SetEntry for
missing key/value), and for NativeMapSet ensure zero-arg and one-arg cases call
SM.SetEntry with undefined for the missing key/value and still return AReceiver;
keep using the existing AArgs^ when present and reference the named symbols
GetEntry, HasKey, DeleteEntry and SetEntry to locate changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc561677-bf34-4a8f-a4a2-61cc792c0154
📒 Files selected for processing (2)
units/Goccia.Runtime.Collections.pasunits/Goccia.Runtime.Operations.pas
Resolve uses clause conflict: keep both Goccia.Parser (from main) and Goccia.Runtime.Collections (from this branch). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set Rec.Delegate := VM.RecordDelegate on iterator result {value, done}
records so Object.prototype methods work in bytecode mode. Use
PROP_VALUE/PROP_DONE constants instead of string literals.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Runtime.Operations.pas (1)
6153-6168:⚠️ Potential issue | 🟡 MinorTreat omitted
Maparguments asundefined.These guards turn
map.get(),map.has(), andmap.set()into early returns/no-ops, but the collection APIs are supposed to operate on anundefinedkey/value when an argument is omitted. As written, anundefinedkey cannot be observed or created through these paths. Normalize missing arguments toGOCCIA_NIL_UNDEFINEDhere, and mirror the same fix in the wrapped branch immediately below so both receivers stay aligned.Also applies to: 6178-6201, 6211-6221
🤖 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 6153 - 6168, The code currently treats omitted map arguments as early exits, preventing observing/creating undefined keys; change the argument handling in both the SouffleMap branch (when SM := GetSouffleMap(AReceiver) is Assigned) and the unwrapped branch (M := UnwrapMapFromReceiver(AReceiver)) so that when AArgCount < 1 you do not return but instead synthesize a key/value of GOCCIA_NIL_UNDEFINED (use SouffleNilWithFlags(GOCCIA_NIL_UNDEFINED) or the equivalent Goccia value used by GNativeArrayJoinRuntime.UnwrapToGocciaValue) before calling SM.GetEntry / M.FindEntry and accessing M.Entries; apply the same normalization to the corresponding code paths for map.get, map.has, and map.set (also in the nearby blocks at the indicated ranges) so both SM and M branches behave identically with omitted args.
🧹 Nitpick comments (2)
units/Goccia.Runtime.Operations.pas (2)
7833-7839: UsePROP_NEXTin the iterator delegate tables.The new iterator tables reintroduce a raw property name. Switching these entries to
PROP_NEXTkeeps the property key centralized with the rest of the bridge.As per coding guidelines "Use constant names from split 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, andGoccia.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` around lines 7833 - 7839, Replace the hardcoded 'next' property name in the iterator method tables with the centralized constant PROP_NEXT: update MAP_ITERATOR_METHODS and SET_ITERATOR_METHODS entries so their Name uses PROP_NEXT (leaving Arity and Callback as-is pointing to NativeMapIteratorNext and NativeSetIteratorNext) to conform with Goccia.Constants.PropertyNames usage.
6145-6487: Add ES spec headers to the new collection callbacks.This whole native Map/Set/iterator block is spec-facing behavior, but it lands without the
// ES2026 §...annotations used elsewhere in the unit. Adding those headers will make later semantic changes much easier to audit.As per coding guidelines "Annotate ECMAScript-specified behavior with spec references using format
// ESYYYY §X.Y.Z SpecMethodName(specParams)immediately above the function body, with inline annotations for multi-step algorithms".🤖 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 6145 - 6487, The new native Map/Set/iterator functions lack ECMAScript spec headers; add ES spec comment lines immediately above each affected function (e.g. NativeMapGet, NativeMapSet, NativeMapHas, NativeMapDelete, NativeMapClear, NativeMapForEach, NativeMapKeys, NativeMapValues, NativeMapEntries, NativeMapIteratorNext, NativeSetIteratorNext, NativeSetHas, NativeSetAdd, NativeSetDelete, NativeSetClear, NativeSetForEach, NativeSetValues, NativeSetEntries) using the project convention "// ES2026 §X.Y.Z SpecMethodName(specParams)" (and inline-step annotations where the algorithm maps to multi-step spec text) so each function is annotated with the corresponding ECMAScript clause just above the function declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 6153-6168: The code currently treats omitted map arguments as
early exits, preventing observing/creating undefined keys; change the argument
handling in both the SouffleMap branch (when SM := GetSouffleMap(AReceiver) is
Assigned) and the unwrapped branch (M := UnwrapMapFromReceiver(AReceiver)) so
that when AArgCount < 1 you do not return but instead synthesize a key/value of
GOCCIA_NIL_UNDEFINED (use SouffleNilWithFlags(GOCCIA_NIL_UNDEFINED) or the
equivalent Goccia value used by GNativeArrayJoinRuntime.UnwrapToGocciaValue)
before calling SM.GetEntry / M.FindEntry and accessing M.Entries; apply the same
normalization to the corresponding code paths for map.get, map.has, and map.set
(also in the nearby blocks at the indicated ranges) so both SM and M branches
behave identically with omitted args.
---
Nitpick comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 7833-7839: Replace the hardcoded 'next' property name in the
iterator method tables with the centralized constant PROP_NEXT: update
MAP_ITERATOR_METHODS and SET_ITERATOR_METHODS entries so their Name uses
PROP_NEXT (leaving Arity and Callback as-is pointing to NativeMapIteratorNext
and NativeSetIteratorNext) to conform with Goccia.Constants.PropertyNames usage.
- Around line 6145-6487: The new native Map/Set/iterator functions lack
ECMAScript spec headers; add ES spec comment lines immediately above each
affected function (e.g. NativeMapGet, NativeMapSet, NativeMapHas,
NativeMapDelete, NativeMapClear, NativeMapForEach, NativeMapKeys,
NativeMapValues, NativeMapEntries, NativeMapIteratorNext, NativeSetIteratorNext,
NativeSetHas, NativeSetAdd, NativeSetDelete, NativeSetClear, NativeSetForEach,
NativeSetValues, NativeSetEntries) using the project convention "// ES2026
§X.Y.Z SpecMethodName(specParams)" (and inline-step annotations where the
algorithm maps to multi-step spec text) so each function is annotated with the
corresponding ECMAScript clause just above the function declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebf79230-eb56-4986-bb3c-0f8de2f7fd93
📒 Files selected for processing (1)
units/Goccia.Runtime.Operations.pas
Summary
TGocciaSouffleMapandTGocciaSouffleSetSouffle-value-backed types inGoccia.Runtime.Collections.pas— storesTSouffleValuekey/value pairs directly, eliminating per-operation wrap/unwrap in native callbacksTGocciaSouffleMapIteratorandTGocciaSouffleSetIteratorwith properNext(out ADone)protocol and GC markingMAP_ITERATOR_METHODS/SET_ITERATOR_METHODS) withnextmethod returning{value, done}recordsGetPropertyfast paths for iterator delegatesGetIteratorto recognize native Map/Set types and native iterators as self-iteratingIteratorNextfast paths for native iteratorskeys(),values(),entries()methods returning native iteratorsCurrent state
The native types and callbacks are defined and tested. Callbacks check both native (
TGocciaSouffleMap) and wrapped (TGocciaWrappedValue<TGocciaMapValue>) types, falling back to bridge for whichever is present.ToSouffleValuestill wraps asTGocciaWrappedValuesince native types don't yet support all operations (getOrInsert, Set operations, subclassing, cloning). Future work: interceptConstruct()to produce native types directly for non-subclassed Map/Set.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit