Don't deoptimize for debugger statement
Categories
(Core :: JavaScript Engine, enhancement, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: jorendorff, Assigned: khyperia)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [MemShrink:P1])
Attachments
(1 file)
We currently deoptimize pretty hard when we see a debugger; statement. It's about as bad as direct eval. Bug 1537609 indicates that this is a real performance problem.
There's no compelling reason for us to do it, either. It's not like a Web developer using the Debugger will always, or even usually, have a debugger statement in the function they want to debug.
If I tear it out, we fail 10 tests under js/src/jit-test/tests/debug. (All other JS shell tests still pass.) Pretty sure I can make all those pass again just by adding eval(""); nearby. Or, you know, I could actually look at them and debug some stuff...
|
||
Comment 1•5 years ago
•
|
||
It seems like de-optimizing functions that contain debugger doesn't fly any more.
If the code obfuscators that pepper the code with debugger statements are widely used, this de-optimization could be a big problem.
When a web page is reloaded under the debugger, we should always have full details available for the user. In the case where we've opened the debugger in a page that is already running, I believe our principle has been to do the best we can without affecting the performance of the ~100% of cases where someone doesn't open the devtools.
|
||
Comment 2•5 years ago
|
||
Marking P1 because this seems like it's relatively low-hanging perf fruit and is known to affect real-world websites.
|
||
Comment 3•5 years ago
|
||
When a debugger statement is present, setBindingsAccessedDynamically is called and because the SharedContext::bindingsAccessedDynamically_ flag is propagated from inner to outer scripts, all surrounding scripts will also have bindingsAccessedDynamically_ set. This in turn means, all surrounding scripts will also always create arguments objects and also request this bindings.
This means for bug 1540101, when the r[0] function (where r[0] is UnityModule and r is the Promise.all result array) is called with r[0](ModuleImports), UnityModule will request a this binding in which the r array is stored (which in addition to the UnityModule also contains two large ArrayBuffers), which will then lead to the observed memory leak (when compared to Chrome, see bug 1540101, comment #1).
|
||
Comment 4•5 years ago
|
||
Steven is this something we might aim to fix in 69? It's been around a while with no one assigned.
|
Reporter | |
Comment 5•5 years ago
|
||
The main task here is fixing dozens of devtools tests that are affected by this.
|
Assignee | |
Comment 6•5 years ago
|
||
|
||
Updated•5 years ago
|
Pushed by ahauck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e84effab739 Don't deopt on debugger statements. r=jorendorff
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
|
||
Comment 9•5 years ago
|
||
Jason, is there any risk that we will have more optimized away variables?
|
|
Reporter | |
Comment 10•5 years ago
|
||
Yes, definitely, but the effect is basically the same as using a breakpoint instead of a debugger statement.
If you notice a regression in behavior, let me know.
I don't know if you remember, but I did mention this when it landed, and asked if anyone had time to check that the behavior is still acceptable -- not that I blame y'all for having other priorities, then or now.
Description
•