Eliminate interpreter bridge for async/await in bytecode mode#110
Eliminate interpreter bridge for async/await in bytecode mode#110
Conversation
Replace AwaitValue() bridge call to Goccia.Evaluator.AwaitValue() with a native implementation in the runtime layer that handles Promises, thenables, and microtask queue draining directly — no interpreter crossing needed. The native implementation handles: - Primitives and heap strings: return as-is (no bridge) - TGocciaPromiseValue: check state, drain microtask queue if pending, return result or rethrow rejection - Thenable objects: create Promise, call .then(resolve, reject) natively, drain queue, return settled result - Non-thenable objects: return as-is This keeps the implementation in the runtime layer (not the VM dispatch loop) to avoid the icache pressure that caused PR #88's regression. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
…Value)
Plain objects like { then(resolve) { resolve(42) } } are TSouffleRecord
in bytecode mode, not TGocciaWrappedValue. The native AwaitValue only
checked for thenables inside TGocciaWrappedValue, missing this case.
Add TSouffleRecord thenable detection: look up 'then' property via
GetProperty, check if it's a callable (TSouffleClosure, NativeFunction,
or BridgedFunction), and invoke it with resolve/reject callbacks.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Suite Timing
Measured on ubuntu-latest x64. |
Benchmark Results254 benchmarks Interpreted: 🟢 1 improved · 🔴 27 regressed · 226 unchanged · avg -2.4% arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +1.5% · Bytecode: 🟢 1, 13 unch. · avg +2.4%
arrays.js — Interp: 🔴 6, 13 unch. · avg -4.0% · Bytecode: 19 unch. · avg +0.9%
async-await.js — Interp: 6 unch. · avg -2.4% · Bytecode: 6 unch. · avg -1.1%
classes.js — Interp: 31 unch. · avg -1.5% · Bytecode: 31 unch. · avg +2.3%
closures.js — Interp: 11 unch. · avg -0.9% · Bytecode: 11 unch. · avg +3.2%
collections.js — Interp: 🔴 2, 10 unch. · avg -3.2% · Bytecode: 🟢 1, 11 unch. · avg +1.2%
destructuring.js — Interp: 🔴 9, 13 unch. · avg -5.1% · Bytecode: 🟢 2, 20 unch. · avg +2.7%
fibonacci.js — Interp: 8 unch. · avg -2.5% · Bytecode: 8 unch. · avg +2.2%
for-of.js — Interp: 7 unch. · avg -1.3% · Bytecode: 🟢 1, 6 unch. · avg +2.7%
iterators.js — Interp: 🔴 6, 14 unch. · avg -5.6% · Bytecode: 🟢 3, 17 unch. · avg +3.3%
json.js — Interp: 20 unch. · avg -2.6% · Bytecode: 🟢 4, 16 unch. · avg +3.2%
jsx.jsx — Interp: 21 unch. · avg -1.9% · Bytecode: 🟢 1, 20 unch. · avg +2.1%
numbers.js — Interp: 🔴 1, 10 unch. · avg -0.4% · Bytecode: 🟢 2, 9 unch. · avg +3.3%
objects.js — Interp: 🔴 1, 6 unch. · avg -4.0% · Bytecode: 7 unch. · avg +2.5%
promises.js — Interp: 12 unch. · avg -1.4% · Bytecode: 🟢 3, 9 unch. · avg +5.1%
strings.js — Interp: 🔴 1, 10 unch. · avg -1.7% · Bytecode: 🟢 2, 9 unch. · avg +3.6%
typed-arrays.js — Interp: 🔴 1, 21 unch. · avg -1.0% · Bytecode: 🟢 1, 21 unch. · avg +1.3%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 4181-4183: Replace the raw
ESouffleThrow.Create(SouffleString('await: Promise did not settle after
microtask drain')) throw with a call into Goccia.Values.ErrorHelper that
produces/raises a JavaScript TypeError (so the thrown value is a
TGocciaThrowValue wrapping a proper JS Error object); locate the current throw
site (the ESouffleThrow.Create + SouffleString usage) and call the appropriate
ErrorHelper TypeError helper (the project's ErrorHelper function that
creates/raises TypeError) with the same message so JS code sees an actual
TypeError instance.
- Around line 4126-4130: ThenVal is being checked against raw AsReference types
but non-native functions may be wrapped in TGocciaWrappedValue, so first unwrap
record ThenVal (e.g., if ThenVal is a TGocciaWrappedValue extract its inner
value) before testing callability; replace hardcoded 'then' string with the
PROP_THEN constant from Goccia.Constants.PropertyNames when calling GetProperty
and when doing the wrapped-object lookup (the earlier lookup at the other
location should also use PROP_THEN), and apply the same unwrap-and-test change
to the similar checks around the 4139-4141 block so
TSouffleClosure/TSouffleNativeFunction/TGocciaBridgedFunction checks operate on
the unwrapped value.
- Around line 4102-4106: The newly created TGocciaPromiseValue must be
temp-rooted immediately after each TGocciaPromiseValue.Create to prevent GC from
collecting it while constructing callbacks or calling Then; update the logic
around the TGocciaPromiseValue.Create sites that build
TGocciaNativeFunctionValue.Create(...) (the Promise/Then construction) to call
AddTempRoot(Promise) right after Create and only call RemoveTempRoot(Promise)
later if this routine actually added the root, and when triggering collection
use CollectIfNeeded(AProtect) so stack-held values are protected per guidelines;
apply the same fix for both creation blocks (the Promise at 4102 and the one at
4132) and ensure any subsequent calls that may allocate (UnwrapToGocciaValue,
TGocciaNativeFunctionValue.Create, and the user then invocation) occur while the
promise is rooted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd9e0176-7375-4b9d-b246-b450cab73de9
📒 Files selected for processing (1)
units/Goccia.Runtime.Operations.pas
- Root synthetic Promise via AddTempRoot immediately after Create, before constructing callbacks or invoking then (prevents GC collection during allocations in that window) - Unwrap record then property before testing callability to handle mixed-mode thenables wrapped in TGocciaWrappedValue - Use PROP_THEN constant instead of hardcoded 'then' string - Use ThrowTypeErrorMessage instead of raw ESouffleThrow for unsettled promise error (produces proper JS TypeError catchable from try/catch) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
AwaitValue()bridge call toGoccia.Evaluator.AwaitValue()with a native implementation in the runtime layerThe native implementation handles:
TGocciaPromiseValue: check state directly; if pending, drain microtask queue and re-check; return result or rethrow rejection.then): create Promise, call.then(resolve, reject)natively, drain queue, return settled resultTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit