1032869 - Debugger should only invalidate code when required for instrumentation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently a Debugger object invalidate all JIT after registering it-self[1] for discarding everything[2].

What we want is to be able to recompile the JITs only when we need to handle a Debugger which implies that the compiled code is in a specific mode.  Currently, the Debugger needs to invalidate code such as we can add Breakpoints and monitor every time we enter/leave a frame.

One option would be to do it in a similar way as the DebugNeedDelazification flag, and have an ensure function[3] which does the delazification if the flag is set.

This way, as long as any debugger instance maintains a breakpoint / onEnterFrame callback / … on the monitored compartment, we can just ensure that we discarded All the Jit code ahead and that we are not producing any JIT code which does not fit the requirements of the Debugger.  This means that in addition of doing the transition, we would also have to ensure that all new compilations are debugger-compliant.  As soon as all debugger instances are no longer using any of these watcher, we can potentially discard all JIT code to recompile without instrumentation.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jscompartment.cpp#844
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.cpp#3132
[3] http://dxr.mozilla.org/mozilla-central/source/js/src/jscompartment.cpp?from=ensureDelazifyScriptsForDebugMode#792

Comment 1

[Shu-yu Guo [:shu]]

Yeah, I've wanted this for a while but I haven't had any cycles to work on it.

There are three classes of hooks:

 1. Hooks that inhibit Ion for the whole compartment, because Ion cannot observe them. This is stuff like onEnterFrame, onExceptionUnwind, etc, which observes every frame.
 2. Hooks that inhibit Ion a particular on-stack frame. This is onStep.
 3. Hooks that don't inhibit Ion at all, like, say, just observing locals on the frame. This has some issues surrounding recover instructions, but we can always treat recover instructions as <optimized out> values.

Your approach sounds good to me -- what I had in mind was basically have more flags like you envisioned, which gets set/unset when any type 1 hooks are set. This would trigger an invalidation of all JIT code in the compartment and a debug mode OSR. Ion itself would then check for something like |if (hasAnyLiveType1DebuggerHooks())| before attempting to compile.

Type 2 hooks should only trigger local invalidation of the on-stack frame.

Type 3 hooks don't need to do anything.

It'll be great if someone can get to it before I can, but if not, I plan to work on it in a few months, after the internship season is over.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Add accessor to check for debug mode with callbacks.

(WIP: not asking for review yet)

This patch adds a different accessor which currently just wrap the
debugMode() function, but is intented to be used as a sub-set of the
debugMode flag dedicated to JIT recompilation.

Comment 3

[Nicolas B. Pierron [:nbp]]

From what I understand, it sounds terrible to recompile every time we remove the last breakpoint, as this would imply that if somebody set a breakpoint at one location, then remove it to set it at another location instead, then we would have 2 recompilations, for (debug jit on) -> (debug jit off) -> (debug jit on).

This think it might make more sense to just discard all compilations as soon as one breakpoint got added, and only reconsider this question for new compartments or when all debuggers are removed (as it is today).

Shu, jimb, what do you think?

Comment 4

[Jim Blandy :jimb]

Yes, having code flop back and forth between compiled and uncompiled forms because of breakpoint activity seems bad.

My understanding is that it's easy to set breakpoints in interpreted and baseline scripts, and hard in Ion scripts. So here's one idea:

- When setting a breakpoint in an Ion-compiled script, throw away the Ion code and let the script drop back to the interpreter or baseline, whatever's easiest.

- When setting or removing a breakpoint on an interpreted or baseline-compiled script, reset the counters that decide whether to jit the script, so that breakpoint activity postpones jitting.

Then, we'll only JIT a script when it has been executed several times (the usual warm-up period) without Debugger doing anything with breakpoints. And obviously, we'll disable Ion-jitting entirely while there are breakpoints.

In other words, we'll just use the usual warm-up period for hysteresis.

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #4)

Jim's idea sounds pretty good to me. I'd do that.

> - When setting a breakpoint in an Ion-compiled script, throw away the Ion
> code and let the script drop back to the interpreter or baseline, whatever's
> easiest.

Just as a FYI, right now, when getting a Debugger.Frame on an Ion frame, this would require rematerialization and thus invalidation right now. This behavior would need to change.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8449463 [details] [diff] [review]
part 1 - Add accessor to check for debug mode with callbacks.

Review of attachment 8449463 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineJIT.cpp
@@ +84,5 @@
>  
>  static bool
>  IsJSDEnabled(JSContext *cx)
>  {
> +    return cx->compartment()->debugModeHasCallbacks() && cx->runtime()->debugHooks.callHook;

Hasn't JSD been removed?

Comment 7

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #6)
> Comment on attachment 8449463 [details] [diff] [review]
> part 1 - Add accessor to check for debug mode with callbacks.
> 
> Review of attachment 8449463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/BaselineJIT.cpp
> @@ +84,5 @@
> >  
> >  static bool
> >  IsJSDEnabled(JSContext *cx)
> >  {
> > +    return cx->compartment()->debugModeHasCallbacks() && cx->runtime()->debugHooks.callHook;
> 
> Hasn't JSD been removed?

This function will be deleted by bug 1031881, which is awaiting review from sfink.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Disable Ion when hooks are added.

(WIP: not asking for review yet)

This patch focus on how to disable Ion execution when the debuggers are
requesting instrumentations capabilities which are not yet handled by
IonMonkey.

This patch does not re-enable Ion as soon as the hooks are removed, but it
does not disable Ion on new globals if all hooks got removed since.

Comment 9

[Nicolas B. Pierron [:nbp]]

(WIP: https://tbpl.mozilla.org/?tree=Try&rev=448a6c706e25)

Comment 10

[Jim Blandy :jimb]

Hi, Nicolas! How is this going?

fitzgen and I have been working on memory tool support in Debugger, and more and more people are getting interested in them. However, it's disappointing when I have to say, "... but simply by turning on Debugger, you disable Ion and get a 4x perf hit."

If I understand correctly, the patches in this bug would let us make a compartment a debuggee without affecting code generation, as long as we don't use invasive hooks like onEnterFrame, onPop, onDebuggerStatement, and so on. A Debugger-based memory analysis tool would need none of those expensive things --- only the object metadata callback. So the patches here would make Debugger much, much more attractive to a number of groups across the organization.

So: what are your plans with regards to this bug? Do you think you'll be able to land it soon? Will it do what we need?

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Jim Blandy :jimb from comment #10)
> So: what are your plans with regards to this bug? Do you think you'll be
> able to land it soon? Will it do what we need?

I am not actively working on it at the moment.  If somebody want to continue to work on these patches feel free to do so.

I will check if the current patches are still working …

Comment 12

[Nicolas B. Pierron [:nbp]]

FYI, I managed to rebase my changes on top of mozilla-central, after a few minor conflicts.

If they work on Try, and you are fine with reviewing these patches without the part for re-enabling Ion, then I can land them in the current state.

remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=d2aab22f7a28
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d2aab22f7a28

Comment 13

[Jim Blandy :jimb]

It looks like they need some further work. I'll try to get some time to pick this up in the next few weeks.

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #13)
> It looks like they need some further work. I'll try to get some time to pick
> this up in the next few weeks.

I can take this.

Comment 15

[Shu-yu Guo [:shu]]

Attachment: Part 1: Rename isDebuggerFrame to isDebuggerEvalFrame.

Comment 16

[Shu-yu Guo [:shu]]

Attachment: Part 2: Mark frames as debuggee frames instead of entire compartments.

Comment 17

[Shu-yu Guo [:shu]]

Attachment: Part 3: WIP new iteration API

Since to be more precise with debug mode toggles I'm pushing the concept of
'debug mode' into frames (see part 2), during toggles that involve many
debuggee globals, the current way of iterating to invalidate scripts and doing
OSR is very inefficient. Currently, we do a whole zone walk for JSScripts for
every compartment we're toggling debug mode for.

This new iteration API introduces a matcher struct 'ExecutionObservableSet'
which matches at two granularities:

1. A set of >= 1 compartment(s)
   For hooks like onEnterFrame.

2. A particular script.
   For setting breakpoints.

I opted to not do a per-frame granularity since that would require
recompilation of a script's JIT code, and thus invalidation of all its frames
anyways. If per-frame is important for perf we can investigate later.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Part 2: Mark frames as debuggee frames instead of entire compartments.

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 3: Selectively deoptimize when Debugger needs to observe execution.

Passes all debug jit-tests. Waiting on you for writing some actual tests.

Comment 20

[Shu-yu Guo [:shu]]

Attachment: Part 3: Selectively deoptimize when Debugger needs to observe execution.

Fix the shell's evalInFrame.

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 3: Selectively deoptimize when Debugger needs to observe execution.

I couldn't separate out the JIT parts from the Debugger parts, everything's
kind of intermingled. So, I'd like 2 sets of eyes on it.

Comment 22

[Jim Blandy :jimb]

Comment on attachment 8479507 [details] [diff] [review]
Part 1: Rename isDebuggerFrame to isDebuggerEvalFrame.

Review of attachment 8479507 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for separating this out.

Comment 23

[Jim Blandy :jimb]

Comment on attachment 8481722 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8481722 [details] [diff] [review]:
-----------------------------------------------------------------

I think this needs some a comment somewhere (jscompartment.h, jsscript.h, Stack.h) explaining the invariants we're trying to preserve and what they get us.

For example, I can infer from JSScript::debugMode that setting the 'observes everything' flag makes all scripts say they're in debug mode. But what does that mean, extensionally? I might guess that if S->debugMode(), then any JIT code for S has been compiled with a debug prologue, a debug epilogue, and block scope maintenance code present. And surely single-stepping requires debug mode code. But does a breakpoint? Does a debugger; statement? These things should be summarized somewhere.

Does InterpreterFrame::DEBUGGEE mean anything other than frame->script()->debugMode()?

And so on.

Comment 24

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #23)
> For example, I can infer from JSScript::debugMode that setting the 'observes
> everything' flag makes all scripts say they're in debug mode. But what does

I meant, JSScript::isDebuggee, not debugMode.

Comment 25

[Jim Blandy :jimb]

Comment on attachment 8481722 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8481722 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscompartment.h
@@ +294,5 @@
>  
>    private:
>      enum {
>          DebugMode = 1 << 0,
> +        DebugObservesAllExecution = 1 << 1,

For example, here we might say, for debugMode:

   Set if any Debugger has this compartment's global as a debuggee.

and for DebugObservesAllExecution:

   Set if any Debugger observing this compartment's global has hooks set
   that require all code in the compartment to be compiled with debugging support.

Or something.

Comment 26

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #23)
> Comment on attachment 8481722 [details] [diff] [review]
> Part 2: Mark frames as debuggee frames instead of entire compartments.
> 
> Review of attachment 8481722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this needs some a comment somewhere (jscompartment.h, jsscript.h,
> Stack.h) explaining the invariants we're trying to preserve and what they
> get us.
> 
> For example, I can infer from JSScript::debugMode that setting the 'observes
> everything' flag makes all scripts say they're in debug mode. But what does
> that mean, extensionally? I might guess that if S->debugMode(), then any JIT
> code for S has been compiled with a debug prologue, a debug epilogue, and
> block scope maintenance code present. And surely single-stepping requires
> debug mode code. But does a breakpoint? Does a debugger; statement? These
> things should be summarized somewhere.
> 
> Does InterpreterFrame::DEBUGGEE mean anything other than
> frame->script()->debugMode()?
> 
> And so on.

I'll write something like the following in the code.

Let a script be considered a debuggee script iff either all execution is observed or the script has breakpoints set.

The invariants are:

Compartment invariants
----------------------

1. When a compartment's debugModeBits is DebugMode, nothing changes to the execution of scripts. Relazification/lazy scripts are disabled. However, it is still used in some places as a fast check for whether we should bothering doing a slow check for if a frame is a debuggee frame.

2. When a compartment's debugModeBits is DebugMode | DebugObservesAllExecution, all scripts are considered debuggee scripts.

Script and Frame Invariants
---------------------------

1. Debuggee scripts always push debuggee frames.

2. Debuggee frames execute all Debugger slow paths.

3. When attempting to enter Baseline, if the script is a debuggee script or if the OSR frame is a debuggee frame, the BaselineScript is compiled with debug hook calls.

4. When attempting to enter Ion, skip if the script is a debuggee script or if the OSR frame is a debuggee frame.

Comment 27

[Shu-yu Guo [:shu]]

Attachment: Part 2: Mark frames as debuggee frames instead of entire compartments.

Added explanatory block comment.

Comment 28

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #26)

Yes, this is worth its weight in gold. (Or, would be, if it weighed a lot.)

Comment 29

[Jim Blandy :jimb]

Comment on attachment 8481722 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8481722 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscompartment.cpp
@@ +878,5 @@
> +        //   var dbg2 = new Debugger(global);
> +        //
> +        // If we don't clear all execution bits, a stale
> +        // DebugObservesAllExecution bit would result in Debugger considering
> +        // 'global' has having been observing all execution for all code

I can't quite parse this. "would result in Debugger considering 'global' has been"?

But I also think a big comment here is unnecessary. Assuming I'm understanding how all this works (!!!), the comments for DebugObservesAllExecution should simply say that it's only set when DebugMode is also set, that it's a sub-mode of debug mode; the accessors should assert that; and then there's no point in describing the consequences of the specific incoherent state where DebugObservesAllExecution is set but DebugMode is not, any more than any other forbidden state.

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 8485327 [details] [diff] [review]
Part 3: Selectively deoptimize when Debugger needs to observe execution.

Review of attachment 8485327 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. Looks reasonable at first glance, but some questions before I take a closer look.

::: js/src/jit/BaselineJIT.cpp
@@ +291,5 @@
>                  return status;
>          }
>      }
>  
> +    return BaselineCompile(cx, script, osrFrame && osrFrame->isDebuggeeFrame());

This needs a comment. Why do we want a debuggee BaselineScript/frames if we don't have a debuggee script?

::: js/src/jit/Ion.cpp
@@ +2106,5 @@
>  
>      if (!script->hasBaselineScript())
>          return Method_Skipped;
>  
> +    if (script->isDebuggee() || (osrFrame && osrFrame->isDebuggeeFrame())) {

When a script becomes a non-debuggee script, its frames are still debuggee frames?

::: js/src/jit/IonFrames.cpp
@@ +404,5 @@
>      bool bailedOutForDebugMode = false;
> +    if (cx->compartment()->debugMode()) {
> +        bool shouldBail = cx->compartment()->debugObservesAllExecution();
> +        if (!shouldBail) {
> +            JitActivation *act = cx->mainThread().activation()->asJit();

Nit: this branch could use a comment.

::: js/src/jit/RematerializedFrame.h
@@ +87,5 @@
>          return top_;
>      }
> +    JSScript *topScript() const {
> +        IonJSFrameLayout *jsFrame = (IonJSFrameLayout *)top_;
> +        return ScriptFromCalleeToken(jsFrame->calleeToken());

Nit: I think s/topScript/outerScript would be clearer, or is "top" referring to something else?

::: js/src/vm/Debugger.cpp
@@ +1776,5 @@
> +
> +    // Keep this list in sync with HookObservesAllExecution.
> +    return (getHook(OnDebuggerStatement) ||
> +            getHook(OnExceptionUnwind) ||
> +            getHook(OnEnterFrame));

Nit: no need for the outer parentheses. Also, hookObservesAllExecution should have the same comment. It might be nicer to have a |static const| array of hooks that observe all execution and then loop over it, or something.

Comment 31

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #30)
> Comment on attachment 8485327 [details] [diff] [review]
> Part 3: Selectively deoptimize when Debugger needs to observe execution.
> 
> Review of attachment 8485327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. Looks reasonable at first glance, but some questions
> before I take a closer look.
> 
> ::: js/src/jit/BaselineJIT.cpp
> @@ +291,5 @@
> >                  return status;
> >          }
> >      }
> >  
> > +    return BaselineCompile(cx, script, osrFrame && osrFrame->isDebuggeeFrame());
> 
> This needs a comment. Why do we want a debuggee BaselineScript/frames if we
> don't have a debuggee script?
> 

See the note in the block comment in jscompartment.h and the explanation for the next comment below.

> ::: js/src/jit/Ion.cpp
> @@ +2106,5 @@
> >  
> >      if (!script->hasBaselineScript())
> >          return Method_Skipped;
> >  
> > +    if (script->isDebuggee() || (osrFrame && osrFrame->isDebuggeeFrame())) {
> 
> When a script becomes a non-debuggee script, its frames are still debuggee
> frames?
> 

"Debuggee" is really a frame concept, not a script concept. A debuggee script is a script that requires all its newborn frames to be debuggee frames. But a Debugger can mark a non-debuggee script's frame as a debuggee frame, like Debugger.Frame.eval.

I'll also fix all the nits. I'm not going to upload a new patch just yet in case jimb has comments saved.

Comment 32

[Jim Blandy :jimb]

Comment on attachment 8486799 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8486799 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I can review the portions of this patch concerned with setting and clearing the flag on BaselineFrame instances. There seem to be at least five different places those are constructed or updated, not including the places JIT code does it. I'm not in a position to be able to tell whether this patch covers them all or not.

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8486799 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8486799 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/jscompartment.h
@@ +424,3 @@
>      // True if this compartment's global is a debuggee of some Debugger
>      // object.
> +    bool debugMode() const { return !!(debugModeBits & DebugMode); }

I wonder if we should rename debugMode() to something like isDebuggee() or hasDebugger()?

::: js/src/vm/Stack.h
@@ +321,5 @@
>          PREV_UP_TO_DATE    =     0x4000,  /* see DebugScopes::updateLiveScopes */
>  
> +        // See comment above 'debugMode' in jscompartment.h for explanation of
> +        // invariants of debuggee compartments, scripts, and frames.
> +        DEBUGGEE           =     0x8000,  // Execution is being observed by Debugger

Nit: I'd use /* */ comments here to match the rest of the enum/class.

Comment 34

[Jan de Mooij [:jandem]]

Comment on attachment 8485327 [details] [diff] [review]
Part 3: Selectively deoptimize when Debugger needs to observe execution.

I reviewed this patch already. Waiting for those comments to be addressed.

Comment 35

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #33)
> I wonder if we should rename debugMode() to something like isDebuggee() or
> hasDebugger()?

I like that suggestion.

Comment 36

[Jim Blandy :jimb]

Comment on attachment 8486799 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8486799 [details] [diff] [review]:
-----------------------------------------------------------------

Some comments in progress:

::: js/src/jscompartment.cpp
@@ +871,5 @@
> +        // 'global' has having been observing all execution for all code
> +        // between |gc()| and |dbg2 = ...|. This would incorrectly result in
> +        // no scripts being invalidated and no frames being patched when a
> +        // hook requiring execution observability is set on |dbg2|.
> +        debugModeBits &= ~DebugExecutionMask;

The text starting with "Debugger considering..." is still not grammatical, and should be fixed.

But I think this comment is just wrong. If we're leaving debug mode because a Debugger got collected, then we know two things:

a) The Debugger being collected was the only one debugging global. Otherwise we wouldn't be leaving debug mode.

b) The Debugger being collected had no all-observing hooks. We don't collect Debuggers that have all-observing hooks set, because they still have visible effects.

The exception is the case where both compartment and Debugger are detected as garbage simultaneously; then we will certainly be calling Debugger::removeDebuggeeGlobalUnderGC (either from sweepAll or detachAllDebuggersFromGlobal, depending on what the GC decides to finalize first) with the hook still set. But that is explicitly not the case in your example.

::: js/src/jscompartment.h
@@ +417,5 @@
> +    //    attempt to enter Ion.
> +    //
> +    // Note that a debuggee frame may exist without its script being a
> +    // debuggee script. e.g., Debugger.Frame.prototype.eval only marks the
> +    // frame in which it is evaluating as a debuggee frame.

This whole comment is very helpful --- thanks! It might be worth also saying that compartment->debugMode() is true exactly when any Debuggers have compartment as a debuggee.

::: js/src/jsscriptinlines.h
@@ +183,5 @@
> +inline bool
> +JSScript::isDebuggee() const
> +{
> +    return compartment_->debugObservesAllExecution() ||
> +           (compartment_->debugMode() && hasDebugScript_);

Everything would still work if we removed the debugMode() check here, right? It's just a short circuit, because

::: js/src/vm/Debugger.cpp
@@ +2365,5 @@
>      } else {
> +        if (debuggees.put(global)) {
> +            debuggeeCompartment->enterDebugMode();
> +            // TODOshu Refine.
> +            return debuggeeCompartment->setDebugObservesAllExecution(cx, invalidate);

Doesn't this fail to remove global from debuggees, as the old patch did, if setDebugObservesAllExecution fails?

::: js/src/vm/Stack.h
@@ +228,5 @@
>      inline void setPrevUpToDate() const;
>  
> +    inline bool isDebuggeeFrame() const;
> +    inline void setIsDebuggeeFrame();
> +    inline void unsetIsDebuggeeFrame();

Isn't the 'Frame' suffix in these names redundant? The 'this' object will always be a frame, so it should be apparent from context.

Comment 37

[Jim Blandy :jimb]

Comment on attachment 8486799 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8486799 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand how a frame's DEBUGGEE bit gets set when, say, we give it an onStep handler.

DebuggerFrame_setOnStep only calls
  incrementStepModeCount, which only calls
    ensureHasDebugScript, which doesn't touch any frame's DEBUGGEE bits; and
    setNewStepMode, which doesn't either.

What am I missing? Is this in part 3?

Comment 38

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #37)
> Comment on attachment 8486799 [details] [diff] [review]
> Part 2: Mark frames as debuggee frames instead of entire compartments.
> 
> Review of attachment 8486799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand how a frame's DEBUGGEE bit gets set when, say, we give it
> an onStep handler.
> 
> DebuggerFrame_setOnStep only calls
>   incrementStepModeCount, which only calls
>     ensureHasDebugScript, which doesn't touch any frame's DEBUGGEE bits; and
>     setNewStepMode, which doesn't either.
> 
> What am I missing? Is this in part 3?

indeed that is part 3. this part just moved the checks onto the frame, but entering debug mode still immediately sets "observing all execution", which I marked with // TODO Refine. part 3 refines it to actually toggle isDebuggee per frame.

Comment 39

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #36)
> ::: js/src/jsscriptinlines.h
> @@ +183,5 @@
> > +inline bool
> > +JSScript::isDebuggee() const
> > +{
> > +    return compartment_->debugObservesAllExecution() ||
> > +           (compartment_->debugMode() && hasDebugScript_);
> 
> Everything would still work if we removed the debugMode() check here, right?
> It's just a short circuit, because

(ahem) ... because when we dispatched hooks we'd notice the lack of debuggers anyway, right?

Comment 40

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #38)
> indeed that is part 3. this part just moved the checks onto the frame, but
> entering debug mode still immediately sets "observing all execution", which
> I marked with // TODO Refine. part 3 refines it to actually toggle
> isDebuggee per frame.

So, the path goes:

    Debugger::addDebuggeeGlobal
    -> JSCompartment::setDebugObservesAllExecution
       -> JSCompartment::updateJITForDebugMode
          -> jit::UpdateForDebugMode
             -> jit::RecompileOnStackBaselineScriptsForDebugMode
                -> PatchBaselineFramesForDebugMode
                   -> setIsDebuggeeFrame

But a lot of this is going to change in part 3, I assume.

Comment 41

[Jim Blandy :jimb]

I think we should use roman numerals for patches:

Part I
Part II
Part MCMLXXXVI

Comment 42

[Jim Blandy :jimb]

It seems like jit::RematerializeAllFrames is never used. It was introduced in changeset 061ebab47be3, but no uses were added there, or in the immediately subsequent changesets.

(I have to admit, I don't really understand the output I'm getting from 'hg grep'.)

Comment 43

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #42)
> It seems like jit::RematerializeAllFrames is never used. It was introduced
> in changeset 061ebab47be3, but no uses were added there, or in the
> immediately subsequent changesets.

Ah, part 3 again.

Comment 44

[Jim Blandy :jimb]

Why is OnExceptionUnwind considered an ObservesAllExecution hook? It seems like it's only called in places that are going to be executed whether or not the debugger is running.

(The Firefox JS debugger sets this callback almost immediately, so making this a less heavy hook will have a big impact.)

Comment 45

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #36)
> Comment on attachment 8486799 [details] [diff] [review]
> Part 2: Mark frames as debuggee frames instead of entire compartments.
> 
> Review of attachment 8486799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some comments in progress:
> 
> ::: js/src/jscompartment.cpp
> @@ +871,5 @@
> > +        // 'global' has having been observing all execution for all code
> > +        // between |gc()| and |dbg2 = ...|. This would incorrectly result in
> > +        // no scripts being invalidated and no frames being patched when a
> > +        // hook requiring execution observability is set on |dbg2|.
> > +        debugModeBits &= ~DebugExecutionMask;
> 
> The text starting with "Debugger considering..." is still not grammatical,
> and should be fixed.
> 
> But I think this comment is just wrong. If we're leaving debug mode because
> a Debugger got collected, then we know two things:
> 
> a) The Debugger being collected was the only one debugging global. Otherwise
> we wouldn't be leaving debug mode.
> 
> b) The Debugger being collected had no all-observing hooks. We don't collect
> Debuggers that have all-observing hooks set, because they still have visible
> effects.
> 
> The exception is the case where both compartment and Debugger are detected
> as garbage simultaneously; then we will certainly be calling
> Debugger::removeDebuggeeGlobalUnderGC (either from sweepAll or
> detachAllDebuggersFromGlobal, depending on what the GC decides to finalize
> first) with the hook still set. But that is explicitly not the case in your
> example.

I don't even have that block comment anymore. Like you said last time, DebugObservesAllExecution is really a "sub mode", so it makes sense to clear both when leaving debug mode.

> ::: js/src/jscompartment.h
> @@ +417,5 @@
> > +    //    attempt to enter Ion.
> > +    //
> > +    // Note that a debuggee frame may exist without its script being a
> > +    // debuggee script. e.g., Debugger.Frame.prototype.eval only marks the
> > +    // frame in which it is evaluating as a debuggee frame.
> 
> This whole comment is very helpful --- thanks! It might be worth also saying
> that compartment->debugMode() is true exactly when any Debuggers have
> compartment as a debuggee.
> 

Sure, I'll add that.

> ::: js/src/jsscriptinlines.h
> @@ +183,5 @@
> > +inline bool
> > +JSScript::isDebuggee() const
> > +{
> > +    return compartment_->debugObservesAllExecution() ||
> > +           (compartment_->debugMode() && hasDebugScript_);
> 
> Everything would still work if we removed the debugMode() check here, right?
> It's just a short circuit, because
> 

I was under the impression DebugScript hangs around after the compartment leaves debug mode, which I just confirmed is wrong. So you're right, debugMode() there is unnecessary.

> ::: js/src/vm/Debugger.cpp
> @@ +2365,5 @@
> >      } else {
> > +        if (debuggees.put(global)) {
> > +            debuggeeCompartment->enterDebugMode();
> > +            // TODOshu Refine.
> > +            return debuggeeCompartment->setDebugObservesAllExecution(cx, invalidate);
> 
> Doesn't this fail to remove global from debuggees, as the old patch did, if
> setDebugObservesAllExecution fails?

Ah yeah, but this patch is just a stepping stone to part 3, which changes the behavior completely anyhow.

> 
> ::: js/src/vm/Stack.h
> @@ +228,5 @@
> >      inline void setPrevUpToDate() const;
> >  
> > +    inline bool isDebuggeeFrame() const;
> > +    inline void setIsDebuggeeFrame();
> > +    inline void unsetIsDebuggeeFrame();
> 
> Isn't the 'Frame' suffix in these names redundant? The 'this' object will
> always be a frame, so it should be apparent from context.

Sure, I'll rename.

Comment 46

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #44)
> Why is OnExceptionUnwind considered an ObservesAllExecution hook? It seems
> like it's only called in places that are going to be executed whether or not
> the debugger is running.

We check compartment->debugMode() in both Interpreter and Baseline right now:

In Interpreter: http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter.cpp#1033
In Baseline: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonFrames.cpp#514

More problematic though, is that when Ion encounters an exception, it jumps directly to the catch block if one exists. It doesn't unwind every frame. That's one of the things debug mode OSR had to change when debug mode is on. When unwinding an exception in Ion in debug mode, we first bail out in place to Baseline so that Baseline takes care of the unwinding logic, including calling the Debugger hooks.

> (The Firefox JS debugger sets this callback almost immediately, so making
> this a less heavy hook will have a big impact.)

Well that's not good. Why is it set immediately? To break on uncaught exceptions?

It should be possible to relax though. Because Ion can't ever optimize out exceptions for correctness, onExceptionUnwind doesn't need to be considered all-observing. Instead, in all exception paths, we consult the compartment's debug mode as before, and check specifically for presence of onExceptionUnwind. If so, we bail out in place like right now.

Comment 47

[Shu-yu Guo [:shu]]

Attachment: Part 2: Mark frames as debuggee frames instead of entire compartments.

Applied comments.

Comment 48

[Shu-yu Guo [:shu]]

Attachment: Part 3: Selectively deoptimize when Debugger needs to observe execution.

Applied comments.

Comment 49

[Shu-yu Guo [:shu]]

Attachment: Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.

While doing this I discovered an existing bug in BaselineCompiler that treats
ops after a 'throw' as unreachable, even when compiling for debug mode. The fix
was simple, so I lumped it in here.

Comment 50

[Jan de Mooij [:jandem]]

Comment on attachment 8497224 [details] [diff] [review]
Part 3: Selectively deoptimize when Debugger needs to observe execution.

Review of attachment 8497224 [details] [diff] [review]:
-----------------------------------------------------------------

Still worried about the ever increasing complexity of all the debugger stuff, but we don't have a lot of alternatives and fortunately the debugger is not exposed to content.

I didn't look closely at the Debugger.cpp changes; I assume Jim knows that file better.

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +462,5 @@
>  
>            case JitFrame_BaselineStub: {
> +            // XXXshu this is kinda gross.
> +            MOZ_ASSERT(iter.current()->prevType() == JitFrame_BaselineJS);
> +            BaselineFrame *prevFrame = (BaselineFrame *)(iter.prevFp() -

It seems simpler/cleaner to do:

JitFrameIterator prev(iter);
++prev;
MOZ_ASSERT(prev.type() == JitFrame_BaselineJS);
...

::: js/src/vm/Debugger.cpp
@@ +1568,5 @@
> +    bool has(JSScript *script) const {
> +        if (script == frame_.script())
> +            return true;
> +        // If we are trying to ensure observability of an inline Ion frame, we
> +        // need to also match the unlined script which pushed the Ion frame so

s/unlined/outer

Comment 51

[Jan de Mooij [:jandem]]

Comment on attachment 8497272 [details] [diff] [review]
Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.

Review of attachment 8497272 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BytecodeAnalysis.cpp
@@ +196,5 @@
>          }
>  
> +        // Handle any fallthrough from this opcode. When compiling for debug
> +        // mode, consider throws fall-through, as thrown exceptions may be
> +        // "caught" by onExceptionUnwind and discarded.

Please make sure we have jit-tests for this "fall-through" behavior, that fail without this patch?

Comment 52

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #50)
> Comment on attachment 8497224 [details] [diff] [review]
> Part 3: Selectively deoptimize when Debugger needs to observe execution.
> 
> Review of attachment 8497224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still worried about the ever increasing complexity of all the debugger
> stuff, but we don't have a lot of alternatives and fortunately the debugger
> is not exposed to content.
> 

IMO this patch series just moved the complexity from AutoDebugModeInvalidation to a smarter, finer-grained invalidation logic. Still probably a small net gain in complexity, but pretty confined.

Comment 53

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #51)
> Comment on attachment 8497272 [details] [diff] [review]
> Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.
> 
> Review of attachment 8497272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/BytecodeAnalysis.cpp
> @@ +196,5 @@
> >          }
> >  
> > +        // Handle any fallthrough from this opcode. When compiling for debug
> > +        // mode, consider throws fall-through, as thrown exceptions may be
> > +        // "caught" by onExceptionUnwind and discarded.
> 
> Please make sure we have jit-tests for this "fall-through" behavior, that
> fail without this patch?

Oh don't worry, plenty of Debugger tests for onExceptionUnwind start failing, which is how I found this in the first place.

Comment 54

[Shu-yu Guo [:shu]]

Attachment: Part 5: Add an auto-updated DebugModeOSRVolatileJitFrameIterator.

Sorry Jan, another one. Since with part 4 we can trigger debug mode OSR while
in HandleExceptionBaseline, we need to have invalidatable JitFrameIterators, as
they cache return addresses, which get patched.

Thankfully it's pretty straightforward to inherit JitFrameIterator.

I also removed an unused JitFrameIterator constructor.

The bug1006473.js test case covers this.

Comment 55

[Shu-yu Guo [:shu]]

Comment on attachment 8497920 [details] [diff] [review]
Part 5: Add an auto-updated DebugModeOSRVolatileJitFrameIterator.

Review of attachment 8497920 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonFrames.cpp
@@ +641,5 @@
>      // otherwise clear the return override.
>      if (cx->runtime()->jitRuntime()->hasIonReturnOverride())
>          cx->runtime()->jitRuntime()->takeIonReturnOverride();
>  
> +    DebugModeOSRVolatileJitFrameIterator iter(cx);

I'll add a comment above this line explaining why this is needed.

Comment 56

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #49)
> Created attachment 8497272 [details] [diff] [review]
> Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.
> 
> While doing this I discovered an existing bug in BaselineCompiler that treats
> ops after a 'throw' as unreachable, even when compiling for debug mode. The
> fix
> was simple, so I lumped it in here.

... Wait, how does debug mode make it possible to continue past a throw? Stepping across a throw should trigger the frame's onPop call, but that shouldn't be able to cancel the throw, only turn it into a 'return'.

Comment 57

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #56)
> (In reply to Shu-yu Guo [:shu] from comment #49)
> > Created attachment 8497272 [details] [diff] [review]
> > Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.
> > 
> > While doing this I discovered an existing bug in BaselineCompiler that treats
> > ops after a 'throw' as unreachable, even when compiling for debug mode. The
> > fix
> > was simple, so I lumped it in here.
> 
> ... Wait, how does debug mode make it possible to continue past a throw?
> Stepping across a throw should trigger the frame's onPop call, but that
> shouldn't be able to cancel the throw, only turn it into a 'return'.

Sorry, I was speaking inaccurately and my comment in the patch is wrong. Debug mode doesn't make code after a throw reachable in the execution sense. However, if onExceptionUnwind does a debug mode OSR and recompiles the stack, we need a place to patch the resume to. For VM calls, which 'throw' is compiled as, debug mode OSR resumes at the next opcode. But since the all remaining bytecode is assumed unreachable, debug mode OSR can't find a place to patch to and crashes.

The better fix here is to make debug mode OSR handle 'throw' differently, instead of compiling all code as reachable.

Comment 58

[Shu-yu Guo [:shu]]

Attachment: Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.

Comment 59

[Shu-yu Guo [:shu]]

Comment on attachment 8505784 [details] [diff] [review]
Part 4: Don't consider onExceptionUnwind an all-execution-observing hook.

Review of attachment 8505784 [details] [diff] [review]:
-----------------------------------------------------------------

Simpler patch in light of comment 56. Carry r+ from jandem.

Comment 60

[Jim Blandy :jimb]

Cool. "Obscure intermediate states need to be handled" is more comforting than "control goes where one would think it mustn't".

Comment 61

[Shu-yu Guo [:shu]]

Attachment: Part 2: Mark frames as debuggee frames instead of entire compartments.

Rebased.

Comment 62

[Shu-yu Guo [:shu]]

Attachment: Part 3: Selectively deoptimize when Debugger needs to observe execution.

Rebased.

Comment 63

[Jim Blandy :jimb]

Comment on attachment 8506310 [details] [diff] [review]
Part 2: Mark frames as debuggee frames instead of entire compartments.

Review of attachment 8506310 [details] [diff] [review]:
-----------------------------------------------------------------

A comment suggestion:

::: js/src/jit/BaselineBailouts.cpp
@@ +594,5 @@
>      }
>  
> +    // If we are bailing to a script whose execution is observed, mark the
> +    // baseline frame as a debuggee frame. This is to cover the case where we
> +    // don't rematerialize the Ion frame via the Debugger.

"... the case where we don't rematerialize the Ion frame via the Debugger" reads as "... the case where something other than Debugger rematerializes the Ion frame" (it seems like "via" begins a restrictive clause). But what I think you mean is "... the case where Debugger forces the Ion frame to bail out, yet does not create a rematerialized frame for it".

If I were the reader, what I'd want the comment to explain here is under which circumstances Debugger forces an Ion frame to bail out, yet does not create a rematerialized frame for it - or at least an example of such a circumstance.

Comment 64

[Jim Blandy :jimb]

Just so folks following along know the story: we realized that the patch doesn't handle setting breakpoints correctly; there's nothing in it to find all the IonScripts into which a given JSScript has been inlined. Shu came up with the following test case:

    // Breakpoints should be hit on scripts gotten not via Debugger.Frame.
     
    var g = newGlobal();
    g.eval("function f(x) { return x + 1; }");
    g.eval("for (var i = 0; i < 100; i++) f(i);");
    var dbg = new Debugger;
    var gw = dbg.addDebuggee(g);
    var fw = gw.getOwnPropertyDescriptor("f").value;
    var hits = 0;
    fw.script.setBreakpoint(0, { hit: function(frame) { hits++; } });
    g.eval("f(42);");
    assertEq(hits, 1);

Scanning the zone for IonScripts, and then scanning each IonScript's snapshots to see if it has inlined the JSScript in which we're setting a breakpoint would be slow. As a conservative test that avoids the Snapshot decoding overhead, we believe we can scan each IonScript's constant table for functions that use that JSScript.

Comment 65

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #64)
> Just so folks following along know the story: we realized that the patch
> doesn't handle setting breakpoints correctly; there's nothing in it to find
> all the IonScripts into which a given JSScript has been inlined. Shu came up
> with the following test case:
> 
>     // Breakpoints should be hit on scripts gotten not via Debugger.Frame.
>      
>     var g = newGlobal();
>     g.eval("function f(x) { return x + 1; }");
>     g.eval("for (var i = 0; i < 100; i++) f(i);");
>     var dbg = new Debugger;
>     var gw = dbg.addDebuggee(g);
>     var fw = gw.getOwnPropertyDescriptor("f").value;
>     var hits = 0;
>     fw.script.setBreakpoint(0, { hit: function(frame) { hits++; } });
>     g.eval("f(42);");
>     assertEq(hits, 1);
> 
> Scanning the zone for IonScripts, and then scanning each IonScript's
> snapshots to see if it has inlined the JSScript in which we're setting a
> breakpoint would be slow. As a conservative test that avoids the Snapshot
> decoding overhead, we believe we can scan each IonScript's constant table
> for functions that use that JSScript.

Neither reading the constant table approach nor reading OSI-indexed Snapshots works, both for the same reason: we can optimize out snapshots for inlinees that cannot be invalidated (e.g., simple functions like the identity function). The type barriers that guard on the inlinee function is often on stuff like JSOP_GETGNAME and those snapshots encode no constants either.

I think I will track the set of inlined JSScripts in each IonScript, all the time. Jan, what do you think?

Comment 66

[Shu-yu Guo [:shu]]

Attachment: Part 2.1: Ensure scripts are observable when breakpoints are set.

Comment 67

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #65)
> (In reply to Jim Blandy :jimb from comment #64)
> > Scanning the zone for IonScripts, and then scanning each IonScript's
> > snapshots to see if it has inlined the JSScript in which we're setting a
> > breakpoint would be slow. As a conservative test that avoids the Snapshot
> > decoding overhead, we believe we can scan each IonScript's constant table
> > for functions that use that JSScript.
> 
> Neither reading the constant table approach nor reading OSI-indexed
> Snapshots works, both for the same reason: we can optimize out snapshots for
> inlinees that cannot be invalidated (e.g., simple functions like the
> identity function). The type barriers that guard on the inlinee function is
> often on stuff like JSOP_GETGNAME and those snapshots encode no constants
> either.

In addition, this is going at the opposite of the optimization I need to do remove the allocation of the scope chain.

> I think I will track the set of inlined JSScripts in each IonScript, all the
> time. Jan, what do you think?

If this is not provided by the PC & script mapping, then yes.

Comment 68

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #65)
> I think I will track the set of inlined JSScripts in each IonScript, all the
> time. Jan, what do you think?

(We did this at some point but I removed it, I think after we got rid of JM.)

It'd probably be simpler to reuse the TI mechanism for invalidating outer script when an inlined script's types change. In IonBuilder::makeInliningDecision we do:

    targetType->watchStateChangeForInlinedCall(constraints());

Then in TypeZone::addPendingRecompile:

    // When one script is inlined into another the caller listens to state
    // changes on the callee's script, so trigger these to force recompilation
    // of any such callers.
    if (script->functionNonDelazifying() && !script->functionNonDelazifying()->hasLazyType())
        ObjectStateChange(cx, script->functionNonDelazifying()->type(), false);

So it seems like you should be able to hook into this somehow, maybe by calling addPendingRecompile somewhere. Does that work?

Comment 69

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #68)
> (In reply to Shu-yu Guo [:shu] from comment #65)
> > I think I will track the set of inlined JSScripts in each IonScript, all the
> > time. Jan, what do you think?
> 
> (We did this at some point but I removed it, I think after we got rid of JM.)
> 
> It'd probably be simpler to reuse the TI mechanism for invalidating outer
> script when an inlined script's types change. In
> IonBuilder::makeInliningDecision we do:
> 
>     targetType->watchStateChangeForInlinedCall(constraints());
> 
> Then in TypeZone::addPendingRecompile:
> 
>     // When one script is inlined into another the caller listens to state
>     // changes on the callee's script, so trigger these to force
> recompilation
>     // of any such callers.
>     if (script->functionNonDelazifying() &&
> !script->functionNonDelazifying()->hasLazyType())
>         ObjectStateChange(cx, script->functionNonDelazifying()->type(),
> false);
> 
> So it seems like you should be able to hook into this somehow, maybe by
> calling addPendingRecompile somewhere. Does that work?

Good idea, that should work.

Comment 70

[Shu-yu Guo [:shu]]

Attachment: Part 2 addendum: Deal with inliners more simply.

Jan's suggestion simplifies the invalidating-inliners case.

Comment 71

[Jim Blandy :jimb]

It's worth pointing out that the new "isDebuggee" predicates are using "debuggee" in a way inconsistent with the definition given in the docs:

https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Conventions

And this despite jandem and me having suggested and approved of these patches' terminology in comments 33 and 35.

We should fix that in a follow-up; I don't want to throw anything inessential in the way of this patch.

Comment 72

[Jim Blandy :jimb]

Comment on attachment 8511310 [details] [diff] [review]
Part 2 addendum: Deal with inliners more simply.

Review of attachment 8511310 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +4458,5 @@
>      if (!handler)
>          return false;
>  
> +    // Ensure observability *before* setting the breakpoint. If the script's
> +    // compartment is not already a debuggee, trying to ensure execution after

We should throw if someone attempts to set a breakpoint in a non-debuggee script ("non-debuggee" in the public API's sense of that term, i.e., not in a compartment whose global is a debuggee).

Comment 73

[Shu-yu Guo [:shu]]

Attachment: Part 2: Move debuggee-ness to frames and selectively deoptimize when Debugger needs to observe execution.

Folded up parts 2, 3, and 2 addendum.

Comment 74

[Jason Orendorff [:jorendorff]]

Review ping!

Comment 75

[Jim Blandy :jimb]

If I'm reading UpdateExecutionObservabilityOfScriptsInZone correctly (with all patches applied):

It seems to me that this:

- accumulates a vector of JSScripts, which will either be one or two entries, or contain all scripts in the zone;

- calls TypeZone::addPendingRecompile on all of them; and

- calls DiscardBaselineScript on those that are not on the stack.

In other words, we build up an explicit representation of all the scripts we care about, and then
subtract out those that are on the stack.

Wouldn't it be more efficient to first note the ones that are on the stack, and then loop over all scripts, calling addPendingRecompile and DiscardBaselineScript as we go? Then there's no need to build up the giant vector, whose only role is to intersect the observable set with the zone.

If it doesn't seem nicer, don't bother. But please do confirm my understanding of the computation being carried out here.

Comment 76

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #75)
> If I'm reading UpdateExecutionObservabilityOfScriptsInZone correctly (with
> all patches applied):
> 
> It seems to me that this:
> 
> - accumulates a vector of JSScripts, which will either be one or two
> entries, or contain all scripts in the zone;
> 
> - calls TypeZone::addPendingRecompile on all of them; and
> 
> - calls DiscardBaselineScript on those that are not on the stack.
> 
> In other words, we build up an explicit representation of all the scripts we
> care about, and then
> subtract out those that are on the stack.
> 
> Wouldn't it be more efficient to first note the ones that are on the stack,
> and then loop over all scripts, calling addPendingRecompile and
> DiscardBaselineScript as we go? Then there's no need to build up the giant
> vector, whose only role is to intersect the observable set with the zone.
> 
> If it doesn't seem nicer, don't bother. But please do confirm my
> understanding of the computation being carried out here.

Your understanding is correct, and what you suggest should be nicer too.

Comment 77

[Jim Blandy :jimb]

Comment on attachment 8512309 [details] [diff] [review]
Part 2: Move debuggee-ness to frames and selectively deoptimize when Debugger needs to observe execution.

Review of attachment 8512309 [details] [diff] [review]:
-----------------------------------------------------------------

Looks absolutely wonderful. Some bugs noted.

::: js/src/jit/BaselineCompiler.cpp
@@ +242,5 @@
>      // The last entry in the last index found, and is used to avoid binary
>      // searches for the sought entry when queries are in linear order.
>      bytecodeMap[script->nTypeSets()] = 0;
>  
> +    if (debugMode_)

It seems like this should have been that way all along.

::: js/src/jit/VMFunctions.cpp
@@ +987,5 @@
>  
>  bool
>  DebugLeaveBlock(JSContext *cx, BaselineFrame *frame, jsbytecode *pc)
>  {
> +    MOZ_ASSERT(frame->isDebuggee());

Don't we leave Baseline scripts with debug instrumentation in place, in some circumstances, even though the JSScripts are no longer debuggees? It seems to me that could lead to calls to these functions from non-debuggee frames.

If that's correct, please make sure we have tests that catch this incorrect assertion.

It seems like several of these VM functions should be testing frame->isDebuggee, not asserting it.

::: js/src/vm/Debugger.cpp
@@ +1684,5 @@
> +    JSScript *singleScriptForZoneInvalidation() const { return script_; }
> +    bool shouldRecompileOrInvalidate(JSScript *script) const { return script == script_; }
> +    bool shouldMarkAsDebuggee(ScriptFrameIter &iter) const {
> +        // AbstractFramePtr can't refer to non-remateralized Ion frames, so if
> +        // iter refers to one such, we know we don't match.

This comment doesn't make sense here in ExecutionObservableScript::shouldMarkAsDebuggee: an iter referring to an non-rematerialized Ion frame could certainly be running script_, either directly or through inlining.

@@ +2837,5 @@
> +                !ensureExecutionObservabilityOfCompartment(cx, debuggeeCompartment))
> +            {
> +                /* Maintain consistency on error. */
> +                debuggees.remove(global);
> +                return false;

It seems like this doesn't preserve the removal of the object metadata callback on error. Granted, the js_ReportOutOfMemory should be skipped, but we should be careful to preserve the back-out behavior here.

@@ +4446,5 @@
>      if (!handler)
>          return false;
>  
> +    // Ensure observability *before* setting the breakpoint. If the script's
> +    // compartment is not already a debuggee, trying to ensure execution after

"ensure observability", right?

@@ -5034,5 @@
>      if (!IsValidHook(args[0])) {
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_CALLABLE_OR_UNDEFINED);
>          return false;
>      }
> -

I think we should leave this blank link here.

::: js/src/vm/Debugger.h
@@ +216,5 @@
> +    };
> +
> +    enum IsObserving {
> +        NotObserving = 0,
> +        Observing = 1

This needs a comment saying that it's converted to and compared with bool, so the values must be as given.

Comment 78

[Shu-yu Guo [:shu]]

I'm hitting very intermittent assertions with JS_GC_ZEAL=14 I don't know how to debug at the moment, unfortunately. :(

Comment 79

[Shu-yu Guo [:shu]]

Attachment: Part 2 addendum: ensure observability on re-enabling Debuggers.

Comment 80

[Jim Blandy :jimb]

Comment on attachment 8521756 [details] [diff] [review]
Part 2 addendum: ensure observability on re-enabling Debuggers.

Review of attachment 8521756 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Sorry for missing this. I'm glad to see that the structure of the rest of the change made this a quick fix.

Comment 81

[Shu-yu Guo [:shu]]

Attachment: Part 2 addendum: ensure debuggee scripts abort Ion compilation if they were made debuggees mid off-thread compilation.

Comment 82

[Jim Blandy :jimb]

Comment on attachment 8521865 [details] [diff] [review]
Part 2 addendum: ensure debuggee scripts abort Ion compilation if they were made debuggees mid off-thread compilation.

Review of attachment 8521865 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 83

[Shu-yu Guo [:shu]]

Attachment: Part 6: Add JSOP_DEBUGAFTERYIELD to fix up resumed generator BaselineFrames.

Comment 84

[Jan de Mooij [:jandem]]

Comment on attachment 8522089 [details] [diff] [review]
Part 6: Add JSOP_DEBUGAFTERYIELD to fix up resumed generator BaselineFrames.

Review of attachment 8522089 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3019,5 @@
>  
> +    if (!bce->yieldOffsetList.append(bce->offset()))
> +        return false;
> +
> +    return Emit1(cx, bce, JSOP_DEBUGAFTERYIELD);

return Emit1(...) >= 0;

::: js/src/jit/BaselineCompiler.cpp
@@ +3340,5 @@
> +{
> +    if (!compileDebugInstrumentation_)
> +        return true;
> +
> +    masm.loadBaselineFramePtr(BaselineFrameReg, R0.scratchReg());

Nit: frame.assertSyncedStack(); before this line.

Comment 85

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/96a2f59f6ce4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/06d07689a043
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d65f01b9e6c6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbd7a8b984a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b160657339f8

Comment 86

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dbd7a8b984a
https://hg.mozilla.org/mozilla-central/rev/b160657339f8
https://hg.mozilla.org/mozilla-central/rev/96a2f59f6ce4
https://hg.mozilla.org/mozilla-central/rev/06d07689a043
https://hg.mozilla.org/mozilla-central/rev/d65f01b9e6c6