638210 - JM: Remove the need for compartment/runtime-wide debug mode

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

The type inference branch (JaegerMonkey repo) has most or all of what's needed here. The goal state is:

- We can recompile a running script from normal mode to debug mode.
- It's not necessary to turn on debug mode for a compartment before debugging. Rather, scripts are recompiled in debug mode as needed.

This will make debuggers more reliable and should also greatly reduce the performance impact of having Firebug installed/on.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

bug 638680 removes "uncached calls" which exist solely for debug mode; there is now one mechanism for all calls, and debug mode simply disables updating of ICs.

Now, there are a few tricky problems to removing debug mode, which have been solved one way or another in the Type Inference branch:
 (1) When recompiling a script, all cached calls to that script must be
     invalidated.
 (2) When destroying a call IC, if it's calling a native, it may be active on
     the stack, in which case it will return to the IC. Care must be taken to
     fix its return address, which is tricky since it's not a stub call but a
     C++ call.
 (3) Resolve hooks, getters, etc could trigger recompilation while inside an
     IC's C++ code, destroying the IC info and crashing, similar to how the
     TraceRecorder can be killed randomly. This is already a problem.

For (1), this seems pretty straightforward, we can thread a list of JITScript/IC pairs through ICs and store the head in JITScript.

For (2), Brian has a wonderful hack (currently x86 only) that patches the C++ return address. We should be able to make this work on x64; ARM is unclear, and may need a trampoline or an extra store.

For (3), there's two routes I'm considering. We could do the TR thing and guess at each point that re-enters the VM, save enough local state to check for recompilation, and then bail out after if it happened. I kind of don't like that because it's considerably gross and it's not always clear what can re-enter, so basically it's a whack-a-mole game (which has been our experience with the recorder).

Another option is to leave ICs intact on recompilation, either reusing the JITScript memory (the allocation size won't change, yet), or by just not actually throwing out the old JITScript until a GC determines it's safe. The hard part there is removing VMFrame::jit(); it's safe for a stale IC to attach code to a stale JITScript, but not to a live one.

Comment 2

[David Mandelin [:dmandelin]]

(In reply to comment #1)
> For (3), there's two routes I'm considering. We could do the TR thing and guess
> at each point that re-enters the VM, save enough local state to check for
> recompilation, and then bail out after if it happened. I kind of don't like
> that because it's considerably gross and it's not always clear what can
> re-enter, so basically it's a whack-a-mole game (which has been our experience
> with the recorder).
>
> Another option is to leave ICs intact on recompilation, either reusing the
> JITScript memory (the allocation size won't change, yet), or by just not
> actually throwing out the old JITScript until a GC determines it's safe. The
> hard part there is removing VMFrame::jit(); it's safe for a stale IC to attach
> code to a stale JITScript, but not to a live one.

It seems to me that it would be nice to refcount the ICs, as is normally done for a structure that can be used at multiple places that drop refs at arbitrary times. The C++ IC stubs would hold a ref using an RAII class--maybe we can even set it up so that they can't even get the IC struct without creating an instance of the RAII class. When a script is recompiled, it would be disconnected from its IC structs, which would then have a 'dead' flag set, possibly other fields reset, and their ref from the script dropped. But the C++ still holds a ref, so it's not cleaned up yet.

At that point, it should be fairly clean to do either method: check the 'dead' flag after coming back from a VM call and before touching the IC data; or continue generating IC code, with that process driven off the IC in such a way that it's harmless.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Another problem: when recompiling for EvalInFrame(), we need to deoptimize the whole script, breaking constant and copy propagation potentially mid-expression. But when this happens, constants and copies will no longer be synced. A few solutions:

(1) Rely on compiler determinism, but going forward, as we want to recompile more aggressively to speculate on types and inline functions, that seems undesirable.

(2) Always sync constants/copies before making fallible C++ calls.

(3) Save constant/copy information at call sites (points at which recompilation can happen), which is an option if (2) costs too much.

For either (2) or (3), we need to change the FrameState. It only looks at the top of the stack for syncing, and searching the whole stack would regress bug 591836. The simplest thing is add another tracker for slots which have pending syncs, so I'll try that.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: sync using the tracker

This regresses bug 591836 from ~9ms to ~15ms. It always uses the tracker to iterate over frame slots.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2: minimize copies

This cuts the regression down to 13ms, but it's kind of unrelated. It makes the "isCopied" bit on frame entries always accurate, so we don't have to walk an entire frame when invalidating backing stores.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3: sync the entire frame, not just stack uses

This doesn't seem to do anything to sunspider (maybe 1ms worse), but is like 100 points off v8, especially on crypto.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

I'm going to stop here for now. The JM architecture isn't great for fixing this, and given that we're planning a better one, spending any more time getting this queue solid and dealing with perf regressions doesn't seem to have any real benefit.

On the upside, we pretty much know exactly what we need to make this work, so addressing the shortcomings of FrameState will be an essential criteria for its next iteration.

I'm leaving this bug open because we *definitely* want the functionality, and don't have it yet (save for TI, which exposed FrameState problems). But, recompilation of (1) optimized code for (2) EvalInFrame will be designed into the next compiler up-front, rather than as an afterthought.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Mandelin from comment #0)
> The type inference branch (JaegerMonkey repo) has most or all of what's
> needed here.

Does that mean this bug is no longer necessary?

Comment 9

[Till Schneidereit [:till]]

JM is gone. Don't know how we're dealing with this in IM, but I'm sure we are, or are tracking that work somewhere else.