914561 - Add SPS instrumentation into the generateEnterJit trampoline.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Currently, when we have some Jit code on the stack, we cannot walk through it to continue on the C++ code.

One option which looks more like what static compilers are doing would be to generate a mapping of all generated PC to the stack depth.  Unfortunately, this is an unpractical approach for the JIT compiler.  Doing so, would implies generating such data for the all Jit code, as well as for every ICs.  Also, the stack depth of each frame is not a constant across all the code of an Ion frame, and we cannot only store one approximate stack depth for one Ion compilation.

The other option which is suggested by the bug title would be to add a SPS Mark in generateEnterJit, which set the stack pointer of the Mark as being as a constant offset away from the return address and from the register spill done by generateEnterJit.

Comment 1

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: Add EnterJit mark on the stack.

This patch add an EnterJIT mark on the sps stack.  The stack pointer is just after the previous frame pointer (not on ARM) & return address and above the spill.

jld: Tell me if this answer your expectations, and I will ask djvj for review after.  Also, tell me when I can land this patch as I guess that we might need some instrumentation to ignore at the moment.

https://tbpl.mozilla.org/?tree=Try&rev=684ac7bbe197

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 806168 [details] [diff] [review]
Add EnterJit mark on the stack.

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

The issues I saw were simple enough that I tried fixing them locally, and I got it to build for b2g.  But the result is that, if I set MOZ_PROFILING_STARTUP, there's Baseline jitcode that tries to do an updatePC on the enterJIT frame (which crashes, because the JSScript pointer is null) via DoCallFallback.  The last few SPS frames are: "shell_start (chrome://browser/content/shell.js:203)", "forEach (self-hosted:310)", "EnterJit".  I don't know enough about this code to see what's breaking here.  I'll attach my incremental changes.

::: js/src/jit/IonMacroAssembler.h
@@ +891,5 @@
> +    // on the SPS stack.
> +    void spsMarkJit(SPSProfiler *p, Register framePtr, Register temp) {
> +        Label spsNotEnabled;
> +        uint32_t *enabledAddr = p->addressOfEnabled();
> +        loadPtr(AbsoluteAddress(enabledAddr), ebx);

Obvious thing that the try build has already pointed out: we can't refer to "ebx" in machine-independent code.

@@ +898,5 @@
> +
> +        Label stackFull;
> +        spsProfileEntryAddress(p, 0, temp, &stackFull);
> +
> +        const char *str = "EnterJit";

If I understand what's going on here: It might be more useful if this was a const char[] (does that guarantee not being merged with anything?) that was made visible to the profiler, so it could be tested with address comparison instead of strcmp.

@@ +899,5 @@
> +        Label stackFull;
> +        spsProfileEntryAddress(p, 0, temp, &stackFull);
> +
> +        const char *str = "EnterJit";
> +        storePtr(ImmWord(str),          Address(temp, ProfileEntry::offsetOfString()));

I think some of these should be ImmPtr, not ImmWord; my build insists on uintptr_t for ImmWord.

@@ +918,5 @@
> +        Label spsNotEnabled;
> +        pop(ebx); // +4: Was the profiler enabled.
> +        branch32(Assembler::Equal, ebx, Imm32(0), &spsNotEnabled);
> +
> +        movePtr(ImmWord(p->addressOfSizePointer()), temp);

I don't understand why this uses p->addressOfSizePointer but spsMarkJit uses p->sizePointer.

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +80,5 @@
>      void *r11;
>      // The abi does not expect r12 (ip) to be preserved
>      void *lr;
>  
> +    size_t hasSPSMark;

This field can't go here — the saved lr has to be the last field before the arguments, or the return won't work.  The last thing GenerateReturn emits is an ldmia sp!, {...} (abbreviated "pop") to restore the non-volatile registers and return in a single instruction; the saved lr is loaded into the pc (== r15), after which there are no more registers to pop the SPS mark and get the stack pointer back to where it was on entry.

Since we're not in Thumb mode I guess we could save and restore SP along with the other registers instead of using writeback, the same way the obsolete APCS frame pointer ABI worked, but I'm pretty sure it's better to just put this field somewhere else.

Specifically, we should be able to push/pop this field in the same place the alignment padding currently is.

@@ +131,5 @@
>      masm.finishDataTransfer();
>      masm.transferMultipleByRuns(NonVolatileFloatRegs, IsStore, sp, DB);
>  
> +    // Push the EnterJIT sps mark.
> +    masm.movePtr(Operand(sp, offsetof(EnterJITStack, lr)), r8);

This offset seems a little arbitrary.

@@ +132,5 @@
>      masm.transferMultipleByRuns(NonVolatileFloatRegs, IsStore, sp, DB);
>  
> +    // Push the EnterJIT sps mark.
> +    masm.movePtr(Operand(sp, offsetof(EnterJITStack, lr)), r8);
> +    masm.spsMarkJit(&cx->runtime()->spsProfiler, r8, r9);

This would put hasSPSMark *below* all the saved registers, not above them.

@@ +336,5 @@
>      //   aasm->as_extdtr(IsStore, 64, true, Offset,
>      //                   JSReturnReg_Data, EDtrAddr(r5, EDtrOffImm(0)));
>  
> +    // Unwind the sps mark.
> +    masm.spsUnmarkJit(&cx->runtime()->spsProfiler, rbx);

…and this also expects hasSPSMark below the saved registers.  (Also, no "rbx" here.)

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: assorted fixes for the enterJIT SPS instrumentation

To be applied on top of the other patch.  Builds, but crashes.

(gdb) bt
#0  js::ProfileEntry::setPC (this=0x403350d0, pc=0x42dcc4a2 ":")
    at /home/jld/src/B2G/gecko/js/src/vm/SPSProfiler.cpp:278
#1  0x41c50240 in js::SPSProfiler::updatePC (cx=0x45c147f0, frame=0xbea4d398, stub=0x4709fc50, 
    argc=3, vp=0xbea4d310, res=...) at /home/jld/src/B2G/gecko/js/src/vm/SPSProfiler.h:174
#2  DoCallFallback (cx=0x45c147f0, frame=0xbea4d398, stub=0x4709fc50, argc=3, vp=0xbea4d310, 
    res=...) at /home/jld/src/B2G/gecko/js/src/jit/BaselineIC.cpp:7561
#3  0x42b764e8 in ?? ()
#4  0x42b764e8 in ?? ()

(gdb) p cx->runtime_->spsProfiler.size_[0]
$2 = 14
(gdb) p cx->runtime_->spsProfiler.stack_[11]
$5 = {string = 0x421f5780 "js::RunScript", sp = 0xbea4d9e0, script_ = 0x0, idx = -1, 
  static NullPCIndex = -1}
(gdb) p cx->runtime_->spsProfiler.stack_[12]
$3 = {string = 0x46adbdc0 "forEach (self-hosted:310)", sp = 0x0, script_ = 0x450c33d0, idx = 201, 
  static NullPCIndex = -1}
(gdb) p cx->runtime_->spsProfiler.stack_[13]
$4 = {string = 0x4222ad88 "EnterJit", sp = 0xbea4d42c, script_ = 0x0, idx = -1, 
  static NullPCIndex = -1}

We can use EnterJit frame recorded in the SPS stack to debug under the confused jitcode (which, as a side effect, verifies that it's mostly doing the right thing):

(gdb) x/9xw 0xbea4d42c
0xbea4d42c:     0x43f8b260      0x45c147f0      0xbea4d498      0xbea4d4d0
0xbea4d43c:     0xbea4d4b8      0x42b6e808      0x0000ffff      0x47153c00
0xbea4d44c:     0x41c5d59b
(gdb) set $r4 = 0x43f8b260
(gdb) set $r5 = 0x45c147f0
(gdb) set $r6 = 0xbea4d498
(gdb) set $r7 = 0xbea4d4d0
(gdb) set $r8 = 0xbea4d4b8
(gdb) set $r9 = 0x42b6e808
(gdb) set $r10 = 0x0000ffff
(gdb) set $r11 = 0x47153c00
(gdb) set $lr = 0x41c5d59b
(gdb) set $sp = 0xbea4d42c + 36
(gdb) set $pc = $lr
(gdb) bt
#0  EnterBaseline (cx=0x45c147f0, fp=0x43f8b260, pc=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/src/jit/BaselineJIT.cpp:123
#1  js::jit::EnterBaselineAtBranch (cx=0x45c147f0, fp=0x43f8b260, pc=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/src/jit/BaselineJIT.cpp:206
#2  0x41b2133a in Interpret (cx=0x45c147f0, state=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/src/vm/Interpreter.cpp:1556
[64 further frames elided]

The part of Interpret() that we're in bears the comment "// Attempt on-stack replacement with Baseline code."  So... we enter a function in the interpreter, set up an SPS frame for it, then switch to a Baseline version and push the EnterJIT frame on top of the function's frame.  I can see how that would break things.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I thought I could make OSR work by having EnterBaselineAtBranch pop the function's SPS frame and unsetPushedSPSFrame before entering Baseline.  But that's not quite working — maybe something doesn't respect the absence of HAS_PUSHED_SPS_FRAME and is trying to pop a frame that's not there?

This probably calls for a debug build.

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Jed Davis [:jld] from comment #4)
> I thought I could make OSR work by having EnterBaselineAtBranch pop the
> function's SPS frame and unsetPushedSPSFrame before entering Baseline.  But
> that's not quite working — maybe something doesn't respect the absence of
> HAS_PUSHED_SPS_FRAME and is trying to pop a frame that's not there?

Note that the generateEnterJit might add an entry in the middle of the stack, so if something expect that the frame is pushed by parent and avoid pushing one, then the assumption will break with this patch.  I guess we whould remove the OSR special cases and always push a new frame when we enter a new mode of execution.

As the EnterJIT stub is pushing a mark, we cannot reuse the one below, unless we want to mutate the parent entry.
Also note that we might have a similar problem on bailouts.

currently: A > B
after: A > EnterJit > A > B

So, what we want in order:
  1/ Remove re-used frames.
  2/ Add EnterJIT frame.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I think I've gotten it working, by having Baseline→Ion OSR check in EnsureCanEnterIon for !hasPushedSPSFrame and pushing the frame at that point.  This suggests that we did interpreter→Baseline OSR, then reached Baseline→Ion OSR in the same function without having pushed the SPS frame, which means that frame was invisible to the profiler until then, which can probably be improved on.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://people.mozilla.org/~bgirard/cleopatra/#report=c1da0e6837f0092436068e74ce845fc3e9e6bdee

Maybe I should take the bug at this point?

Comment 8

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Jed Davis [:jld] from comment #7)
> https://people.mozilla.org/~bgirard/cleopatra/
> #report=c1da0e6837f0092436068e74ce845fc3e9e6bdee
> 
> Maybe I should take the bug at this point?

Feel free. ;)

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

nbp has convinced me that we actually do want to go with what's described in comment #5.  Stuff:

* Push SPS frame on OSR into Baseline; this might have to go into the trampoline.
* Push SPS frame on OSR into Ion; there's already a thing for running a prologue and then jumping into the middle of a function.
* Make return from OSR not expect the outer execution environment's frame to have already been popped.
* Bailout probably also needs adjustment?

The result should be that push and pop operations will nicely line up, instead of taking the current situation with different modes of execution compensating for each other's stack effects and making it worse.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p0-sps-enterjit-mark-hg0.diff

This adds an EnterJIT mark on entry to jitcode (call or OSR).  It doesn't distinguish BC from Ion, or do anything on OSR/bailout between those two modes.  But this seems to be enough to keep the SPS stack balanced and give the unwinder (in the next patch) the info it needs.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p1-sps-enterjit-ehabi-hg0.diff

…and this is the glue for the profiler in EHABI mode.

Similar glue for breakpad (or the new new unwinder that will replace it) is left as an exercise for another bug.

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8363284 [details] [diff] [review]
bug914561-p1-sps-enterjit-ehabi-hg0.diff

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

::: tools/profiler/TableTicker.cpp
@@ +491,5 @@
> +  mcontext_t savedContext;
> +  PseudoStack *pseudoStack = aProfile.GetPseudoStack();
> +
> +  array.count = 0;
> +  for (uint32_t i = pseudoStack->stackSize(); i > 0; --i) {

I'd like to see a complain explaining what is happening here. Something like
// Jit Pseudostack entries store the CPU Context before entering the JIT
// This allows us to resume unwinding past JS jit blocks.

Are you building pseudoStack in the right order? I believe it's expected to be current PC working backwards to the main function.

@@ +493,5 @@
> +
> +  array.count = 0;
> +  for (uint32_t i = pseudoStack->stackSize(); i > 0; --i) {
> +    volatile StackEntry &entry = pseudoStack->mStack[i - 1];
> +    if (!entry.js() && 0 == strcmp(entry.label(), "EnterJIT")) {

We don't use the '0 == cond' style in the profiler.

@@ +518,5 @@
> +    }
> +  }
> +
> +  // Now unwind whatever's left.
> +  array.count += EHABIStackWalk(*mcontext, aProfile.GetStackTop(),

This code could in theory unwind past a JIT block I believe? If this stack unwind isn't strictly increasing then the merging code could fail (or maybe deadlock?).

Can you confirm that the stack is in the right order and that it will always be going up the stack (towards main)?

Comment 13

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8363282 [details] [diff] [review]
bug914561-p0-sps-enterjit-mark-hg0.diff

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

I am not sure to understand the OSR part in generateEntrerJit, and I failed to understand from where the "2*" is coming from.
I am canceling the review for now, ask me again for review after the explanation.

Also, can you add either as part of this patch or as part of another patch a way to toggle the sps flag from the testing functions?  You can add it as part of JS_SetGlobalJitCompilerOption.
And add test case where we trigger this OSR case.

::: js/src/jit/IonMacroAssembler.cpp
@@ +1866,5 @@
> +    uint32_t *enabledAddr = p->addressOfEnabled();
> +    movePtr(ImmPtr(enabledAddr), temp);
> +    load32(Address(temp, 0), temp);
> +    push(temp); // +4: Did we push an sps frame.
> +    branch32(Assembler::Equal, temp, Imm32(0), &spsNotEnabled);

nit: use branchTestPtr(Assembler::Zero, temp, temp, …);

@@ +1890,5 @@
> +void
> +MacroAssembler::spsUnmarkJit(SPSProfiler *p, Register temp) {
> +    Label spsNotEnabled;
> +    pop(temp); // +4: Was the profiler enabled.
> +    branch32(Assembler::Equal, temp, Imm32(0), &spsNotEnabled);

nit: same here.

@@ +1908,5 @@
> +// Baseline frame pointer, pointing after the end of a BaselineFrame
> +// struct which has already been prepared by |initForOsr|.
> +void
> +MacroAssembler::spsFixupOsr(SPSProfiler *p, Register framePtr, Register temp, Register temp2) {
> +    static const size_t twoFrames = 2 * sizeof(ProfileEntry);

I am not sure to understand why this function is needed, and why we have a 2-frame offset.

@@ +1915,5 @@
> +
> +    // Test if SPS is enabled (i.e., if spsMarkJit pushed an SPS frame).
> +    movePtr(ImmPtr(enabledAddr), temp);
> +    load32(Address(temp, 0), temp);
> +    branch32(Assembler::Equal, temp, Imm32(0), &skip);

nit: and here.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> I am not sure to understand the OSR part in generateEntrerJit, and I failed
> to understand from where the "2*" is coming from.

This diagram might help:

// SPS stack: | ... ...  ... ... |
//            +------------------+
//            | theFunction      |  sp-2 (pushed/popped by interpreter)
//            +------------------+
//            | EnterJIT         |  sp-1 (pushed/popped by trampoline)
//            +------------------+
//            | theFunction copy |  sp   (pushed by this function;
//            +------------------+        popped by Baseline jitcode)

...but, now that I think about this, there's an edge case that I'm not handling correctly.  If we have profiling enabled and then call a function that disables profiling and enters a loop that is OSRed, we'll have an SPS frame for the function but no enterJIT, so we'll need to duplicate the top stack frame, thusly:

// SPS stack: | ... ...  ... ... |
//            +------------------+
//            | theFunction      |  sp-1 (pushed/popped by interpreter)
//            +------------------+
//            | theFunction copy |  sp   (pushed by this function;
//            +------------------+        popped by Baseline jitcode)

Or else handle that case some other way, like by clearing the HAS_PUSHED_SPS_FRAME flag in the Baseline frame.

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Jed Davis [:jld] from comment #14)
> ...but, now that I think about this, there's an edge case that I'm not
> handling correctly.  If we have profiling enabled and then call a function
> that disables profiling and enters a loop that is OSRed, we'll have an SPS
> frame for the function but no enterJIT, so we'll need to duplicate the top
> stack frame, thusly:

Disabling profiling by calling SPSProfiler::enable(false) invalidates all jitcode, including cancelling off-thread compilations and baseline code, see http://dxr.mozilla.org/mozilla-central/source/js/src/vm/SPSProfiler.cpp#52 . Is this still a concern with that in mind?

Comment 16

[Kannan Vijayan [:djvj]]

(In reply to Jed Davis [:jld] from comment #14)
> Or else handle that case some other way, like by clearing the
> HAS_PUSHED_SPS_FRAME flag in the Baseline frame.

Close.  See EnsureCanEnterIon in BaselineIC.cpp, towards the end in the conditional |if (isLoopEntry)|.

For transitions between SPS being enabled and not when moving from Baseline to Ion, if the Baseline frame's SPS state doesn't correspond to the compiled Ion code's SPS instrumentation, then we simply don't enter Ion.

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #15)
> (In reply to Jed Davis [:jld] from comment #14)
> > ...but, now that I think about this, there's an edge case that I'm not
> > handling correctly.  If we have profiling enabled and then call a function
> > that disables profiling and enters a loop that is OSRed, we'll have an SPS
> > frame for the function but no enterJIT, so we'll need to duplicate the top
> > stack frame, thusly:
> 
> Disabling profiling by calling SPSProfiler::enable(false) invalidates all
> jitcode, including cancelling off-thread compilations and baseline code, see
> http://dxr.mozilla.org/mozilla-central/source/js/src/vm/SPSProfiler.cpp#52 .
> Is this still a concern with that in mind?

Yes.  (And I thought Baseline code wasn't specialized on profiler state, but that doesn't actually matter here.)

A test case, for xpcshell, is basically:

var p = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
p.StartProfiler(100, 10, ["js"], 1);
(function (){
  p.StopProfiler();
  var n = 10000;
  while (--n);
})();

And that fails an assertion with the patch I posted.  (The initial value of |n| could possibly be smaller; it just needs to be enough to enter Baseline.)

(In reply to Kannan Vijayan [:djvj] from comment #16)
> (In reply to Jed Davis [:jld] from comment #14)
> > Or else handle that case some other way, like by clearing the
> > HAS_PUSHED_SPS_FRAME flag in the Baseline frame.

…and this fixes that assertion failure.

> Close.  See EnsureCanEnterIon in BaselineIC.cpp, towards the end in the
> conditional |if (isLoopEntry)|.
> 
> For transitions between SPS being enabled and not when moving from Baseline
> to Ion, if the Baseline frame's SPS state doesn't correspond to the compiled
> Ion code's SPS instrumentation, then we simply don't enter Ion.

This is about moving from the interpreter to Baseline.  The Baseline<->Ion cases seem to work as-is.

Comment 18

[Kannan Vijayan [:djvj]]

(In reply to Jed Davis [:jld] from comment #17)
> This is about moving from the interpreter to Baseline.  The Baseline<->Ion
> cases seem to work as-is.

Ah, sorry about that.  You're right - in that case we simply clear the HAS_PUSHED_SPS_FRAME flag on the baseline frame when OSR-ing into it.

See BaselineFrame::initForOsr in jit/BaselineFrame.cpp

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 8363284 [details] [diff] [review]
bug914561-p1-sps-enterjit-ehabi-hg0.diff

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

r- for now until the comments have been discussed.

::: tools/profiler/TableTicker.cpp
@@ +493,5 @@
> +
> +  array.count = 0;
> +  for (uint32_t i = pseudoStack->stackSize(); i > 0; --i) {
> +    volatile StackEntry &entry = pseudoStack->mStack[i - 1];
> +    if (!entry.js() && 0 == strcmp(entry.label(), "EnterJIT")) {

We don't use the '0 == cond' style in the profiler.

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> Also, can you add either as part of this patch or as part of another patch a
> way to toggle the sps flag from the testing functions?  You can add it as
> part of JS_SetGlobalJitCompilerOption.

We already have enableSPSProfilingAssertions and disableSPSProfiling, which do this, and which a few of the JIT tests use.

What we don't do is expose the contents of that profile buffer.

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Benoit Girard (:BenWa) from comment #12)
[...]
> > +  for (uint32_t i = pseudoStack->stackSize(); i > 0; --i) {
[...]
> Are you building pseudoStack in the right order? I believe it's expected to
> be current PC working backwards to the main function.

Yes; this is iterating the pseudostack from high indices (callee/leaf) to low indices (caller/main).

> @@ +518,5 @@
> > +    }
> > +  }
> > +
> > +  // Now unwind whatever's left.
> > +  array.count += EHABIStackWalk(*mcontext, aProfile.GetStackTop(),
> 
> This code could in theory unwind past a JIT block I believe? If this stack
> unwind isn't strictly increasing then the merging code could fail (or maybe
> deadlock?).
>
> Can you confirm that the stack is in the right order and that it will always
> be going up the stack (towards main)?

It's nondecreasing.  For the first frame unwound, a leaf function that doesn't save any registers and just does `bx lr` won't change the stack pointer.  Otherwise it's strictly increasing unless the EH tables are malicious (there's a case I can think of that would leave SP unchanged and avoids the infinite loop countermeasure from bug 916106, but I can't see any legitimate reason for a compiler to generate it) and that should be a safe assumption.

I'll make the code in the unwinder that enforces that a little clearer, as well as adding more comments in TableTicker.cpp.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p0-sps-enterjit-mark-hg1.diff

With fixes, more comments, and tests.

Comment 23

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p1-sps-enterjit-ehabi-hg1.diff

With more comments.

Comment 24

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

There's a slight conflict with bug 841646, but I think the fix is just to remove the "cx, " from the Interpreter.cpp change.

Comment 25

[Benoit Girard (:BenWa)]

Comment on attachment 8368313 [details] [diff] [review]
bug914561-p1-sps-enterjit-ehabi-hg1.diff

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

::: tools/profiler/TableTicker.cpp
@@ +491,5 @@
> +  mcontext_t savedContext;
> +  PseudoStack *pseudoStack = aProfile.GetPseudoStack();
> +
> +  array.count = 0;
> +  // The pseudostack contains an "EnterJIT" marker whenever we enter

s/marker/pseudo stack entry - I want to keep the terminology clear to avoid confusion.

Comment 26

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Jed Davis [:jld] from comment #20)
> (In reply to Nicolas B. Pierron [:nbp] from comment #13)
> > Also, can you add either as part of this patch or as part of another patch a
> > way to toggle the sps flag from the testing functions?  You can add it as
> > part of JS_SetGlobalJitCompilerOption.
> 
> We already have enableSPSProfilingAssertions and disableSPSProfiling, which
> do this, and which a few of the JIT tests use.

Ok, but we need to increase the number of tests cases using these functions, such as switching it to true while in a loop which would be OSR, or after the OSR.  And also mix these functions within the same loop.

> What we don't do is expose the contents of that profile buffer.

I think we should do that, especially since we are now able to enforce the fact that a function should compile after X iterations with JS_SetGlobalJitCompilerOption.

Comment 27

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8368311 [details] [diff] [review]
bug914561-p0-sps-enterjit-mark-hg1.diff

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

Add a test case such as:

while (n--) {
  disableSPSProfiling();
  if (!n) return;
  enableSPSProfilingAssertions(true);
}

and the opposite, to make sure that we do not attempt to pop something which is not pushed:

while (n--) {
  enableSPSProfilingAssertions(true);
  if (!n) return;
  disableSPSProfiling();
}

::: js/src/jit-test/tests/profiler/enterjit-osr-disabling.js
@@ +1,1 @@
> +enableSPSProfilingAssertions(false);

why "false"? We always want to assert like crazy. In fact why do we have 2 modes of assertions levels?

@@ +1,5 @@
> +enableSPSProfilingAssertions(false);
> +(function() {
> +   disableSPSProfiling();
> +   var n = 10000;
> +   while (n--);

We no longer need to iterate as much, we can change the number of iteration before a compilation:

setJitCompilerOption("baseline.usecount.trigger", 10);
setJitCompilerOption("ion.usecount.trigger", 20);

::: js/src/jit/IonMacroAssembler.cpp
@@ +1860,5 @@
> +// Creates an enterJIT pseudostack frame, as described above.  Pushes
> +// a word to the stack to indicate whether this was done.  |framePtr| is
> +// the pointer to the machine-dependent saved state.
> +void
> +MacroAssembler::spsMarkJit(SPSProfiler *p, Register framePtr, Register temp) {

style-nit: This file style is to add the open brace on a new line after the function prototype.
         & same thing for all other functions.

@@ +1864,5 @@
> +MacroAssembler::spsMarkJit(SPSProfiler *p, Register framePtr, Register temp) {
> +    Label spsNotEnabled;
> +    uint32_t *enabledAddr = p->addressOfEnabled();
> +    movePtr(ImmPtr(enabledAddr), temp);
> +    load32(Address(temp, 0), temp);

replace these 2 instructions by:

  load32(AbsoluteAddress(enableAddr), temp);

@@ +1881,5 @@
> +
> +    /* Always increment the stack size, whether or not we actually pushed. */
> +    bind(&stackFull);
> +    movePtr(ImmPtr(p->addressOfSizePointer()), temp);
> +    loadPtr(Address(temp, 0), temp);

replace these 2 instructions by:

  loadPtr(AbsoluteAddress(p->addressOfSizePointer()), temp);

@@ +1892,5 @@
> +// frame, pops it.
> +void
> +MacroAssembler::spsUnmarkJit(SPSProfiler *p, Register temp) {
> +    Label spsNotEnabled;
> +    pop(temp); // +4: Was the profiler enabled.

nit: s/+4/-4/

@@ +1912,5 @@
> +// SPS stack: | ... ...  ... ... |
> +//            +------------------+
> +//            | theFunction      |  sp-2 (pushed/popped by interpreter)
> +//            +------------------+
> +//            | EnterJIT         |  sp-1 (pushed/popped by trampoline)

nit: s/trampoline/OSR trampoline/

@@ +1917,5 @@
> +//            +------------------+
> +//            | theFunction copy |  sp   (pushed by this function;
> +//            +------------------+        popped by Baseline jitcode)
> +void
> +MacroAssembler::spsFixupOsr(SPSProfiler *p, Register framePtr, Register temp, Register temp2) {

Rewrite this function in C++ and call it from BaselineFrame::initForOsr.

@@ +1925,5 @@
> +
> +    // Test if the function pushed an SPS frame; if not, there's nothing to do.
> +    branchTest32(Assembler::Zero,
> +                 Address(framePtr, BaselineFrame::reverseOffsetOfFlags()),
> +                 Imm32(BaselineFrame::HAS_PUSHED_SPS_FRAME),

Improve the comment:
  // Test if the *interpreter* psuhed an SPS frame.

  // This check is made on the flags of the BaselineFrame as the HAS_PUSHED_SPS_FRAME flag is copied over from the StackFrame in BaselineFrame::initForOsr.

@@ +1932,5 @@
> +    // Test if SPS is currently enabled (i.e., if spsMarkJit pushed an
> +    // SPS frame).  Special handling is required if not; see below.
> +    movePtr(ImmPtr(enabledAddr), temp);
> +    load32(Address(temp, 0), temp);
> +    branchTest32(Assembler::Zero, temp, temp, &spsDisabledBeforeOsr);

This test should be added as part of the condition of initForOsr before copying the HAS_PUSHED_SPS_FRAME, such as the HAS_PUSHED_SPS_FRAME clearly reflect its actual meaning.  This would be better than clearing it after.

::: tools/profiler/tests/test_enterjit_osr.js
@@ +22,5 @@
> +	let then = Date.now();
> +	do {
> +	    let n = 10000;
> +	    while (--n); // OSR happens here
> +	    // Spin until we're sure we have a sample.

You should also be able to use the testing functions here.

::: tools/profiler/tests/xpcshell.ini
@@ +9,5 @@
>  [test_run.js]
>  skip-if = true
> +[test_enterjit_osr.js]
> +[test_enterjit_osr_disabling.js]
> +skip-if = !debug

Any reason to avoid running these test if they are not in debug builds?  AFAIK testing functions are always available.

Comment 28

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

A few things where I have more comments than just "thanks; will fix":

(In reply to Nicolas B. Pierron [:nbp] from comment #27)
> ::: js/src/jit-test/tests/profiler/enterjit-osr-disabling.js
> @@ +1,1 @@
> > +enableSPSProfilingAssertions(false);
> 
> why "false"? We always want to assert like crazy. In fact why do we have 2
> modes of assertions levels?

I have no idea.  I was following the example of the other tests I saw.

> @@ +1917,5 @@
> > +//            +------------------+
> > +//            | theFunction copy |  sp   (pushed by this function;
> > +//            +------------------+        popped by Baseline jitcode)
> > +void
> > +MacroAssembler::spsFixupOsr(SPSProfiler *p, Register framePtr, Register temp, Register temp2) {
> 
> Rewrite this function in C++ and call it from BaselineFrame::initForOsr.

BaselineFrame::initForOsr turns out to be a much better place to do this.  Because we have the script/function from the StackFrame, there's no need to do Forth-style stack gymnastics on the pseudostack, and it turns into something like this:

         SPSProfiler *p = &(cx->runtime()->spsProfiler);
         p->enter(fp->script(), fp->maybeFun());

Also, this way we can push an SPS frame for Baseline even if the interpreter didn't, which means that if we have a function that turns on profiling in the interpreter and then starts looping, it's eligible for Baseline→Ion OSR.

> ::: tools/profiler/tests/xpcshell.ini
> @@ +9,5 @@
> >  [test_run.js]
> >  skip-if = true
> > +[test_enterjit_osr.js]
> > +[test_enterjit_osr_disabling.js]
> > +skip-if = !debug
> 
> Any reason to avoid running these test if they are not in debug builds? 
> AFAIK testing functions are always available.

The rationale was that the only way the test could fail was by causing a debug-only assertion failure, so it wouldn't do anything useful on non-debug builds.

Comment 29

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p0-sps-enterjit-mark-hg2.diff

Comment 30

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p1-sps-enterjit-ehabi-hg2.diff

Fixed misuse of terminology in comment, as indicated in comment #25.  Carrying over r+.

Comment 31

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8393859 [details] [diff] [review]
bug914561-p0-sps-enterjit-mark-hg2.diff

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

::: js/src/jit/BaselineFrame.cpp
@@ +179,5 @@
>  
>      if (fp->hasReturnValue())
>          setReturnValue(fp->returnValue());
>  
> +    if (fp->hasPushedSPSFrame()) {

Can we remove this condition?  (I've confirmed that it doesn't break any tests, but that doesn't necessarily mean that it's the right thing to do.)

@@ +188,5 @@
> +        // Baseline frame to have the SPS flag set, it must have its own SPS
> +        // frame, which the Baseline code will pop on return.
> +        if (p->enabled()) {
> +            p->enter(fp->script(), fp->maybeFun());
> +            flags_ |= BaselineFrame::HAS_PUSHED_SPS_FRAME;

I've confirmed that some of the tests in this patch break if we just set the flag without pushing a pseudostack frame here.

Comment 32

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8393859 [details] [diff] [review]
bug914561-p0-sps-enterjit-mark-hg2.diff

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

This patch is better and easier to understand :)
Nice work!

::: js/src/jit/BaselineFrame.cpp
@@ +179,5 @@
>  
>      if (fp->hasReturnValue())
>          setReturnValue(fp->returnValue());
>  
> +    if (fp->hasPushedSPSFrame()) {

Yes, we can remove this condition.

Comment 33

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p0-sps-enterjit-mark-hg3.diff

Rebased; comment #32 applied; carrying over r+.

Trying: https://tbpl.mozilla.org/?tree=Try&rev=794fda2a1d0f

Comment 34

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Nicolas B. Pierron [:nbp] from comment #27)
> @@ +1864,5 @@
> > +MacroAssembler::spsMarkJit(SPSProfiler *p, Register framePtr, Register temp) {
> > +    Label spsNotEnabled;
> > +    uint32_t *enabledAddr = p->addressOfEnabled();
> > +    movePtr(ImmPtr(enabledAddr), temp);
> > +    load32(Address(temp, 0), temp);
> 
> replace these 2 instructions by:
> 
>   load32(AbsoluteAddress(enableAddr), temp);

Slight problem: that doesn't exist on x86/x86_64.  There's a load32 that takes a const void* and a Register for those two architectures, and they seem to do the right thing, but they're defined on a different kind of MacroAssembler class than where the AbsoluteAddress load32 is defined on ARM, and I'm having trouble following the inheritance hierarchy.  Suggestions?

Comment 35

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-fix-masm-load32.diff

I think I've figured this out.  Tested locally on x64, and tried: https://tbpl.mozilla.org/?tree=Try&rev=b5f91b0249df

Comment 36

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8395149 [details] [diff] [review]
bug914561-fix-masm-load32.diff

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

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +684,5 @@
> +        if (JSC::X86Assembler::isAddressImmediate(address.addr)) {
> +            movl(Operand(address), dest);
> +        } else {
> +            mov(ImmPtr(address.addr), ScratchReg);
> +            movl(Operand(ScratchReg, 0x0), dest);

nit: use load32(Address(ScratchReg, 0), dest)

and same thing for the 3 others.

Comment 37

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-fix-masm-load32.diff

Revised as suggested; carrying over r+.

Comment 38

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug914561-p0-sps-enterjit-mark-hg4.diff

Rebase over trivial context change.

Comment 39

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42a241a931a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/16ae1500d5ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9dc1a7d4649

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42a241a931a0
https://hg.mozilla.org/mozilla-central/rev/16ae1500d5ac
https://hg.mozilla.org/mozilla-central/rev/d9dc1a7d4649