[for-of-performance] Add inlining [v8/v8 : main]
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 4 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
4 comments
ninja review! LGTM (but i didn't look at the resulting maglev graph nor generated code to verify)
ReduceResult result = ReduceCall(next_method, args, feedback_source);
if (!result.IsDoneWithValue()) return result;
ValueNode* result_object = result.value();See below
MaybeReduceResult done_maybe_result =
TryBuildLoadNamedProperty(result_object, broker()->done_string(),
done_feedback, build_generic_load);
if (done_maybe_result.IsDoneWithAbort()) return done_maybe_result;
DCHECK(done_maybe_result.IsDoneWithValue());
done = done_maybe_result.value();This could be
```
GET_VALUE_OR_ABORT(done, TryBuildLoadNameProperty(...));
```
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
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 added 1 comment![]( "Open in Gerrit")
ReduceResult result = ReduceCall(next_method, args, feedback_source);for this call and the done string load, you need to add a lazy deopt continuation, so that if the call or field load trigger a lazy deopt, we complete the ForOfNext operation before returning to the bytecode. The two loads will also need an eager deopt continuation, for the same reason except for eager deopts rather than lazy ones (e.g. if a map check fails for the load, we need to deopt but also complete the operation).
`TryReduceGetIterator` has examples of both.
Related details
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
ReduceResult result = ReduceCall(next_method, args, feedback_source);for this call and the done string load, you need to add a lazy deopt continuation, so that if the call or field load trigger a lazy deopt, we complete the ForOfNext operation before returning to the bytecode. The two loads will also need an eager deopt continuation, for the same reason except for eager deopts rather than lazy ones (e.g. if a map check fails for the load, we need to deopt but also complete the operation).
`TryReduceGetIterator` has examples of both.
Oops, sorry, I totally missed this. Rezvan, could you also check whether it's feasible to add a test which will fail if this is not implemented properly?
Related details
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Message from Rezvan Mahdavi Hezaveh![]( "Open in Gerrit")
- Leszek Swirski
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 5 comments![]( "Open in Gerrit")
Leszek, I am getting a runtime error referring to the incorrect number of paramteres in the descriptort some time to figure this out but I am stuck. Let's keep our next meeting and discuss it. Feel free to review this CL before our meeting as well and I will apply the comments. Thanks!
Marja, I would appreciate if you take another look too. Thanks!
ReduceResult result = ReduceCall(next_method, args, feedback_source);Marja Hölttäfor this call and the done string load, you need to add a lazy deopt continuation, so that if the call or field load trigger a lazy deopt, we complete the ForOfNext operation before returning to the bytecode. The two loads will also need an eager deopt continuation, for the same reason except for eager deopts rather than lazy ones (e.g. if a map check fails for the load, we need to deopt but also complete the operation).
`TryReduceGetIterator` has examples of both.
Oops, sorry, I totally missed this. Rezvan, could you also check whether it's feasible to add a test which will fail if this is not implemented properly?
Leszek, I looked at `TryReduceGetIterator` and added lazy scope for next method call, but I did not understand why we needed the same for done string load (and not the value string load)? For the next method call and two loads I added eager scopes.
Marja, I added the test `for-of-array-iterator-optimization-maglev-eager-check.js` with the help from gemini code assist but I am not sure if it really tests what you are referring to.
ReduceResult result = ReduceCall(next_method, args, feedback_source);
if (!result.IsDoneWithValue()) return result;
ValueNode* result_object = result.value();Rezvan Mahdavi HezavehSee below
Done
MaybeReduceResult done_maybe_result =
TryBuildLoadNamedProperty(result_object, broker()->done_string(),
done_feedback, build_generic_load);
if (done_maybe_result.IsDoneWithAbort()) return done_maybe_result;
DCHECK(done_maybe_result.IsDoneWithValue());
done = done_maybe_result.value();This could be
```
GET_VALUE_OR_ABORT(done, TryBuildLoadNameProperty(...));
```
Done
ReduceResult value_result = Select(Rezvan Mahdavi Hezavehditto
Done
Related details
- Leszek Swirski
- 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. |
Leszek Swirski (Gerrit)
Leszek Swirski added 4 comments![]( "Open in Gerrit")
Leszek, I am getting a runtime error referring to the incorrect number of paramteres in the descriptort some time to figure this out but I am stuck. Let's keep our next meeting and discuss it. Feel free to review this CL before our meeting as well and I will apply the comments. Thanks!
Marja, I would appreciate if you take another look too. Thanks!
It's because the continuation builtins expect one extra argument (the call result) on the stack, which is technically documented somewhere but a bit hidden and counterintuitive
ReduceResult result = ReduceCall(next_method, args, feedback_source);Marja Hölttäfor this call and the done string load, you need to add a lazy deopt continuation, so that if the call or field load trigger a lazy deopt, we complete the ForOfNext operation before returning to the bytecode. The two loads will also need an eager deopt continuation, for the same reason except for eager deopts rather than lazy ones (e.g. if a map check fails for the load, we need to deopt but also complete the operation).
`TryReduceGetIterator` has examples of both.
Rezvan Mahdavi HezavehOops, sorry, I totally missed this. Rezvan, could you also check whether it's feasible to add a test which will fail if this is not implemented properly?
Leszek, I looked at `TryReduceGetIterator` and added lazy scope for next method call, but I did not understand why we needed the same for done string load (and not the value string load)? For the next method call and two loads I added eager scopes.
Marja, I added the test `for-of-array-iterator-optimization-maglev-eager-check.js` with the help from gemini code assist but I am not sure if it really tests what you are referring to.
we need a lazy continuation for the "done" load because we still need to do the "value" load after it completes -- afaict we don't need a lazy continuation after the "value" load because at that point (after the load) everything is done anyway.
this, Builtin::kForOfNext, {},unfortunately you can't just use the `ForOfNext` builtin here -- the eager continuation happens here _after_ the next method has already been called, so calling `ForOfNext` will call the next method twice. Instead, you should create a new builtin which does just the done and value loads, which takes as input the next return value.
In general, you've added scopes here but not real continuation builtins -- you're attempting to restart the ForOfNext operation in all these cases, whereas a continuation builtin needs to, like the name suggests, _continue_ the operaiton, since it's purpose is to take a partial state that you deopted in the middle of, and finish doing what it was doing before it deopted, without doing things twice.
result_object, broker()->value_string(), value_feedback));you need to load "done" before "value" -- this is observable if there are accessors.
Related details
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 6 comments![]( "Open in Gerrit")
In this latest patch:
- we two lazy continuations. One for loading done and one for loading value.
- we also have two eager continuations. One for loading done and one for loading value.
- _ the eager deopt and lazy deopt tests looks correct to me and they are passing. Please take a look and let me know if I miss anything.
I applied all the comments from our 1:1 meeting with Leszek.
I added one unresolved comment explaining where I stuck at right now. PTAL and let me know if you have any suggestions.
ReduceResult result = ReduceCall(next_method, args, feedback_source);Marja Hölttäfor this call and the done string load, you need to add a lazy deopt continuation, so that if the call or field load trigger a lazy deopt, we complete the ForOfNext operation before returning to the bytecode. The two loads will also need an eager deopt continuation, for the same reason except for eager deopts rather than lazy ones (e.g. if a map check fails for the load, we need to deopt but also complete the operation).
`TryReduceGetIterator` has examples of both.
Rezvan Mahdavi HezavehOops, sorry, I totally missed this. Rezvan, could you also check whether it's feasible to add a test which will fail if this is not implemented properly?
Leszek SwirskiLeszek, I looked at `TryReduceGetIterator` and added lazy scope for next method call, but I did not understand why we needed the same for done string load (and not the value string load)? For the next method call and two loads I added eager scopes.
Marja, I added the test `for-of-array-iterator-optimization-maglev-eager-check.js` with the help from gemini code assist but I am not sure if it really tests what you are referring to.
we need a lazy continuation for the "done" load because we still need to do the "value" load after it completes -- afaict we don't need a lazy continuation after the "value" load because at that point (after the load) everything is done anyway.
That makes sense, done.
unfortunately you can't just use the `ForOfNext` builtin here -- the eager continuation happens here _after_ the next method has already been called, so calling `ForOfNext` will call the next method twice. Instead, you should create a new builtin which does just the done and value loads, which takes as input the next return value.
In general, you've added scopes here but not real continuation builtins -- you're attempting to restart the ForOfNext operation in all these cases, whereas a continuation builtin needs to, like the name suggests, _continue_ the operaiton, since it's purpose is to take a partial state that you deopted in the middle of, and finish doing what it was doing before it deopted, without doing things twice.
Okay. I got it. I added one continuation for eager deopt that only loads value and done values.
result_object, broker()->value_string(), value_feedback));you need to load "done" before "value" -- this is observable if there are accessors.
Done
MaybeReduceResult MaglevGraphBuilder::TryReduceForOfNext(Running `IteratorsWithOptimization` test from JSTests5.json (related perf test for ForOfNext optimization) crashed in `CHECK(bytecode_analysis().IsLoopHeader(offset))` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/maglev/maglev-graph-builder.cc;l=17265)
It looks like that the generated graph from inlining does not have a correct loop header. I tried a few ways to find the root cause but I was unsuccessful.
I need help debugging this issue, I think it's deeper than my understanding of optimization and deoptimization compiler layers.
MaybeReduceResult done_result = TryBuildLoadNamedProperty(
result_object, broker()->done_string(), done_feedback);
if (done_result.IsDoneWithAbort() || done_result.IsFail()) {
return done_result;
}I used `GET_VALUE_OR_ABORT` here and below but it does not check for `isFail` case. Looking at the `TryReduceGetIterator`, I decided to not use the macro and add checks here.
Related details
- Leszek Swirski
- 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. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 2 comments![]( "Open in Gerrit")
I believe the CL is complete and ready for review.
MaybeReduceResult MaglevGraphBuilder::TryReduceForOfNext(Running `IteratorsWithOptimization` test from JSTests5.json (related perf test for ForOfNext optimization) crashed in `CHECK(bytecode_analysis().IsLoopHeader(offset))` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/maglev/maglev-graph-builder.cc;l=17265)
It looks like that the generated graph from inlining does not have a correct loop header. I tried a few ways to find the root cause but I was unsuccessful.
I need help debugging this issue, I think it's deeper than my understanding of optimization and deoptimization compiler layers.
Following Leszek's suggestion and return the `result` instead of `ReduceResult::Done()` inside `VisitForOfNext()` resolved this issue.
Related details
- Leszek Swirski
- Marja Hölttä
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 7 comments![]( "Open in Gerrit")
generally looking good; here are some initial comments. I'll also take a closer look at the generated code...
BranchIfToBooleanIsTrue(var_done.value(), &if_done_true, &if_done_false);Suggestion: Could you split this fix out of this CL, since it's pretty unrelated? And in that CL, could you add tests 1) checking that we don't try to retrieve the value when 'done' is true 2) that your builtin also works correctly when either the done getter or the value getter throws an exception?
// --for-of-optimizationDoes this actually kick in? Don't you need another Flags: on this line too?
// If your EagerDeoptFrameScope was missing or wrong, the engine would
// likely crash here because it wouldn't know how to return to the
// middle of the ForOf iteration in the interpreter.I'm not fully sure what this test is asserting... it doesn't have a getter for "value", so it can't assert that after the deopt, we retrieve the value exactly once (not twice, not zero times).
I'm also a bit unsure, just by reading it, whether it will deopt or not. Since the warmup phase already returns the oddly-shaped object, won't we have it in the feedback, and Maglev actually won't deopt? It also doesn't have any assertUnoptimized calls.
(Is this vibe coded? This comment sounds a bit like it...)
console.log('Deoptimization successful and recovered!');The tests should ideally be silent, please no console.log in tests unless really really important.
assertUnoptimized(testForOfDeopt);The only thing test seems to assert is that testForDeopt is now unoptimized, which is easy to believe, since the test quite forcefully deopts it. But here too we don't assert that the continuation does the right thing (apart from obvious things like not crashing) or that the values produced by the iteration are correct.
console.log('Deoptimization successful and recovered!');Ditto
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä added 4 comments![]( "Open in Gerrit")
TF_BUILTIN(ForOfNextLoadValueEagerDeoptContinuation,Don't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
return {var_value.value(), var_done.value()};A follow-up idea, def not for this CL: I suppose we don't need two return values, since we only need the "value" if "done" is false, so, if we use a sentinel for the "done = true" case (something that cannot be a valid value), we could have only one return value. Not sure if that makes life easier somehow (I remember having two return values used to be a pain sometimes but maybe those problems have been resolved meanwhile?)
// --for-of-optimizationDoes this actually kick in? Don't you need another Flags: on this line too?
Verified that this does *not* kick in. I slapped an assertTrue(false) here and ran x64.optdebug.check, and observed which command line was used for this test. Here it is:
`
Command: out/x64.optdebug/d8 --test test/mjsunit/mjsunit.js test/mjsunit/es6/for-of-array-iterator-optimization-maglev-eager-check.js --random-seed=-2054084000 --nohard-abort --verify-heap --enable-slow-asserts --allow-natives-syntax --maglev --no-stress-maglev
`
// If your EagerDeoptFrameScope was missing or wrong, the engine would
// likely crash here because it wouldn't know how to return to the
// middle of the ForOf iteration in the interpreter.I'm not fully sure what this test is asserting... it doesn't have a getter for "value", so it can't assert that after the deopt, we retrieve the value exactly once (not twice, not zero times).
I'm also a bit unsure, just by reading it, whether it will deopt or not. Since the warmup phase already returns the oddly-shaped object, won't we have it in the feedback, and Maglev actually won't deopt? It also doesn't have any assertUnoptimized calls.
(Is this vibe coded? This comment sounds a bit like it...)
Also verified this test 1) does not deopt 2) does not hit the ForOfNextLoadValueEagerDeoptContinuation (I just put a Print in there)
-> I'll let you take it from here to update this test so that it actually hits that code path, please let me know if you need help!
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 11 comments![]( "Open in Gerrit")
Thanks for your comments Marja! I have decided to spend more time on this CL in the following couple weeks and hand it over to your team by the end of Q1 if it is not finished.
Can you please take a look to the new tests and my comments here, and help me understand if the eager test is incorrect or the eager deopt code path? Thanks!
Print("Inside ForOfNextLoadDoneLazyDeoptContinuation --------");Keeping the prints for now to debug the code, I'll remove them before landing the CL.
Don't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
Based on my understanding no, please correct me if I am wrong. Eager deopts happen *before* loading the values and lazy deopts happen *after* loading a value. So:
1) LoadDoneEagerDeopt: happens when we do not have the `done` and `value` values.
2) LoadValueEagerDeopt: happens when we have `done` but we do not have `value`.
3) LoadDoneLazyDeopt: happens when we have `done` but we do not have `value`.
4) LoadValueLazyDeopt: happens when we have both `done` and `value` so we do not need this one.
BranchIfToBooleanIsTrue(var_done.value(), &if_done_true, &if_done_false);Suggestion: Could you split this fix out of this CL, since it's pretty unrelated? And in that CL, could you add tests 1) checking that we don't try to retrieve the value when 'done' is true 2) that your builtin also works correctly when either the done getter or the value getter throws an exception?
Sure, done.
A follow-up idea, def not for this CL: I suppose we don't need two return values, since we only need the "value" if "done" is false, so, if we use a sentinel for the "done = true" case (something that cannot be a valid value), we could have only one return value. Not sure if that makes life easier somehow (I remember having two return values used to be a pain sometimes but maybe those problems have been resolved meanwhile?)
I like this idea, and yes having two return values is still a pain. It could be a good follow-up CL!
Marja HölttäDoes this actually kick in? Don't you need another Flags: on this line too?
Verified that this does *not* kick in. I slapped an assertTrue(false) here and ran x64.optdebug.check, and observed which command line was used for this test. Here it is:
`
Command: out/x64.optdebug/d8 --test test/mjsunit/mjsunit.js test/mjsunit/es6/for-of-array-iterator-optimization-maglev-eager-check.js --random-seed=-2054084000 --nohard-abort --verify-heap --enable-slow-asserts --allow-natives-syntax --maglev --no-stress-maglev
`
Ah, that's because of formatting. I totally missed it, thanks for catching it.
// If your EagerDeoptFrameScope was missing or wrong, the engine would
// likely crash here because it wouldn't know how to return to the
// middle of the ForOf iteration in the interpreter.Marja HölttäI'm not fully sure what this test is asserting... it doesn't have a getter for "value", so it can't assert that after the deopt, we retrieve the value exactly once (not twice, not zero times).
I'm also a bit unsure, just by reading it, whether it will deopt or not. Since the warmup phase already returns the oddly-shaped object, won't we have it in the feedback, and Maglev actually won't deopt? It also doesn't have any assertUnoptimized calls.
(Is this vibe coded? This comment sounds a bit like it...)
Also verified this test 1) does not deopt 2) does not hit the ForOfNextLoadValueEagerDeoptContinuation (I just put a Print in there)
-> I'll let you take it from here to update this test so that it actually hits that code path, please let me know if you need help!
The test is vibe coded, yeah. I tried to read and understand it and make sure it is correct (which I have not been successful apperantly...)
For some reason I had two versions of this file locally and the other one had `AssertUnoptimized()`. Sorry for the confusion.
I ended up rewriting the test totally. I also run the test with `--trace-maglev-graph-building --trace-deopt` and in the last line of the output I see
`[bailout (kind: deopt-eager, reason: Insufficient type feedback for generic named access): begin. deoptimizing 0x7a8b010319cd <JSFunction testForOfEagerDeopt (sfi = 0x7a8b01031651)>, 0x7a81010011b5 <Code MAGLEV>, opt id 0, node id 0, bytecode offset 7, deopt exit 0, FP to SP delta 24, caller SP 0x7ffc899bd7b8, pc 0x560be0000145]`
I also added an assert to check the result. But unfortunately, the print statement I added in my EagerDeoptContinuation is not printed when I run the test, so eager deopt is not working or is not triggered by this test.
I cannot determine if the test is incorrect or the eager deopt code. Can you help me understand this part Marja?
console.log('Deoptimization successful and recovered!');The tests should ideally be silent, please no console.log in tests unless really really important.
Done
// 3. Trigger Eager Deopt on the SECOND iteration of a single call
// Iter 1: getMap() -> maps[0] (value 10). sum = 10.
// Iter 2: getMap() -> maps[1] (value 32). CHECKMAPS FAILS -> DEOPT.
// Interpreter resumes Iter 2: adds 32. sum = 42.For both tests I kept the generated comments to make the test readable. I can remove them if they look unnecessary to you.
assertUnoptimized(testForOfDeopt);The only thing test seems to assert is that testForDeopt is now unoptimized, which is easy to believe, since the test quite forcefully deopts it. But here too we don't assert that the continuation does the right thing (apart from obvious things like not crashing) or that the values produced by the iteration are correct.
I run the test with `--trace-maglev-graph-building --trace-deopt` and in the last line of the output I see
`[bailout (kind: deopt-lazy, reason: (unknown)): begin. deoptimizing 0x7a15010317bd <JSFunction testForOfDeopt (sfi = 0x7a1501031555)>, 0x7a0b01001715 <Code MAGLEV>, opt id 0, node id 0, bytecode offset 409, deopt exit 18, FP to SP delta 96, caller SP 0x7fffcd544e50, pc 0x55c57e8814c4]`. Then I added an assert to check the result. Also, adding a print inside `ForOfNextLoadDoneLazyDeoptContinuation` confirming that we enter the continuation.
I think the combination of these three show that the lazy deopt is working correctly. Let me know if it is still incomplete.
console.log('Deoptimization successful and recovered!');Rezvan Mahdavi HezavehDitto
Done
Related details
- Leszek Swirski
- 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. |
Marja Hölttä (Gerrit)
Marja Hölttä added 1 comment![]( "Open in Gerrit")
// If your EagerDeoptFrameScope was missing or wrong, the engine would
// likely crash here because it wouldn't know how to return to the
// middle of the ForOf iteration in the interpreter.Marja HölttäI'm not fully sure what this test is asserting... it doesn't have a getter for "value", so it can't assert that after the deopt, we retrieve the value exactly once (not twice, not zero times).
I'm also a bit unsure, just by reading it, whether it will deopt or not. Since the warmup phase already returns the oddly-shaped object, won't we have it in the feedback, and Maglev actually won't deopt? It also doesn't have any assertUnoptimized calls.
(Is this vibe coded? This comment sounds a bit like it...)
Rezvan Mahdavi HezavehAlso verified this test 1) does not deopt 2) does not hit the ForOfNextLoadValueEagerDeoptContinuation (I just put a Print in there)
-> I'll let you take it from here to update this test so that it actually hits that code path, please let me know if you need help!
The test is vibe coded, yeah. I tried to read and understand it and make sure it is correct (which I have not been successful apperantly...)
For some reason I had two versions of this file locally and the other one had `AssertUnoptimized()`. Sorry for the confusion.
I ended up rewriting the test totally. I also run the test with `--trace-maglev-graph-building --trace-deopt` and in the last line of the output I see
`[bailout (kind: deopt-eager, reason: Insufficient type feedback for generic named access): begin. deoptimizing 0x7a8b010319cd <JSFunction testForOfEagerDeopt (sfi = 0x7a8b01031651)>, 0x7a81010011b5 <Code MAGLEV>, opt id 0, node id 0, bytecode offset 7, deopt exit 0, FP to SP delta 24, caller SP 0x7ffc899bd7b8, pc 0x560be0000145]`
I also added an assert to check the result. But unfortunately, the print statement I added in my EagerDeoptContinuation is not printed when I run the test, so eager deopt is not working or is not triggered by this test.I cannot determine if the test is incorrect or the eager deopt code. Can you help me understand this part Marja?
Here's how I investigated it:
1) The bailout is:
```
[bailout (kind: deopt-eager, reason: Insufficient type feedback for generic named access): begin. deoptimizing 0x1ef101031ca9 <JSFunction testForOfEagerDeopt (sfi = 0x1ef10103192d)>, 0x0a3d010011b5 <Code MAGLEV>, opt id 0, node id 0, bytecode offset 7, deopt exit 0, FP to SP delta 24, caller SP 0x7ffd845687d8, pc 0x7f32e0000145]
```
So we see the deopt is in `testForOfEagerDeopt`, as expected. But the bytecode offset is 7, which is this:
```
[generated bytecode for function: testForOfEagerDeopt (0x1ef10103192d <SharedFunctionInfo testForOfEagerDeopt>)]
Bytecode length: 135
Parameter count 1
Register count 15
Frame size 120
805 S> 0xa3d01000e28 @ 0 : 0c LdaZero
0xa3d01000e29 @ 1 : d2 Star0
826 S> 0xa3d01000e2a @ 2 : 19 04 LdaImmutableCurrentContextSlot [4]
0xa3d01000e2c @ 4 : b8 00 ThrowReferenceErrorIfHole [0:"iterable"]
0xa3d01000e2e @ 6 : cc Star6
0xa3d01000e2f @ 7 : bf f3 00 02 GetIterator r6, FBV[0], FBV[2]
0xa3d01000e33 @ 11 : cd Star5
0xa3d01000e34 @ 12 : 33 f4 01 04 GetNamedProperty r5, [1:"next"], FBV[4]
```
so it's right at the start of the function, when doing GetIterator, before it even starts iterating. That doesn't sound right.
2) Looking at your test, looks like your warm up phase doesn't even execute testForOfEagerDeopt, so... that's not going to work.
Gemini is great but it often fails at subtleties like this, when it creates a test that looks like it might be correct but it actually isn't. Looks like we need some human brainpower to verify these things atm.
I think the "getMap" idea is correct, it's good you have this func which you never opt (I hope this also means you never inline it, please verify this is the case, I want to be super skeptical with all the details like this) and which you can command to return an object of an odd shape.
1) I'd make the trigger even more explicit so it's easier to control from the outside. Like `if (triggerDeopt) { return odd; }` and then you can set triggerDeopt to true when you want to deopt.
2) Now you can warm up properly, just make triggerDeopt be false the whole time and warm up the function you're going to optimize, not some other func.
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
TF_BUILTIN(ForOfNextLoadValueEagerDeoptContinuation,Rezvan Mahdavi HezavehDon't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
Based on my understanding no, please correct me if I am wrong. Eager deopts happen *before* loading the values and lazy deopts happen *after* loading a value. So:
1) LoadDoneEagerDeopt: happens when we do not have the `done` and `value` values.
2) LoadValueEagerDeopt: happens when we have `done` but we do not have `value`.
3) LoadDoneLazyDeopt: happens when we have `done` but we do not have `value`.
4) LoadValueLazyDeopt: happens when we have both `done` and `value` so we do not need this one.
Ah, are you saying that we don't need an explicit continuation in the "value getter lazy deopts" case, but the right thing will happen automatically?
Please add a test for the "value getter lazy deopts" nevertheless.
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
TF_BUILTIN(ForOfNextLoadValueEagerDeoptContinuation,Rezvan Mahdavi HezavehDon't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
Marja HölttäBased on my understanding no, please correct me if I am wrong. Eager deopts happen *before* loading the values and lazy deopts happen *after* loading a value. So:
1) LoadDoneEagerDeopt: happens when we do not have the `done` and `value` values.
2) LoadValueEagerDeopt: happens when we have `done` but we do not have `value`.
3) LoadDoneLazyDeopt: happens when we have `done` but we do not have `value`.
4) LoadValueLazyDeopt: happens when we have both `done` and `value` so we do not need this one.
Ah, are you saying that we don't need an explicit continuation in the "value getter lazy deopts" case, but the right thing will happen automatically?
Please add a test for the "value getter lazy deopts" nevertheless.
You can use the helper %DeoptimizeFunction for triggering a lazy deopt.
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
TF_BUILTIN(ForOfNextLoadValueEagerDeoptContinuation,Rezvan Mahdavi HezavehDon't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
Marja HölttäBased on my understanding no, please correct me if I am wrong. Eager deopts happen *before* loading the values and lazy deopts happen *after* loading a value. So:
1) LoadDoneEagerDeopt: happens when we do not have the `done` and `value` values.
2) LoadValueEagerDeopt: happens when we have `done` but we do not have `value`.
3) LoadDoneLazyDeopt: happens when we have `done` but we do not have `value`.
4) LoadValueLazyDeopt: happens when we have both `done` and `value` so we do not need this one.
Marja HölttäAh, are you saying that we don't need an explicit continuation in the "value getter lazy deopts" case, but the right thing will happen automatically?
Please add a test for the "value getter lazy deopts" nevertheless.
You can use the helper %DeoptimizeFunction for triggering a lazy deopt.
or... I think that's the right function, but please verify 😊
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
One more tip for developing the test (only during development ofc): add a lot of debug prints, like this:
```
function getMap() {
print("in getmap");
if (triggerDeopt) {
print("getmap returning a weird shaped object");
}
}
```
and
```
function testForOfEagerDeopt() {
let sum = 0;
for (const x of iterable) {
print("iterating, got " + x);
sum += x;
// you can also control the "done" from the outside so no break needed here
// let's keep this function as simple as possible!
}
return sum;
}
```
-> With these, you would've noticed that the deopt does *not* happen after the "getmap returning a weird shaped object" printout, but earlier.
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
Thanks for the detailed explanation Marja! I made some progress today, but apparently not enough. After applying your suggestions, I see the following bailout:
`[bailout (kind: deopt-eager, reason: wrong map): begin. deoptimizing 0x79d101031b91 <JSFunction testForOfEagerDeopt (sfi = 0x79d101031901)>, 0x79c301001799 <Code MAGLEV>, opt id 0, node id 0, bytecode offset 26, deopt exit 6, FP to SP delta 96, caller SP 0x7ffcee313ae0, pc 0x558ebe401299]`
and offset is 26, which is:
```
0x79c301000dfc @ 24 : 11 LdaTrue
0x79c301000dfd @ 25 : cb Star7
1086 s> 0x79c301000dfe @ 26 : b3 f4 f5 f3 06 ForOfNext r5, r4, r6-r7, FBV[6]
0x79c301000e03 @ 31 : 0b f2 Ldar r7
0x79c301000e05 @ 33 : a3 29 JumpIfToBooleanTrue [41] (0x79c301000e2e @ 74)
```
The location seems correct now but we still don't go into EagerDeoptContinuation (there is no print line of `Inside ForOfNextLoadDoneEagerDeoptContinuation --------` in the console). As a result, and the fact that True was loaded for `done` in offset 24, the code returns immediately and the assertEqual on `result` fails.
I believe the test is correct now, can you confirm that? If yes, can you take a look at my EagerContinuations and see if you notice anything incorrect there? Thanks!
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
alright, I was having a lot of fun with this one, here are the results 😊
RETURN_IF_ABORT(BuildCheckJSReceiver(result_object));And your test discovered a bug here. It's very subtle, I had to get a lot of help from Leszek for this one 😊
The bug is that (unintuitively?), you have two checks here, CheckJSReceiver and CheckMaps (below), and you're trying to attach two different deopt continuations to them. (The default one, which will do whatever, to CheckJSReceiver, and then your custom one to CheckMaps.)
That doesn't work, since you cannot do that in Maglev (have two consecutive checks with different continuations.)
You'll anyway need to pull your EagerDeoptScope up so that it covers the CheckJSReceiver failing, since you also need your continuation to kick in when the object returned by next() is not a JSReceiver. That should fix this bug too.
However, now the continuation is hit but the test still fails. I didn't debug that yet.
Also, there might be some problems with the "next" continuation, I didn't yet check whether that one is now wired up correctly
Please also add a test for the "CheckJSReceiver failing" case, since you don't seem to have one yet.
Good news, the test is now legit, I think. It could be a bit simpler, like this:
```
const maps_normal = [
{value: 10, done: false}, {value: 20, done: false},
{value: undefined, done: true}
];
const maps_weird = [
{value: 11, done: false}, {value: 21, done: false, extra: 'trigger deopt'},
{value: undefined, done: true}
];
let triggerDeopt = false;
function getMap(iteration) {
if (!triggerDeopt) {
return maps_normal[iteration];
}
return maps_weird[iteration];
}
%NeverOptimizeFunction(getMap);const iterable = {
[Symbol.iterator]() {
let iteration = 0;
return {
next() {
return getMap(iteration++);
}
};
}
};
```but okay, your test is also legit, that is not the bug
Marja Hölttä (Gerrit)
Marja Hölttä added 1 comment![]( "Open in Gerrit")
{value: 10, done: false}, {value: 32, done: false, extra: 'trigger'},Another missing test: what if there's no "done" property in the result object, what happens then?
(Do you already have a test for that for the lower tiers? If not, those should be added too.)
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 3 comments![]( "Open in Gerrit")
I kept my tomorrow's meeting with Leszek to discuss the unresolved comments.
RETURN_IF_ABORT(BuildCheckJSReceiver(result_object));And your test discovered a bug here. It's very subtle, I had to get a lot of help from Leszek for this one 😊
The bug is that (unintuitively?), you have two checks here, CheckJSReceiver and CheckMaps (below), and you're trying to attach two different deopt continuations to them. (The default one, which will do whatever, to CheckJSReceiver, and then your custom one to CheckMaps.)
That doesn't work, since you cannot do that in Maglev (have two consecutive checks with different continuations.)
You'll anyway need to pull your EagerDeoptScope up so that it covers the CheckJSReceiver failing, since you also need your continuation to kick in when the object returned by next() is not a JSReceiver. That should fix this bug too.
However, now the continuation is hit but the test still fails. I didn't debug that yet.
Also, there might be some problems with the "next" continuation, I didn't yet check whether that one is now wired up correctly
Please also add a test for the "CheckJSReceiver failing" case, since you don't seem to have one yet.
Thanks for debugging Marja! After moving the EagerDeoptScope up, the test hit the continuation correctly but as you said the result is still incorrect (<11> instead of <32> after applying your suggestion to the test). It looks like the continuation ignores the returned results. Could this be the problem? maybe because we have two returned values from continuations?
// If your EagerDeoptFrameScope was missing or wrong, the engine would
// likely crash here because it wouldn't know how to return to the
// middle of the ForOf iteration in the interpreter.Happy to hear that and thanks for the confirmation!
I also applied your suggestion to simplify the test.
Related details
- Leszek Swirski
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä added 2 comments![]( "Open in Gerrit")
// Iter 1: getMap(0) -> maps_weird[0] (value 10). sum = 11.11
assertUnoptimized(testForOfDeopt);Rezvan Mahdavi HezavehThe only thing test seems to assert is that testForDeopt is now unoptimized, which is easy to believe, since the test quite forcefully deopts it. But here too we don't assert that the continuation does the right thing (apart from obvious things like not crashing) or that the values produced by the iteration are correct.
I run the test with `--trace-maglev-graph-building --trace-deopt` and in the last line of the output I see
`[bailout (kind: deopt-lazy, reason: (unknown)): begin. deoptimizing 0x7a15010317bd <JSFunction testForOfDeopt (sfi = 0x7a1501031555)>, 0x7a0b01001715 <Code MAGLEV>, opt id 0, node id 0, bytecode offset 409, deopt exit 18, FP to SP delta 96, caller SP 0x7fffcd544e50, pc 0x55c57e8814c4]`. Then I added an assert to check the result. Also, adding a print inside `ForOfNextLoadDoneLazyDeoptContinuation` confirming that we enter the continuation.
I think the combination of these three show that the lazy deopt is working correctly. Let me know if it is still incomplete.
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 3 comments![]( "Open in Gerrit")
The eager test is passing after manually storing the resuults into registers, thanks Leszek for the pointer!
return {var_value.value(), var_done.value()};Rezvan Mahdavi HezavehA follow-up idea, def not for this CL: I suppose we don't need two return values, since we only need the "value" if "done" is false, so, if we use a sentinel for the "done = true" case (something that cannot be a valid value), we could have only one return value. Not sure if that makes life easier somehow (I remember having two return values used to be a pain sometimes but maybe those problems have been resolved meanwhile?)
I like this idea, and yes having two return values is still a pain. It could be a good follow-up CL!
SuspendGeneratorBaseline
RETURN_IF_ABORT(BuildCheckJSReceiver(result_object));Rezvan Mahdavi HezavehAnd your test discovered a bug here. It's very subtle, I had to get a lot of help from Leszek for this one 😊
The bug is that (unintuitively?), you have two checks here, CheckJSReceiver and CheckMaps (below), and you're trying to attach two different deopt continuations to them. (The default one, which will do whatever, to CheckJSReceiver, and then your custom one to CheckMaps.)
That doesn't work, since you cannot do that in Maglev (have two consecutive checks with different continuations.)
You'll anyway need to pull your EagerDeoptScope up so that it covers the CheckJSReceiver failing, since you also need your continuation to kick in when the object returned by next() is not a JSReceiver. That should fix this bug too.
However, now the continuation is hit but the test still fails. I didn't debug that yet.
Also, there might be some problems with the "next" continuation, I didn't yet check whether that one is now wired up correctly
Please also add a test for the "CheckJSReceiver failing" case, since you don't seem to have one yet.
Thanks for debugging Marja! After moving the EagerDeoptScope up, the test hit the continuation correctly but as you said the result is still incorrect (<11> instead of <32> after applying your suggestion to the test). It looks like the continuation ignores the returned results. Could this be the problem? maybe because we have two returned values from continuations?
My suspicion about ignoring the returned result was somewhat correct. In my discussion with Leszek, I learned that eager continuations store the result in accumulator and our code stub assember code expect to see the result on two registers. SO, we decided to store the results on the registers manually in the eager continuations (and later follow your suggestion on changing the builtins to only have one return value in a follow-up CL). After manually saving the results into resgisters, the eager test now is passing! Please take a look at the code and I will add the additional tests you have requested hopefully tomorrow.
Related details
- Leszek Swirski
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 3 comments![]( "Open in Gerrit")
TF_BUILTIN(ForOfNextLoadValueEagerDeoptContinuation,Rezvan Mahdavi HezavehDon't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
Marja HölttäBased on my understanding no, please correct me if I am wrong. Eager deopts happen *before* loading the values and lazy deopts happen *after* loading a value. So:
1) LoadDoneEagerDeopt: happens when we do not have the `done` and `value` values.
2) LoadValueEagerDeopt: happens when we have `done` but we do not have `value`.
3) LoadDoneLazyDeopt: happens when we have `done` but we do not have `value`.
4) LoadValueLazyDeopt: happens when we have both `done` and `value` so we do not need this one.
Marja HölttäAh, are you saying that we don't need an explicit continuation in the "value getter lazy deopts" case, but the right thing will happen automatically?
Please add a test for the "value getter lazy deopts" nevertheless.
Marja HölttäYou can use the helper %DeoptimizeFunction for triggering a lazy deopt.
or... I think that's the right function, but please verify 😊
Oh well ... It turned out that you are mright. We do need a `LoadValueLazyDeoptContinuation` to have a working and correct deoptimization. But there are no steps inside it, we just need to return values. I realized that after adding the test you requested here. Thanks for the excellent suggestion!
RETURN_IF_ABORT(BuildCheckJSReceiver(result_object));Rezvan Mahdavi HezavehAnd your test discovered a bug here. It's very subtle, I had to get a lot of help from Leszek for this one 😊
The bug is that (unintuitively?), you have two checks here, CheckJSReceiver and CheckMaps (below), and you're trying to attach two different deopt continuations to them. (The default one, which will do whatever, to CheckJSReceiver, and then your custom one to CheckMaps.)
That doesn't work, since you cannot do that in Maglev (have two consecutive checks with different continuations.)
You'll anyway need to pull your EagerDeoptScope up so that it covers the CheckJSReceiver failing, since you also need your continuation to kick in when the object returned by next() is not a JSReceiver. That should fix this bug too.
However, now the continuation is hit but the test still fails. I didn't debug that yet.
Also, there might be some problems with the "next" continuation, I didn't yet check whether that one is now wired up correctly
Please also add a test for the "CheckJSReceiver failing" case, since you don't seem to have one yet.
Rezvan Mahdavi HezavehThanks for debugging Marja! After moving the EagerDeoptScope up, the test hit the continuation correctly but as you said the result is still incorrect (<11> instead of <32> after applying your suggestion to the test). It looks like the continuation ignores the returned results. Could this be the problem? maybe because we have two returned values from continuations?
My suspicion about ignoring the returned result was somewhat correct. In my discussion with Leszek, I learned that eager continuations store the result in accumulator and our code stub assember code expect to see the result on two registers. SO, we decided to store the results on the registers manually in the eager continuations (and later follow your suggestion on changing the builtins to only have one return value in a follow-up CL). After manually saving the results into resgisters, the eager test now is passing! Please take a look at the code and I will add the additional tests you have requested hopefully tomorrow.
Today I learned that lazy continuations also need manual storing the results in the registers, so I added this for lazy deopt as well. My earlier lazy test was passing because at the time of deoptimization, the code return done:true which was equivalent to ignoring the results of the continuation. That's why I did not realize that sooner. I changed the test too.
The whole CL still have unresolved comments and needs polishing but I feel that the main problems are solved now (hopefully)
// Iter 1: getMap(0) -> maps_weird[0] (value 10). sum = 11.Rezvan Mahdavi Hezaveh11
Done
Related details
- Leszek Swirski
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 2 comments![]( "Open in Gerrit")
{value: 10, done: false}, {value: 32, done: false, extra: 'trigger'},Another missing test: what if there's no "done" property in the result object, what happens then?
(Do you already have a test for that for the lower tiers? If not, those should be added too.)
Good catch. I added tests for it (including in lower tiers).
// Copyright 2026 the V8 project authors. All rights reserved.This test was also missing: eager deopt on value getter. It was really challenging to design a test to hit the right deoptimization continuation, so I used gemini heavily here. Now the test is passing and I can see `Inside ForOfNextLoadValueEagerDeoptContinuation --------` printed in the output so it hits the correct continuation which I guess means that the test and code are correct.
Can someone take a look and confirm?
Related details
- Leszek Swirski
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 8 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
8 comments
// Copyright 2026 the V8 project authors. All rights reserved.This test was also missing: eager deopt on value getter. It was really challenging to design a test to hit the right deoptimization continuation, so I used gemini heavily here. Now the test is passing and I can see `Inside ForOfNextLoadValueEagerDeoptContinuation --------` printed in the output so it hits the correct continuation which I guess means that the test and code are correct.
Can someone take a look and confirm?
The approach for triggering the deopt when reading the value looks solid.
const objA1 = new MapAClass(10, false);Can you make the values differ, so it's possible to get the correct sum only one way? Now you can e.g., get value 20 by 10 + 10 but also if you just count objB_trap which has 20.
With values like this, you might the the correct result even if you're counting the wrong objects (e.g., counting some of them twice and omitting some.)
next() {For an extra safeguard, could you make sure this next() function is never optimized? (Here and in other tests)
// 1. The trap iteration (gracefully handled by your continuation!): 20This should say objB_trap
let deopt_now = false;Could you reset this to false in a more robust way? Now it's sometimes reset but not always, super hard to see what the test is actually doing.
if (deopt_now) {Isn't deopt_now always true when you end up in this function?
let count = 0;Maybe here, reset deopt_now to false?
const result = testForOfDeopt();You're not testing the right result here. %OptimizeMaglevOnNextCall sets a flag so that the function is optimized *before* it's executed the next time. So the testForDeopt() above is the one run with a freshly optimized function, and this run is when the deoptimization has already happened, and you haven't optimized again, so here you're just testing the interpreter.
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
testForOfDeopt();Oh, wait, we're running this without resetting deopt_now to false... so this will just terminate the iteration right away? If this is how this test is supposed to work, fair, you can just assert here that the result is 0.
Marja Hölttä (Gerrit)
Marja Hölttä added 2 comments![]( "Open in Gerrit")
if (deopt_now) return {done: true};I think this test is so confusing since you're using the same variable deopt_now for two purposes: actually deopting, and signalling "terminate the iteration". Separating these concerns should make the test clearer.
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 12 comments![]( "Open in Gerrit")
Thanks for the comments Marja!
Rezvan Mahdavi Hezavehstray change
Done
{value: 10, done: false}, {value: 32, done: false, extra: 'trigger'},Rezvan Mahdavi HezavehAnother missing test: what if there's no "done" property in the result object, what happens then?
(Do you already have a test for that for the lower tiers? If not, those should be added too.)
Good catch. I added tests for it (including in lower tiers).
Done
Can you make the values differ, so it's possible to get the correct sum only one way? Now you can e.g., get value 20 by 10 + 10 but also if you just count objB_trap which has 20.
With values like this, you might the the correct result even if you're counting the wrong objects (e.g., counting some of them twice and omitting some.)
That's a good point. I changed the values.
For an extra safeguard, could you make sure this next() function is never optimized? (Here and in other tests)
Done
// 1. The trap iteration (gracefully handled by your continuation!): 20Rezvan Mahdavi HezavehThis should say objB_trap
Done
Could you reset this to false in a more robust way? Now it's sometimes reset but not always, super hard to see what the test is actually doing.
I moved the reset logic into the [Symbol.iterator] method so every time a for-of loop begins, the flag is reset.
Isn't deopt_now always true when you end up in this function?
Yeah, it was. Thanks for catching that! By the changes I made in Symbol.iterator, now it is false for the first and second call and true for the third call of the getter. Also, the loop is terminated after the third call.
Maybe here, reset deopt_now to false?
Done
I think this test is so confusing since you're using the same variable deopt_now for two purposes: actually deopting, and signalling "terminate the iteration". Separating these concerns should make the test clearer.
I changed the termination condition to be `count`. Now the flag is only a signal for deopting.
Oh, wait, we're running this without resetting deopt_now to false... so this will just terminate the iteration right away? If this is how this test is supposed to work, fair, you can just assert here that the result is 0.
Based on your below comment, I removed this line.
const result = testForOfDeopt();Rezvan Mahdavi HezavehYou're not testing the right result here. %OptimizeMaglevOnNextCall sets a flag so that the function is optimized *before* it's executed the next time. So the testForDeopt() above is the one run with a freshly optimized function, and this run is when the deoptimization has already happened, and you haven't optimized again, so here you're just testing the interpreter.
I removed the `testForOfDeopt()` afetr `%OptimizeMaglevOnNextCall`. I think the test is correct now, can you confirm?
Related details
- Leszek Swirski
- Marja Hölttä
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
2 comments
thanks! still lgtm
const result = testForOfDeopt();Rezvan Mahdavi HezavehYou're not testing the right result here. %OptimizeMaglevOnNextCall sets a flag so that the function is optimized *before* it's executed the next time. So the testForDeopt() above is the one run with a freshly optimized function, and this run is when the deoptimization has already happened, and you haven't optimized again, so here you're just testing the interpreter.
I removed the `testForOfDeopt()` afetr `%OptimizeMaglevOnNextCall`. I think the test is correct now, can you confirm?
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
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")
Leszek is OOO, Victor, could you OWNERS-review the Maglev parts?
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
- Rezvan Mahdavi Hezaveh
- Victor Gomes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Victor Gomes voted and added 4 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Code-Review | +1 |
4 comments
LGTM + some suggestion for more idiomatic Maglev.
if (done_result.IsDoneWithAbort()) return done_result;```
RETURN_IF_ABORT(done_result);
```
MaybeReduceResult result = TryBuildLoadNamedProperty(
result_object, broker()->value_string(), value_feedback);
if (result.IsDone()) return result.Checked();```
RETURN_IF_DONE(TryBuildLoadNamedProperty(
result_object, broker()->value_string(), value_feedback));
```
MaybeReduceResult result =
TryReduceForOfNext(iterator, next_method, register_pair, call_slot);
if (!result.IsFail()) {
return result.Checked();
}
Maybe simply:
```
RETURN_IF_DONE(TryReduceForOfNext(iterator, next_method, register_pair, call_slot));
```
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 6 comments![]( "Open in Gerrit")
Thanks for the reviews!
Marja, please take a look at unresolved comments. I kept them unresolved to make sure you will see my responses. If you don't see any other problem, we can land this CL. When I run the perf test locally, the numbers are worst compared to before adding deoptimization and inlining. I may have missed something. But this is an old CL, let's land it and I figure the problem out in a follow-up CL.
Print("Inside ForOfNextLoadDoneLazyDeoptContinuation --------");Rezvan Mahdavi HezavehKeeping the prints for now to debug the code, I'll remove them before landing the CL.
Acknowledged
if (done_result.IsDoneWithAbort()) return done_result;Rezvan Mahdavi Hezaveh```
RETURN_IF_ABORT(done_result);
```
Done
MaybeReduceResult result = TryBuildLoadNamedProperty(
result_object, broker()->value_string(), value_feedback);
if (result.IsDone()) return result.Checked();```
RETURN_IF_DONE(TryBuildLoadNamedProperty(
result_object, broker()->value_string(), value_feedback));
```
Done
MaybeReduceResult result =
TryReduceForOfNext(iterator, next_method, register_pair, call_slot);
if (!result.IsFail()) {
return result.Checked();
}
Maybe simply:
```
RETURN_IF_DONE(TryReduceForOfNext(iterator, next_method, register_pair, call_slot));
```
Done
// 3. Trigger Eager Deopt on the SECOND iteration of a single call
// Iter 1: getMap() -> maps[0] (value 10). sum = 10.
// Iter 2: getMap() -> maps[1] (value 32). CHECKMAPS FAILS -> DEOPT.
// Interpreter resumes Iter 2: adds 32. sum = 42.Rezvan Mahdavi HezavehFor both tests I kept the generated comments to make the test readable. I can remove them if they look unnecessary to you.
Acknowledged
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marja Hölttä
| Code-Review | +1 |
2 comments
TF_BUILTIN(ForOfNextLoadValueEagerDeoptContinuation,Don't we need a LoadValueLazyDeoptContinuation too, since the value getter might lazy deopt?
(I also noticed there are no tests for that case..)
Based on my understanding no, please correct me if I am wrong. Eager deopts happen *before* loading the values and lazy deopts happen *after* loading a value. So:
1) LoadDoneEagerDeopt: happens when we do not have the `done` and `value` values.
2) LoadValueEagerDeopt: happens when we have `done` but we do not have `value`.
3) LoadDoneLazyDeopt: happens when we have `done` but we do not have `value`.
4) LoadValueLazyDeopt: happens when we have both `done` and `value` so we do not need this one.
Marja HölttäAh, are you saying that we don't need an explicit continuation in the "value getter lazy deopts" case, but the right thing will happen automatically?
Please add a test for the "value getter lazy deopts" nevertheless.
Marja HölttäYou can use the helper %DeoptimizeFunction for triggering a lazy deopt.
Rezvan Mahdavi Hezavehor... I think that's the right function, but please verify 😊
Oh well ... It turned out that you are mright. We do need a `LoadValueLazyDeoptContinuation` to have a working and correct deoptimization. But there are no steps inside it, we just need to return values. I realized that after adding the test you requested here. Thanks for the excellent suggestion!
Acknowledged
RETURN_IF_ABORT(BuildCheckJSReceiver(result_object));Rezvan Mahdavi HezavehAnd your test discovered a bug here. It's very subtle, I had to get a lot of help from Leszek for this one 😊
The bug is that (unintuitively?), you have two checks here, CheckJSReceiver and CheckMaps (below), and you're trying to attach two different deopt continuations to them. (The default one, which will do whatever, to CheckJSReceiver, and then your custom one to CheckMaps.)
That doesn't work, since you cannot do that in Maglev (have two consecutive checks with different continuations.)
You'll anyway need to pull your EagerDeoptScope up so that it covers the CheckJSReceiver failing, since you also need your continuation to kick in when the object returned by next() is not a JSReceiver. That should fix this bug too.
However, now the continuation is hit but the test still fails. I didn't debug that yet.
Also, there might be some problems with the "next" continuation, I didn't yet check whether that one is now wired up correctly
Please also add a test for the "CheckJSReceiver failing" case, since you don't seem to have one yet.
Rezvan Mahdavi HezavehThanks for debugging Marja! After moving the EagerDeoptScope up, the test hit the continuation correctly but as you said the result is still incorrect (<11> instead of <32> after applying your suggestion to the test). It looks like the continuation ignores the returned results. Could this be the problem? maybe because we have two returned values from continuations?
Rezvan Mahdavi HezavehMy suspicion about ignoring the returned result was somewhat correct. In my discussion with Leszek, I learned that eager continuations store the result in accumulator and our code stub assember code expect to see the result on two registers. SO, we decided to store the results on the registers manually in the eager continuations (and later follow your suggestion on changing the builtins to only have one return value in a follow-up CL). After manually saving the results into resgisters, the eager test now is passing! Please take a look at the code and I will add the additional tests you have requested hopefully tomorrow.
Today I learned that lazy continuations also need manual storing the results in the registers, so I added this for lazy deopt as well. My earlier lazy test was passing because at the time of deoptimization, the code return done:true which was equivalent to ignoring the results of the continuation. That's why I did not realize that sooner. I changed the test too.
The whole CL still have unresolved comments and needs polishing but I feel that the main problems are solved now (hopefully)
Acknowledged
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Leszek Swirski
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[for-of-performance] Add inlining
This CL inlines for-of loop on array iterator with deopt support. This
change introduces several deoptimization continuations:
- ForOfNextLoadDone{Lazy,Eager}DeoptContinuation
- ForOfNextLoadValue{Lazy,Eager}DeoptContinuation
These builtins allow the compiler to handle cases where loading the
'done' or 'value' properties from an iterator result object triggers a
deoptimization (e.g., due to a map change or a side-effecting getter).
The Maglev graph builder now attempts to reduce ForOfNext by
speculatively inlining the .next() call and the subsequent property
accesses, falling back to the generic builtin only when necessary.
- M src/builtins/builtins-definitions.h
- M src/builtins/builtins-iterator-gen.cc
- M src/builtins/builtins-iterator-gen.h
- M src/codegen/interface-descriptors-inl.h
- M src/codegen/interface-descriptors.h
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-graph-builder.h
- M src/maglev/maglev-ir.h
- M test/mjsunit/es6/for-of-array-iterator-optimization-baseline.js
- M test/mjsunit/es6/for-of-array-iterator-optimization-ignition.js
- A test/mjsunit/es6/for-of-array-iterator-optimization-maglev-eager-check-missing-done.js
- A test/mjsunit/es6/for-of-array-iterator-optimization-maglev-eager-check.js
- A test/mjsunit/es6/for-of-array-iterator-optimization-maglev-eager-value-check.js
- A test/mjsunit/es6/for-of-array-iterator-optimization-maglev-lazy-check.js
- A test/mjsunit/es6/for-of-array-iterator-optimization-maglev-lazy-value-check.js
- M test/mjsunit/es6/for-of-array-iterator-optimization-maglev.js
- M test/mjsunit/es6/for-of-array-iterator-optimization-turbofan.js
Code-Review: +1 by Marja Hölttä, +1 by Victor Gomes
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |