[defer-import-eval] Add Blink integration for dynamic import.defer [chromium/src : main]
Olivier Flückiger (Gerrit)
Olivier Flückiger added 2 comments![]( "Open in Gerrit")
lgtm for what it's worth. adding reviewers, since I am not a chromium reviewer.
export const value = 42;could you add another test that has a TLA so we can make sure the eager evaluation part works?
Related details
- Caio Lima
- Kentaro Hara
- Leszek Swirski
Code-Coverage
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")
Thanks for the review! Looks like I'm still missing a CO to third_party/blink/web_tests/VirtualTestSuites. Do you know who I could include here?
could you add another test that has a TLA so we can make sure the eager evaluation part works?
- Kentaro Hara
- Leszek Swirski
- Olivier Flückiger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
- Caio Lima
- David Baron
- Kentaro Hara
- Olivier Flückiger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
- Caio Lima
- Kentaro Hara
- Olivier Flückiger
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
Talked offline to Kouhei, he should be able to do a review next week.
Related details
- Caio Lima
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 5 comments![]( "Open in Gerrit")
Basically looks good.
v8::ModuleImportPhase import_phase = v8::ModuleImportPhase::kEvaluation);Could you remove the default `kEvaluation` to avoid possible import phase mismatch?
v8::ModuleImportPhase phase = v8::ModuleImportPhase::kEvaluation);ditto, Could you remove the default `kEvaluation` to avoid possible import phase mismatch?
[[nodiscard]] ScriptEvaluationResult EvaluateForImportPhase(nit: I prefer a name like `RunScriptOnScriptStateAndReturnValueWithImportPhase()`
to clarify this is mostly just adding import phase arg to `RunScriptOnScriptStateAndReturnValue()`.
(The name is long, but shouldn't be a large problem as we only have two callers)
DCHECK_EQ(execute_script_policy,Drive-by: could you use `CHECK()` here?
Related details
- Caio Lima
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
- Caio Lima
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
<!doctype html>Can we move the tests to public WPT
(under `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/`)?
We can mark the tests as `tentative`
https://web-platform-tests.org/writing-tests/file-names.html#test-features
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
<!doctype html>Can we move the tests to public WPT
(under `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/`)?We can mark the tests as `tentative`
https://web-platform-tests.org/writing-tests/file-names.html#test-features
Sure. One question that I have is that this test right now fails, because we have the flag disabled when running this test here. Should we keep the expected like that as well? Additionally, show we also move the TLA version?
Related details
- Hiroshige Hayashizaki
Caio Lima (Gerrit)
Caio Lima added 6 comments![]( "Open in Gerrit")
Thank you very much for the reviews so far.
I see that I'm now missing owner to review the flag override between Chromium and V8. Could you help me find somebody that could review this part, please?
v8::ModuleImportPhase import_phase = v8::ModuleImportPhase::kEvaluation);Could you remove the default `kEvaluation` to avoid possible import phase mismatch?
Done
v8::ModuleImportPhase phase = v8::ModuleImportPhase::kEvaluation);ditto, Could you remove the default `kEvaluation` to avoid possible import phase mismatch?
Done
[[nodiscard]] ScriptEvaluationResult EvaluateForImportPhase(nit: I prefer a name like `RunScriptOnScriptStateAndReturnValueWithImportPhase()`
to clarify this is mostly just adding import phase arg to `RunScriptOnScriptStateAndReturnValue()`.(The name is long, but shouldn't be a large problem as we only have two callers)
Acknowledged
Drive-by: could you use `CHECK()` here?
Done
<!doctype html>Caio LimaCan we move the tests to public WPT
(under `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/`)?We can mark the tests as `tentative`
https://web-platform-tests.org/writing-tests/file-names.html#test-features
Sure. One question that I have is that this test right now fails, because we have the flag disabled when running this test here. Should we keep the expected like that as well? Additionally, show we also move the TLA version?
- David Baron
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
- Caio Lima
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
+Kentaro for content/renderer/render_process_impl
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 2 comments![]( "Open in Gerrit")
Would you add WPTs for cyclic imports something like the following? Please correct me if I misunderstand how cyclic dependency works. (cc: @hiro...@chromium.org)
```
// a.js
import defer * as nsB from "./b.js";
window.__moduleWithTlaDepEvaluated = true;
export const value = nsB.tlaValue;
// b.js
window.__tlaModuleEvaluated = true;
await import.defer("./a.js");
export const tlaValue = "tla-done";
// in WPT HTML
delete window.__tlaModuleEvaluated;
delete window.__moduleWithTlaDepEvaluated;
const ns = await import.defer("./resources/module-with-tla-dep.js");
assert_true(window.__tlaModuleEvaluated, "TLA dependency should be eagerly evaluated");
assert_true(window.__moduleWithTlaDepEvaluated, "Deferred entrypoint can be evaluated due to cyclic TLA dependency");
assert_equals(ns.value, "tla-done", "Accessing namespace property should return the exported value");
```
#include "v8-callbacks.h"nit: please specify the full path.
```suggestion
#include "v8/include/v8-callbacks.h"
```
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Code-Review | +1 |
tnak, could you take a look?
Overall LGTM (non-owner). Apologie for my late review.
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
Would you add WPTs for cyclic imports something like the following? Please correct me if I misunderstand how cyclic dependency works. (cc: @hiro...@chromium.org)
```
// a.js
import defer * as nsB from "./b.js";
window.__moduleWithTlaDepEvaluated = true;
export const value = nsB.tlaValue;// b.js
window.__tlaModuleEvaluated = true;
await import.defer("./a.js");
export const tlaValue = "tla-done";// in WPT HTML
delete window.__tlaModuleEvaluated;
delete window.__moduleWithTlaDepEvaluated;
const ns = await import.defer("./resources/module-with-tla-dep.js");
assert_true(window.__tlaModuleEvaluated, "TLA dependency should be eagerly evaluated");
assert_true(window.__moduleWithTlaDepEvaluated, "Deferred entrypoint can be evaluated due to cyclic TLA dependency");
assert_equals(ns.value, "tla-done", "Accessing namespace property should return the exported value");
```
I realized that the expectation I wrote in the example was incorrect. My intention was something like this.
```
// b.js
window.__tlaModuleEvaluated = true;
await import("./a.js");
export const tlaValue = "tla-done";
```
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
thanks for the review! I can add such kind of test. There's just a couple of tweaks we need to do.
If we import defer `b.js` in `a`, and there's a cycle there from `b` back to `a`, what happens is that on the `nsB.tlaValue` we will throw the exception `TypeError: Deferred module is not ready for sync execution`. This happens because all deferred module accesses need to happen synchronously, but at the time we evaluate this, `b` module will be in `EVALUATING-ASYNC` state, which will cause this exception to happen. However to get the expectation you intend, we will need to change `export const value = nsB.tlaValue;` to something like `export function getValue() { return nsB.tlaValue; }`. This change allows `a.js` to evaluate successfully, and when the access `nsB.tlaValue` happens, the whole SCC is in `EVALUATED` state. Would these changes be fine for you? My idea is to add both cases.
Related details
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
`VirtualTestSuites` still LGTM
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
Thank you for the correction! Adding both cases SGTM.
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
I've added `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-defer-eval.tentative.html` and `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-fn-defer-eval.tentative.html`
nit: please specify the full path.
```suggestion
#include "v8/include/v8-callbacks.h"
```
Done
<!doctype html>Caio LimaCan we move the tests to public WPT
(under `third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/`)?We can mark the tests as `tentative`
https://web-platform-tests.org/writing-tests/file-names.html#test-features
Caio LimaSure. One question that I have is that this test right now fails, because we have the flag disabled when running this test here. Should we keep the expected like that as well? Additionally, show we also move the TLA version?
I've changed all tests for external.
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 2 comments![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Code-Review | +1 |
Takashi Nakayamatnak, could you take a look?
Overall LGTM (non-owner). Apologie for my late review.
Let me send my review back to @hiro...@chromium.org.
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
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 added 1 comment![]( "Open in Gerrit")
SetV8FlagIfOverridden(blink::features::kJavaScriptDeferPhaseImports,This flag should be added to src/gin/ instead.
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Caio Lima (Gerrit)
Caio Lima added 1 comment![]( "Open in Gerrit")
SetV8FlagIfOverridden(blink::features::kJavaScriptDeferPhaseImports,This flag should be added to src/gin/ instead.
I moved. Could you check if it's correct, please? Running tests locally, It seems to be working.
Related details
- David Baron
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Olivier Flückiger
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
`VirtualTestSuites` (unchanged) still LGTM
Related details
- Caio Lima
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Olivier Flückiger
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
GIN_EXPORT BASE_DECLARE_FEATURE(kJavaScriptDeferPhaseImports);you don't, strictly speaking, need this anymore, now that we have V8Flag magic (you can do `--enable-features=V8Flag_js_defer_import_eval`)
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Olivier Flückiger
Code-Coverage
Code-Owners
Caio Lima (Gerrit)
Caio Lima added 3 comments![]( "Open in Gerrit")
Thank you very much for your time reviewing it, and the patience to guide me through the proper way of adding tests and properly configuring flags. I'm sorry such changes are taking so much of your attention, but I'd probably need some re-reviews before being able to commit it, since last change cleared required votes from before.
SetV8FlagIfOverridden(blink::features::kJavaScriptDeferPhaseImports,Caio LimaThis flag should be added to src/gin/ instead.
I moved. Could you check if it's correct, please? Running tests locally, It seems to be working.
Done
GIN_EXPORT BASE_DECLARE_FEATURE(kJavaScriptDeferPhaseImports);you don't, strictly speaking, need this anymore, now that we have V8Flag magic (you can do `--enable-features=V8Flag_js_defer_import_eval`)
Thank you for letting me know about it. I just removed this flag and I'm using this V8Flag magic on Virtual tests as well.
Related details
- David Baron
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Olivier Flückiger
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
- Caio Lima
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Caio Lima
- Hiroshige Hayashizaki
- Kentaro Hara
- Kouhei Ueno
- Leszek Swirski
- Olivier Flückiger
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/58815.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Caio Lima (Gerrit)
Caio Lima voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[defer-import-eval] Add Blink integration for dynamic import.defer
Integrate V8's import.defer (deferred module evaluation) into Blink's
dynamic import pipeline. When a dynamic import uses the kDefer phase,
Blink calls V8's EvaluateForImportDefer and returns a deferred namespace
exotic object via GetModuleNamespace(kDefer).
Changes:
- Add v8::ModuleImportPhase parameter to V8ScriptRunner::EvaluateModule
and ModuleRecord::V8Namespace, dispatching to EvaluateForImportDefer
and GetModuleNamespace(kDefer) respectively.
- Add ModuleScript::EvaluateForImportPhase as the single evaluation
entry point, with RunScriptOnScriptStateAndReturnValue delegating to
it with kEvaluation.
- Thread import_phase through DynamicImportTreeClient and
ModuleResolutionSuccessCallback.
- Add JavaScriptDeferPhaseImports RuntimeEnabledFeature and wire it to
V8's --js-defer-import-eval flag.
- Add wpt_internal smoke test to VirtualTestSuite.
- M third_party/blink/renderer/bindings/core/v8/module_record.cc
- M third_party/blink/renderer/bindings/core/v8/module_record.h
- M third_party/blink/renderer/bindings/core/v8/module_record_test.cc
- M third_party/blink/renderer/bindings/core/v8/v8_script_runner.cc
- M third_party/blink/renderer/bindings/core/v8/v8_script_runner.h
- M third_party/blink/renderer/core/script/dynamic_module_resolver.cc
- M third_party/blink/renderer/core/script/module_script.cc
- M third_party/blink/renderer/core/script/module_script.h
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/basic-deferred-evaluation.tentative-expected.txt
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/basic-deferred-evaluation.tentative.html
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-defer-eval.tentative-expected.txt
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-defer-eval.tentative.html
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-fn-defer-eval.tentative-expected.txt
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-fn-defer-eval.tentative.html
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/cyclic-tla-dep-a.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/cyclic-tla-dep-b.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/cyclic-tla-fn-dep-a.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/cyclic-tla-fn-dep-b.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/module-with-tla-dep.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/side-effect-module.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/resources/tla-module.js
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/tla-deferred-evaluation.tentative-expected.txt
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/tla-deferred-evaluation.tentative.html
- A third_party/blink/web_tests/virtual/javascript-defer-phase-imports/README.md
- A third_party/blink/web_tests/virtual/javascript-defer-phase-imports/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/basic-deferred-evaluation.tentative-expected.txt
- A third_party/blink/web_tests/virtual/javascript-defer-phase-imports/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-defer-eval.tentative-expected.txt
- A third_party/blink/web_tests/virtual/javascript-defer-phase-imports/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/cyclic-tla-fn-defer-eval.tentative-expected.txt
- A third_party/blink/web_tests/virtual/javascript-defer-phase-imports/external/wpt/html/semantics/scripting-1/the-script-element/module/dynamic-import/tla-deferred-evaluation.tentative-expected.txt
Code-Review: +1 by Takashi Nakayama, +1 by David Baron
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/58815
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |