Closed Bug 1538375 Opened 3 years ago Closed 2 years ago

Don't deoptimize for debugger statement

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
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...

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.

Marking P1 because this seems like it's relatively low-hanging perf fruit and is known to affect real-world websites.

Priority: -- → P1

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).

Blocks: 1540101

Steven is this something we might aim to fix in 69? It's been around a while with no one assigned.

Flags: needinfo?(sdetar)
Whiteboard: [MemShrink:P1]

The main task here is fixing dozens of devtools tests that are affected by this.

Assignee: nobody → khyperia
Status: NEW → ASSIGNED
Flags: needinfo?(sdetar)
Pushed by ahauck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e84effab739
Don't deopt on debugger statements. r=jorendorff
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Jason, is there any risk that we will have more optimized away variables?

Flags: needinfo?(jorendorff)

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.

Flags: needinfo?(jorendorff)
Blocks: 1122908
You need to log in before you can comment on or make changes to this bug.