[defer-import-eval] dynamic import with deferred evaluation support [v8/v8 : main]
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
This still is a WIP, but I'm submitting the patch so our conversation about how to implement `PerformPromiseAll` on C++ is more concrete.
transitioning macro PerformPromiseAllFixedArray(This is not being used by current Patch, but it's here as a reference so we can compare the C++ version. It's mostly a copy and paste from `PerformPromiseAll` above, module the machinery to deal with iterator, instead of a FixedArray.
MaybeHandle<JSPromise> JSPromise::PerformPromiseAll(This is the version of builtin function in C++, instead of torque. it might have some machinery missing or wrong, but it's a working version now.
Related details
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
V8_WARN_UNUSED_RESULT MaybeLocal<Value> DeferredEvaluate(I'm tinking in actually rename it to `EvaluateAsyncDependencies`. It is probably a much better name. WDYT?
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 6 comments![]( "Open in Gerrit")
V8_WARN_UNUSED_RESULT MaybeLocal<Value> DeferredEvaluate(I'm tinking in actually rename it to `EvaluateAsyncDependencies`. It is probably a much better name. WDYT?
Or `EvaluateForImportDefer`?
* dependencies are not going to be evaluated.Implements 13.3.10.4.1 ContinueDynamicImport, Step 6.e.
has_error = true;nit: return here, instead of break.
CHECK(eval_result->IsPromise());SyntheticModules don't seem to return promises. Am I missing something?
static MaybeHandle<JSPromise> PerformPromiseAll(Let's add a comment that this is limited to the case
`PerformPromiseAll([native-promise, ...], %Promise%, NewPromiseCapability(%Promise%), %Promise.resolve%)`
MaybeHandle<JSPromise> JSPromise::PerformPromiseAll(This is the version of builtin function in C++, instead of torque. it might have some machinery missing or wrong, but it's a working version now.
I had good success recently letting gemini help with translations from torque to C++. Here is what it had to say:
✦ Under the assumption that we are using the native %Promise% constructor, native %Promise.resolve%, and a list of native Promises (the "Fast Path"), the C++ implementation is largely behaviorally equivalent to the Torque implementation. It correctly avoids allocating throwaway promises (by passing undefined to PerformPromiseThen) and correctly suppresses debug events for the internal capability.
However, there are two remaining observable discrepancies:
1. Missing Debugger Support (Catch Prediction)
The Torque implementation explicitly marks the reject handler of the capability as a "forwarding" handler when the debugger is active. This allows the V8 debugger to correctly predict that a rejection flowing through Promise.all will be caught later, preventing false positive "Uncaught Exception" pauses.
* Torque (`src/builtins/promise-all.tq`):
1 // For catch prediction, don't treat the .then calls as handling it;
2 // instead, recurse outwards.
3 if (IsDebugActive()) deferred {
4 SetPropertyStrict(context, reject, kPromiseForwardingHandlerSymbol, True);
5 }
* C++ (`src/objects/objects.cc`):
This check and property assignment are completely missing.
Consequence: When debugging, rejections occurring within the Promise.all chain might be incorrectly flagged as "uncaught" by the debugger, even if the result of Promise.all has a .catch() attached.
2. Missing Range Check (Too Many Elements)
The Torque implementation enforces a hard limit on the number of elements, corresponding to the maximum size of the hash field where the index is stored (kPropertyArrayHashFieldMax, approximately 2 million).
* Torque (`src/builtins/promise-all.tq`):
1 // Check if we reached the limit.
2 if (index == kPropertyArrayHashFieldMax) {
3 ThrowRangeError(MessageTemplate::kTooManyElementsInPromiseCombinator, 'all');
4 }
* C++ (`src/objects/objects.cc`):
The C++ implementation iterates over the std::vector and casts the size to int. It does not check against kPropertyArrayHashFieldMax. It passes the raw index to factory->CreatePromiseAllResolveElementFunction.
Consequence: If asyncDepsEvaluationPromises contains more than ~2.1 million elements ($2^{21}$), the Torque version correctly throws a RangeError. The C++ version will proceed, likely corrupting the properties_or_hash field of the created resolve functions (due to bitfield overflow), leading to undefined behavior or a crash when those functions are eventually executed.Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 2 comments![]( "Open in Gerrit")
resolve_element_context->SetNoCell(is this intermediate count observable somehow or could we just set it to "length" outside the loop and be done with it?
if (final_remaining == 0) {I think this cannot happen due to line 4949
Caio Lima (Gerrit)
Caio Lima added 5 comments![]( "Open in Gerrit")
Thank you very much for the review! I'll fix them ASAP.
CHECK(eval_result->IsPromise());SyntheticModules don't seem to return promises. Am I missing something?
We can't have `SyntheticModules`, because they are always evaluated synchronously. Here we just evaluate asynchronous dependencies (i.e SourceTextModules with TLA).
Regarding debugging checks, I'm not 100% confident, but I've tested the following on Chrome and I couldn't reach the "Pause on uncaught exception", just when I enable "Pause on caught exception":
```
// a.mjs
throw new Error();
await 0;
// b.mjs
await 42;
// entrypoint.mjs
let a = import("a.mjs");
let b - import("b.mjs");
let [a_ns, b_ns] = Promise.all([a, b]);
```
When I enable "Pause on caught exception", the execution then breaks at `a.mjs:1` `throw`.
My test removed the debugging setup from `promise-all.tq`, and I've found no difference on debugger behavior. Given that, I decided to not include the debugging machinery here. IIUC, exceptions thrown while a module is being evaluated is considered a caught by its Promising wrapping.
resolve_element_context->SetNoCell(is this intermediate count observable somehow or could we just set it to "length" outside the loop and be done with it?
I'll double check it. It came mostly from original torque file, but I was thinking in the condition where `perform_promise_then` is called with an already fulfilled promise. But thinking this through, I think the resolve callback for each Promise is only going to run on next micro task and this logic is here to handle user-defined Promises.
if (final_remaining == 0) {I think this cannot happen due to line 4949
The logic is the same I mentioned above. Probably it's here because of user-defined promises, but this implementation only considers builtin promises. If I confirm that, I'll remove this part.
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 2 comments![]( "Open in Gerrit")
CHECK(eval_result->IsPromise());Caio LimaSyntheticModules don't seem to return promises. Am I missing something?
We can't have `SyntheticModules`, because they are always evaluated synchronously. Here we just evaluate asynchronous dependencies (i.e SourceTextModules with TLA).
Ah right. Could we maybe type the evaluation_list in GatherAsynchronousTransitiveDependencies a list of SourceTextModule's then?
I see. Tbh. I am not familiar with the debugger integration.
What I would like to avoid is that the PerformPromiseAll misbehaves when used in a different context.
So, I am totally fine with it having limitations on how you can use it. But I would like that we understand (and check or restrict by construction) what the limitations are.
With the debugger I am not sure. If I understand the comment correctly the difference might become observable if you add a try/catch block around Promise.all?
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caio Lima (Gerrit)
Caio Lima added 2 comments![]( "Open in Gerrit")
CHECK(eval_result->IsPromise());Caio LimaSyntheticModules don't seem to return promises. Am I missing something?
Olivier FlückigerWe can't have `SyntheticModules`, because they are always evaluated synchronously. Here we just evaluate asynchronous dependencies (i.e SourceTextModules with TLA).
Ah right. Could we maybe type the evaluation_list in GatherAsynchronousTransitiveDependencies a list of SourceTextModule's then?
Makes sense!
If there's a try catch capturing `Promise.all`, and we enable "Pause on caught exception", the behavior is the same as well, because it will pause when `throw` in `a.mjs` happens. I don't fully understand this interaction with the debugger, but I'll get a deeper understanding to properly set the implementation.
Related details
- Leszek Swirski
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
apparently this is the test for that feature: https://source.chromium.org/chromium/chromium/src/+/main:v8/test/debugger/debug/es6/debug-promises/promise-all-uncaught.js
Related details
- Caio Lima
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |