[wasm] With lazy compilation, don't eagerly compile large functions [v8/v8 : main]
Andreas Haas (Gerrit)
2 comments:
Patchset:
I added a test now. Please take a look.
File src/wasm/module-compiler.cc:
Patch Set #4, Line 3146:
if (FLAG_experimental_wasm_gc && !FLAG_wasm_lazy_compilation) {Isn't this whole code meaningless by now, because we shipped dynamic tiering?
To view, visit change 3790860. To unsubscribe, or for help writing mail filters, visit settings.
Jakob Kummerow (Gerrit)
Attention is currently required from: Andreas Haas.
Patch set 5:Code-Review +1
2 comments:
Patchset:
Two questions about this test:
• in which way would it fail (or otherwise notify us) if we accidentally did eagerly compile the huge function?
• when the test doesn't fail, how much time does it need on a slow configuration (such as a simulator build with slow DCHECKs and a sanitizer)?I'd be fine with not having this test if you conclude that it's not a particularly useful way to spend try bot time.
File src/wasm/module-compiler.cc:
Patch Set #4, Line 3146:
if (FLAG_experimental_wasm_gc && !FLAG_wasm_lazy_compilation) {Isn't this whole code meaningless by now, because we shipped dynamic tiering?
Looks like in the default configuration this code is indeed not needed any more; but dynamic tiering can still be disabled via flags, so I'm not sure we should rip it out just yet.
To view, visit change 3790860. To unsubscribe, or for help writing mail filters, visit settings.
Andreas Haas (Gerrit)
Patch set 6:Commit-Queue +2
1 comment:
Patchset:
Two questions about this test: […]
I don't think this test is particularly useful. It tests for a bug in special handling of a corner case that may never exist again in the future. If you are okay with it I will delete the test.
To view, visit change 3790860. To unsubscribe, or for help writing mail filters, visit settings.
Andreas Haas (Gerrit)
Attention is currently required from: Andreas Haas.
Patch set 7:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: test/mjsunit/regress/wasm/big-function-with-wasm-gc.js
Insertions: 0, Deletions: 27.
The diff is too large to show. Please review the diff.
```
[wasm] With lazy compilation, don't eagerly compile large functions
R=jkum...@chromium.org
Bug: v8:12926
Change-Id: I5942af918142a72158149e9820f49d4f07bb5266
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3790860
Reviewed-by: Jakob Kummerow <jkum...@chromium.org>
Commit-Queue: Andreas Haas <ah...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82118}
---
M src/wasm/module-compiler.cc
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc
index 7b3e511..26d44cb 100644
--- a/src/wasm/module-compiler.cc
+++ b/src/wasm/module-compiler.cc
@@ -3145,7 +3145,7 @@
ExecutionTier reached_tier =
CompilationStateImpl::ReachedTierField::decode(function_progress);
- if (FLAG_experimental_wasm_gc) {
+ if (FLAG_experimental_wasm_gc && !FLAG_wasm_lazy_compilation) {
// The Turbofan optimizations we enable for WasmGC code can (for now)
// take a very long time, so skip Turbofan compilation for super-large
// functions.
To view, visit change 3790860. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
Attention is currently required from: Andreas Haas.
1 comment:
Commit Message:
Patch Set #8, Line 7:
[wasm] With lazy compilation, don't eagerly compile large functionsMaybe today I am particularly bad with making sense of changes I see flying by, but this CL does not seem to do what this title says. Also, what's the motivation for this change? Shouldn't this be in the CL description?
It looks to me as if this CL actually *enables* eager Liftoff compilation of huge functions, even if lazy compilation is enabled.
Let me explain, and maybe you can show me where I am wrong:
If lazy compilation is off, this change does not change anything (as `!FLAG_wasm_lazy_compilation` will always be true).
If lazy compilation is on, we should have `required_baseline_tier` and `required_top_tier` both be `kNone` (this is set in `CompilationStateImpl::SetupCompilationProgressForFunction`). After this CL, we will change the baseline tier to `kLiftoff` for huge functions, i.e. huge functions will eagerly be compiled.
To view, visit change 3790860. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
Commit Message:
Patch Set #8, Line 7:
[wasm] With lazy compilation, don't eagerly compile large functions
Maybe today I am particularly bad with making sense of changes I see flying by, but this CL does not […]
Ok, I read the condition wrong, we actually *skip* Jakob's if-block if lazy compilation is on, to avoid accidentally triggering eager Liftoff compilation. So the title is actually correct :)
Sorry for the noise. I would still have welcomed some more description about what the problem is that this CL is avoiding.
To view, visit change 3790860. To unsubscribe, or for help writing mail filters, visit settings.