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)
(deleted),
text/x-phabricator-request
|
Details |
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•6 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•6 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•6 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).
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
|
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
•