[tiering] Don't reset to unoptimized code when a TurboFan job is aborted [v8/v8 : main]
Pan, Tao (Gerrit)
Pan, Tao added 1 comment![]( "Open in Gerrit")
function->UpdateCode(isolate, shared->GetCode(isolate));Pan, TaoI think the fix here is to just remove the `UpdateCode` line. Seems like that would not be needed here, as long as the maglev code itself is still valid (and if not it is deopted independently).
Olivier FlückigerThe function‘s code was set the Builtin
Pan, TaoIn general, the reason for aborting concurrent turbofan compilation can also cause the executing maglev code to become invalid. In the example, marking maglev code for deoptimization and aborting turbofan compilation occur successively.
Of course, but then if we don't update code, maglev deopts normally which will reset the budget.
When starting turbofan compilation, the function's code is set to Code BUILTIN InterpreterEntryTrampoline.
I don't think that is true. Where would that be?
When aborting the turbofan compilation, if the function's code is not replaced with other code, the next invocation will still start turbofan compilation.
No, we still reset the tiering request.
At this point, there is no pointer pointing to the maglev code, so it is not possible to directly replace the function's code with the maglev code. A complex approach is to cache a pointer to the maglev code at the beginning of turbofan compilation, and use this cached maglev code pointer (if the maglev code is still valid) to update the function's code when aborting the turbofan compilation, and then tier up to turbofan normally.
Sorry I am not following. What you are describing is not how it is supposed to work. If that is true it's a bug.
Olivier FlückigerI'm very sorry. There were some errors in my previous description.
In the example I provided, the Code BUILTIN InterpreterEntryTrampoline is set in https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/code.cc;drc=4a3d24cd677fe4fbdc643c3ad3d100dcdf2d3fa8;l=182 when the first line below occurs. When the second line below occurs, if the function's code not changed, the interrupt budget will be set to ```v8_flags.invocation_count_for_turbofan * bytecode_length``` and there will be no deopt because the maglev code has already expired when the first line below occurs. The issue (using turbofan budget to re-tier to maglev) still exists.
````
[marking dependent code 0x03ea010878e5 <Code MAGLEV> (0x2bba011c9331 <SharedFunctionInfo __webpack_modules__../node_modules/acorn/dist/acorn.mjs.pp$5.parseExprSubscripts>) (opt id 65) for deoptimization, reason: dependent prototype chain changed]
[aborted optimizing 0x2bba011da609 <JSFunction __webpack_modules__../node_modules/acorn/dist/acorn.mjs.pp$5.parseExprSubscripts (sfi = 0x2bba011c9331)> (target TURBOFAN_JS) because: Bailed out due to dependency change - took 0.004, 5.096, 0.018 ms invocation_count: 665]
````
Pan, TaoSorry I really don't understand the argument.
If the builtin is set to IET (according to the place you linked), then SetTieringInProgress resets the budget to the Maglev budget, regardless if its before or after the UpdateCode.
Olivier FlückigerPlease have a look at related code. After [the builtin is set to IET](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/code.cc;drc=4a3d24cd677fe4fbdc643c3ad3d100dcdf2d3fa8;l=182), SetTieringRequest in any cases (line 206 and 214). After aborting turbofan compilation, if not replacing function's code, both [isolate->js_dispatch_table().IsTieringRequested(dispatch_handle()](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-function-inl.h;drc=b251030f22d6810f6755262d2182316f60552d60;l=276) and [function->IsTieringRequestedOrInProgress(isolate)](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/execution/tiering-manager.cc;drc=31f78e6f02609926458a75b38eb88ab3ce51d12e;l=198) return true, so [return v8_flags.invocation_count_for_turbofan * bytecode_length;](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/execution/tiering-manager.cc;drc=31f78e6f02609926458a75b38eb88ab3ce51d12e;l=222).
Pan, TaoI think you are confused. There are two things:
1. the code object
2. the entrypointif we deopt lazy we set the entrypoint to MarkLazyDeoptimized, which will reset the budget cleanly.
So I don't understand your explanation. And if it were true, then IsTieringRequested should be fixed to not return true if no tiering is requested.
Olivier FlückigerSimilar with https://chromium-review.googlesource.com/c/v8/v8/+/7789650, the root cause is that the tiering request is cleared by ```JSFunction::UpdateCode```. Change ```JSFunction::UpdateCode``` to ```JSFunction::UpdateCodeKeepTieringRequests``` can ensure resetting the budget. Could you please review the new implementation?
Pan, TaoRight, and I have been arguing for several iterations now that the whole UpdateCode is not needed here at all. And I still don't understand why you think it is needed.
I'm very sorry. Previously, I misjudged that the interrupt budget was not reset after removing ```function->UpdateCode(isolate, shared->GetCode(isolate));``` because I did not see ```JSFunction::SetInterruptBudget``` executed between ```Code::SetMarkedForDeoptimization``` and ```TieringManager::Optimize```. In fact, [the interrupt budget has been directly reset to 1](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/runtime/runtime-compiler.cc;drc=66fc85730249a379d5b8e1add0fe4d118d38d009;l=368). I removed the whole UpdateCode just now. Could you please have a look?
Related details
- Olivier Flückiger
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger voted and added 1 comment![]( "Open in Gerrit")
Votes added by Olivier Flückiger
| Code-Review | +1 |
1 comment
Related details
- Darius Mercadier
- Olivier Flückiger
- Pan, Tao
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17ef1f3a290000
Related details
- Darius Mercadier
- Pan, Tao
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1044c64b290000