[defer-import-eval] Consider the whole SCC when checking sync readiness [v8/v8 : main]
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
It's important to highlight that the change I've made on `SourceTextModule::GatherAsynchronousTransitiveDependencies` by removing `module->status() == kEvaluatingAsync` makes tests `cyclic-tla-fn-defer-eval.tentative.html` and `cyclic-tla-defer-eval.tentative.html` timeout due to an infinity loop. Right now, those tests are not following current spec (see [1] for details). The order for landing here should be to first land [2] that fixes the test, and then this CL, that will make implementation align with spec again.
[1] - https://github.com/tc39/proposal-defer-import-eval/issues/83
[2] - https://chromium-review.googlesource.com/c/chromium/src/+/8037401
Related details
Code-Owners
Code-Review
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")
+Olivier and +Marja.
Sorry that this tangled a bit 2 issues, but without fixing https://issues.chromium.org/issues/530457806, I was not going to be able to properly add the test case that was reported on https://issues.chromium.org/issues/525234471.
Related details
- Marja Hölttä
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä added 3 comments![]( "Open in Gerrit")
olivf@ will hopefully review from modules POV, just minor comments here:
Bug: 525234471, 530457806You can use Fixed: 1234 if this CL fixes those bugs. You can also have both a Bug: line and a Fixed: line.
static bool IsModuleSCCEvaluated(Handle<SourceTextModule> module);This could use a comment, and the comment should spell out what SCC is.
Related details
- Caio Lima
- 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")
if (!IsTheHole(module->cycle_root())) {when is the cycle_root the hole? and can we write a testcase for that?
Related details
- Caio Lima
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")
if (!IsTheHole(module->cycle_root())) {when is the cycle_root the hole? and can we write a testcase for that?
The `cycle_root` starts as `the hole` until it finds a cycle or it finishes its evaluation on non-cyclic cases. It means that any test case using `import defer` will first make this `!IsTheHole(module->cycle_root())` return false, given that we call `GatherAsynchronousTransitiveDependencies` while `cycle_root` is still empty (i.e `TheHole`). The test case added on this CL stress the path where `!IsTheHole(module->cycle_root())` is true, because we first call `import("A")` before executing `GatherAsynchronousTransitiveDependencies`, but other tests with `import defer` already cover the `false` branch scenario. Does it make sense?
Related details
- 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. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
static bool IsModuleSCCEvaluated(Handle<SourceTextModule> module);This could use a comment, and the comment should spell out what SCC is.
Added and thanks for this comment. One observation is that this link will only work after we land https://github.com/tc39/proposal-defer-import-eval/pull/85.
Related details
- Marja Hölttä
- 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. |
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
You can use Fixed: 1234 if this CL fixes those bugs. You can also have both a Bug: line and a Fixed: line.
Done
Related details
- Marja Hölttä
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Olivier Flückiger
| Code-Review | +1 |
2 comments
if (!IsTheHole(module->cycle_root())) {Caio Limawhen is the cycle_root the hole? and can we write a testcase for that?
The `cycle_root` starts as `the hole` until it finds a cycle or it finishes its evaluation on non-cyclic cases. It means that any test case using `import defer` will first make this `!IsTheHole(module->cycle_root())` return false, given that we call `GatherAsynchronousTransitiveDependencies` while `cycle_root` is still empty (i.e `TheHole`). The test case added on this CL stress the path where `!IsTheHole(module->cycle_root())` is true, because we first call `import("A")` before executing `GatherAsynchronousTransitiveDependencies`, but other tests with `import defer` already cover the `false` branch scenario. Does it make sense?
- Caio Lima
- Marja Hölttä
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")
I also changed the test to guarantee the order of evaluation, where we first evaluate A.js, and we then evaluate Middle.js while A.mjs is still blocked.
if (!IsTheHole(module->cycle_root())) {Caio Limawhen is the cycle_root the hole? and can we write a testcase for that?
Olivier FlückigerThe `cycle_root` starts as `the hole` until it finds a cycle or it finishes its evaluation on non-cyclic cases. It means that any test case using `import defer` will first make this `!IsTheHole(module->cycle_root())` return false, given that we call `GatherAsynchronousTransitiveDependencies` while `cycle_root` is still empty (i.e `TheHole`). The test case added on this CL stress the path where `!IsTheHole(module->cycle_root())` is true, because we first call `import("A")` before executing `GatherAsynchronousTransitiveDependencies`, but other tests with `import defer` already cover the `false` branch scenario. Does it make sense?
yes, makes sense. Can you add a comment please?
- Marja Hölttä
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä added 1 comment![]( "Open in Gerrit")
this somehow lost the LGTMs. is this good to land or is there something that has to be fixed?
Related details
- Caio Lima
- Olivier Flückiger
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger voted and added 1 comment![]( "Open in Gerrit")
Votes added by Olivier Flückiger
| Code-Review | +1 |
1 comment
lgtm, thank you
Related details
- Caio Lima
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Caio Lima
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Caio Lima
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[defer-import-eval] Consider the whole SCC when checking sync readiness
Touching a deferred namespace forces synchronous evaluation of the
deferred module. To decide whether that is allowed,
`ReadyForSyncExecution` and `GatherAsynchronousTransitiveDependencies`
only inspected the status of each individual module. However, in a
cycle containing top-level await, a module can already be kEvaluated
while its cycle root is still kEvaluatingAsync. Both functions would
then wrongly treat the dependency graph as settled: the deferred
module was evaluated synchronously while its dependency's cycle was
still waiting to finish, producing a pending (instead of fulfilled)
evaluation promise and failing a check in
JSDeferredModuleNamespace::EvaluateModuleSync.
In this change we introduce `IsModuleSCCEvaluated`, which considers a
module settled only when its cycle root reached kEvaluated (or
kErrored), and use it in `ReadyForSyncExecution` and
`GatherAsynchronousTransitiveDependencies`. Synchronous evaluation of
the deferred module now only proceeds once the whole strongly-connected
component has settled.
This is the engine counterpart of the spec issue
https://github.com/tc39/proposal-defer-import-eval/issues/84
(assertion failure on EvaluateModuleSync), which is still open and
will be discussed in the July 2026 plenary.
The regression test asserts the module evaluation order: the async
cycle {A, B} must fully settle before nsD.z triggers the deferred
evaluation of D.
In addition to that, this change includes the fix for the bug [1] by
removing the early return on `GatherAsynchronousTransitiveDependencies`
when `module.[[Status]]` is `kEvaluatingAsync`.
[1] - https://issues.chromium.org/issues/530457806
- M src/objects/source-text-module.cc
- M src/objects/source-text-module.h
- A test/mjsunit/harmony/modules-import-defer-sync-eval-async-cycle.js
Code-Review: +1 by Olivier Flückiger, +1 by Marja Hölttä
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |