1715444 - Do not emit code and metadata for breakpoints that are at the same location as the previous one

Status: RESOLVED WONTFIX

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Lars T Hansen [:lth]]

See bug 1714086 for a full analysis and test case.

The representation of breakpoints in baseline-compiled debug-enabled code is currently a 10-byte call (load tls instruction + patchable CALL instruction). This gives rise to call metadata + a stack map, and the stack map creation is forced in debug mode; it is never filtered by other mechanisms. Altogether, this amounts to at least 50 bytes per breakpoint site. For large programs, I've seen this amount to 1GB of data and a lot of time spent in compilation, even on very fast systems (19s on my Xeon workstation for the TC in question).

In addition, debug mode forces a sync between every two instructions to stabilize the stack for the breakpoint, further bloating code.

In the baseline compiler, many bytecode instructions don't give rise to any machine code due to various optimizations. For example, DROP and x.CONST only affect the compiler's internal stack, no code is emitted for these. We can reduce the overhead of debug information by about 40% by not emitting breakpoints that are redundant because they happen at the same site as another breakpoint.

This is actually not completely benign. Consider code like this:

   i32.add
   drop
   i32.sub

With breakpoints, it looks like this:

   breakpoint 0 for i32.add
   i32.add
   breakpoint 1 for drop
   drop
   breakpoint 2 for i32.sub
   i32.sub

If we just don't emit code for breakpoint 2 because it would end up at the code location for breakpoint 1 and is hence redundant, the debugging experience will be that the debugger will stop at the DROP, and then a single step would take it past the I32.SUB. We could perhaps change that so that it would instead stop at the I32.SUB, skipping over the DROP. Neither solution has perfect usability.

Comment 1

[bjorn3]

Would it be possible to allow multiple user facing breakpoints at the same address? So if breakpoint 1 is activated and breakpoint 2 isn't, when a breakpoint is hit, breakpoint 1 would be reported as current location and vice versa if breakpoint 2 is enabled. Of both are enabled single stepping from breakpoint 1 would do nothing except for changing the reported breakpoint to breakpoint 2. Or would it be hard to remove the 1 to 1 mapping between addresses and breakpoint locations?

Alternatively you could put the breakpoint at I32.SUB as you suggested and at the same time not show DROP as a breakpointable location in the wasm source view just like non-code lines. That should make it less confusing to the user why they can't put a breakpoint where they expect they can.

Comment 2

[Lars T Hansen [:lth]]

After prototyping this it's clear that a naive implementation can be pretty confusing to use. Consider

       (func (export "f") (param i32) (result i32)
         (local $x i32)
         local.get 0
         i32.const 2
         i32.mul
         local.set $x
         local.get $x))`))

where the breakpoint locations are on the local.get 0 and then not until the local.set $x because one is emitted for the local.get (it's the first instruction in the function) but that generates no code, nor is there code for the i32.const, and so a breakpoint is not emitted for the i32.const or the i32.mul, both having the same code buffer location as the first local.get. In a local sense, as discussed above, it would be significantly less confusing if the lone breakpoint were emitted for the i32.mul.

But then, a breakpoint is emitted for the local.set (because i32.mul generated some code) and then again for the final local.get (because the local.set generated code), and so it additionally feels randomly inconsistent that the final local.get gets a breakpoint while (with the proposed new breakpoint scheme) the first local.get would not.

If Firefox is going to have a wasm-aware debugger at all then it should be as usable as it can be (and we're already compromised because there's no way to view the wasm evaluation stack), so I will want to rethink this. The most predictable solution is probably if we consistently don't emit breakpoints for some common but generally effect-free instructions such as local.get, global.get, X.const, and drop. Here that would have been OK because there is a breakpoint at the end of the function regardless. It will be useful to see what kind of memory savings that might lead to.

Comment 3

[Lars T Hansen [:lth]]

One final experiment. If I don't generate breakpoints for DROP, NOP, and x.CONST, then memory use drops by 13% and time by 19% on this test case, relative to always emitting a breakpoint. This is significant, but the fact remains that this may make debugging surprising, and really the bug is elsewhere. Other methods for reducing debug memory pressure should be tried before we take this any further.

Comment 4

[Lars T Hansen [:lth]]

Attachment: Patch to omit breakpoint info for uninteresting opcodes