IonMonkey: Provide line number information to SPS

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: u443197, Assigned: u443197)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ion:t])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
This has been done with JaegerMonkey in bug 778979, now it should be done with IonMonkey
Whiteboard: [ion:t]
(Assignee)

Comment 1

5 years ago
Created attachment 652229 [details] [diff] [review]
Part 1: Move SPSInstrumentation to a template and rework the interface
(Assignee)

Comment 2

5 years ago
Created attachment 652230 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed
(Assignee)

Updated

5 years ago
Depends on: 736299
(Assignee)

Comment 3

5 years ago
Turns out SPS is dying while stack walking because %rbp is getting trashed, hence the dependence on 736299. I tested out SPS with stack walking disabled, and assertions aren't being tripped left and right (or at all!).
(Assignee)

Comment 4

5 years ago
Created attachment 652237 [details] [diff] [review]
Part 3: Don't die on erroneous frame pointers when stack walking
Attachment #652237 - Flags: review?(bgirard)
(Assignee)

Updated

5 years ago
No longer depends on: 736299
(Assignee)

Updated

5 years ago
Attachment #652229 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Attachment #652237 - Attachment is obsolete: true
Attachment #652237 - Flags: review?(bgirard)
(Assignee)

Comment 5

5 years ago
After discussion with BenWa and looking at a few examples, we decided it's probably best to actually respect the frame pointer instead of just hoping sps can deal with it.
Depends on: 736299
Attachment #652229 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

5 years ago
Attachment #652230 - Flags: review?(nicolas.b.pierron)
Comment on attachment 652230 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed

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

1/ Decouple the MASM from the profiling, at best add an assertion which ensure that we are profiling calls correctly.
2/ Replace LCallBoundary by an SPSCallBoundary class which use a constructor and a destructor to update the profiler stack around a call.
3/ Ensure that you are only using the call boundary once per call.
3/ VM functions calls are not covered by the current mechanism and the handleException of the VM wrapper will not pop all frames from the profiler stack.

Nice approach and nice work, but still this plenty of corner cases to fix before being acceptable.

::: js/src/ion/IonMacroAssembler.h
@@ +31,1 @@
>   

nit: remove trailing whitespace.

@@ +482,5 @@
> +    void callWithABI(void *fun, Result result = GENERAL) {
> +        // There aren't any guaranteed non-live registers here, so if we need to
> +        // use one for instrumentation it must be pushed/poppped around the
> +        // instrumentation.
> +        if (sps) {

In general I would prefer to avoid such instrumentation in the macro assembler, but doing otherwise sounds more error prone because we can easily forget to add the call boundary.

Another solution to prevent such instrumentation while not forgetting to add the call boundary would be to add an assertion to ensure that any profiled call is under a call scope.

@@ +485,5 @@
> +        // instrumentation.
> +        if (sps) {
> +            reserveStack(STACK_SLOT_SIZE);
> +            storePtr(CallTempReg0, Address(StackPointer, 0));
> +            sps->leave(*pc_, *this, CallTempReg0);

nit: See comment below.  You can do such instrumentation by adding a class with a constructor and destructor to produce this pieces of assembly.  Also you might want to assert that no other Call scope is around a callWithABI.

@@ +490,5 @@
> +            loadPtr(Address(StackPointer, 0), CallTempReg0);
> +            freeStack(STACK_SLOT_SIZE);
> +        }
> +
> +        archCallWithABI(fun, result);

nit:  Don't rename the symbols, just use  MacroAssemblerSpecific::callWithABI.

@@ +495,5 @@
> +
> +        if (sps) {
> +            reserveStack(STACK_SLOT_SIZE);
> +            storePtr(CallTempReg0, Address(StackPointer, 0));
> +            sps->reenter(*this, CallTempReg0);

nit: After a call you can use any register which is not used for returning a value from the call.  Which means not ReturnReg, and not JS_ReturnReg*.

@@ +506,5 @@
> +        if (sps) {
> +            // We're going to leave this function, so there's no need to emit
> +            // reentry instrumentation.
> +            sps->skipNextReenter();
> +            IonInstrumentation::CallScope cs(sps, *pc_, *this, CallTempReg2);

CallTempReg2 ?
Is this code removing all frames of the current Ion activation ?

@@ +510,5 @@
> +            IonInstrumentation::CallScope cs(sps, *pc_, *this, CallTempReg2);
> +            archHandleException();
> +        } else {
> +            archHandleException();
> +        }

nit:

    …
    IonInstrumentation::CallScope cs(sps, *pc_, *this, CallTempReg2);
}
MacroAssemblerSpecific::handleException()

::: js/src/ion/Lowering.cpp
@@ +225,5 @@
>      uint32 argslot = getArgumentSlotForCall();
>      JSFunction *target = call->getSingleTarget();
>  
> +    if (gen->instrumentedProfiling() &&
> +        !add(new LCallBoundary(tempFixed(CallTempReg0), true), call))

We usually produce only one LIR node for each MIR node.  I don't think you need to handle this with a LIR instruction.  A call boundary can be instrumented with a simple class which create the leave at the construction and the reenter at the destruction, such as:

{
  CallBoundary(sps, &temp, call);

  … calling code …
}

@@ +232,3 @@
>      if (target && target->isNative()) {
>          if (call->isDOMFunction()) {
>              LCallDOMNative *lir = new LCallDOMNative(argslot,

This LIR is using callWithABI to implement it's function call, by creating the CallBoundaries instead of using a CallBoundary class, you will create 2 call boundaries where only one is needed.

@@ +274,5 @@
>      freeArguments(call->numStackArgs());
> +
> +    if (gen->instrumentedProfiling() &&
> +        !add(new LCallBoundary(tempFixed(CallTempReg0), false), call))
> +        return false;

nit: same here.

::: js/src/ion/MIR.h
@@ +282,5 @@
>      static void PrintOpcodeName(FILE *fp, Opcode op);
>      virtual void printOpcode(FILE *fp);
>  
> +    void setTrackedPc(jsbytecode *pc) {
> +        trackedPc_ = pc;

Why removing the check which ensure that we do not define the trackedPc multiple times ?

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +139,1 @@
>      

nit: remove trailing whitespace.
Attachment #652230 - Flags: review?(nicolas.b.pierron)
Comment on attachment 652230 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed

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

Additional Re-review:
- correction: CallVM are handled correctly with the CallScope function.
- Modifications made in ion::HandleException (IonFrames.cpp) to pop SPS stack, seen on Alex screen, are not present in this patch.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +347,5 @@
>      IonCode *wrapper = ion->generateVMWrapper(cx, fun);
>      if (!wrapper)
>          return false;
>  
> +    IonInstrumentation::CallScope cs(&sps, lastPC, masm, CallTempReg2);

This line register the last registered PC into the last SPS entry for all CallVM.  So call to VM function are handled as opposed as my previous comment.  The only detail is that this callVM function might be call inside an instruction such as visitCallGeneric which is already wrapped.
(Assignee)

Comment 8

5 years ago
Created attachment 653873 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (v2)

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #6)

> Another solution to prevent such instrumentation while not forgetting to add
> the call boundary would be to add an assertion to ensure that any profiled
> call is under a call scope.

The assertion in place, and work has been done to make sure it doesn't trip.
This is for both callWithABI and handleException.

> nit:  Don't rename the symbols, just use 
> MacroAssemblerSpecific::callWithABI.

I decided against this because then each specific assembler would have to have a constructor which takes the instrumentation as an argument which seemed like unnecessary duplication.

> We usually produce only one LIR node for each MIR node.  I don't think you
> need to handle this with a LIR instruction.  A call boundary can be
> instrumented with a simple class which create the leave at the construction
> and the reenter at the destruction, such as:
> 
> {
>   CallBoundary(sps, &temp, call);
> 
>   … calling code …
> }

The LCallBoundary class no longer exists.

> Why removing the check which ensure that we do not define the trackedPc
> multiple times ?

When LICM moved code around, I had to always make sure that the listed pc for an instruction always fell within range of the current script's code. This check prevented this when code was moved, so I removed it.


Overall, this patch has a lot more 'IonInstrumentation::CallScope' objects all over the place, and I was running into some crashes when running in the browser of the pc index being wildly out of bounds (turned out to be OOL instructions). I put in some extra work to make sure that the 'pc' of an OOL instruction is sane.

This also has the fix nbp and I talked about with MPolyInlineDispatch nodes with a fallback by adding a new basic block and some extra phis.
Attachment #652230 - Attachment is obsolete: true
Attachment #653873 - Flags: review?(nicolas.b.pierron)
Comment on attachment 653873 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (v2)

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

Sorry, I did not noticed before, but CallScope is a nice C++ trick but as briefly commented here it is error prone because the register given at the creation of the callScope is used implicitly when the callScope is destructed and this does not show up in the code generator.  As you told me in person, this sounded to be fine in the test suite which means that errors I caught by reviewing are effort were not catch by the test suite, and I cannot accept such things knowing that this is not obvious to track.

So you should instrument the masm in IonMacroAssembler, even if I don't like this solution, by overwrite the function and calling the parent class methods with the explicit name of the parent class.  Keep the code minimal in the overwritten version, such as:

  void callWithABI(…) {
    SampleProfiler::leaveJS(&sps, pc);
    MacroAssemblerSpecific::callWithABI(…);
    SampleProfiler::reenterJS(&sps, pc, unusedAfterCallReg);
  };

::: js/src/ion/IonMacroAssembler.h
@@ +477,5 @@
> +
> +    // Emits a call to a C/C++ function, resolving all argument moves.
> +    void callWithABI(void *fun, Result result = GENERAL) {
> +        JS_ASSERT_IF(sps && sps->enabled(), sps->left());
> +        archCallWithABI(fun, result);

nit: Do not use the arch prefix, use MacroAssemblerSpecific:: instead.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +358,5 @@
>      IonCode *wrapper = ion->generateVMWrapper(cx, fun);
>      if (!wrapper)
>          return false;
>  
> +    IonInstrumentation::CallScope cs(&sps, lastPC, masm, CallTempReg2);

Please use/create another static const which is by definition differnt than the JS_ReturnReg / ReturnReg

@@ +427,5 @@
> +    // Normally saveVolatile would be used, but the instrumentation needs to get
> +    // some register so instead just keep track of the relevant registers
> +    RegisterSet regs = RegisterSet::Volatile();
> +    regs.maybeTake(dest);
> +    masm.PushRegsInMask(regs);

It seems like you just want to make saveVolatile return one of the unused registers by calling getAny inside the saveVolatile.

@@ +439,1 @@
>      masm.storeCallResult(dest);

move storeCallResult inside the previous scope because the C return register is a volatile register which might be the register returned by .getAny().

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +1277,1 @@
>          masm.storeCallResult(output);

same here.

::: js/src/ion/x86/MacroAssembler-x86.cpp
@@ +139,1 @@
>      

nit: trailing whitespace.
Attachment #653873 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 10

5 years ago
Created attachment 654209 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (v3)

I might have overlooked a thing here or there, but this is the bulk of the changes minus future minor tweaks. It failed 32-bit try last night before I added some extra push logic in reenterAfterCall(), and I'm running that through try again.
Attachment #653873 - Attachment is obsolete: true
Attachment #654209 - Flags: review?(nicolas.b.pierron)
Comment on attachment 654209 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (v3)

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

Apply nits and go to land.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +210,5 @@
>  IonCompartment::generateArgumentsRectifier(JSContext *cx)
>  {
> +    // Don't use rbp for this function because if profiling is enabled it will
> +    // cause stack walkers to get very confused.
> +

nit: // Do not erase the frame pointer in this function.

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +212,5 @@
>  IonCompartment::generateArgumentsRectifier(JSContext *cx)
>  {
> +    // Don't use ebp for this function because if profiling is enabled it will
> +    // cause stack walkers to get very confused.
> +

nit: Remove this comment since you are using it your-self.  Then you might want to rename ebp to FramePointer to ensure no-body erase it.

@@ +270,5 @@
>          masm.j(Assembler::NonZero, &copyLoopTop);
>      }
>  
>      // Construct descriptor.
> +    masm.movl(ebp, ebx);

masm.lea(Address(ebp, 4), ebx); // to account for the frame pointer in the frame size.
Attachment #654209 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

Accidentally uploaded the complete wrong patch previously, but I will also take those into account!
Attachment #654209 - Attachment is obsolete: true
Attachment #654269 - Flags: review?(nicolas.b.pierron)
Comment on attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

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

This patch sounds good, except that modifications made to HandleException are still lacking in this patch — hg queues issues ?
Apply nits, include IonFrames.cpp modifications in this patch, ask :djvj to review IonBuilder.cpp and ask me again to review this patch with HandleException modifications.

::: js/src/ion/IonMacroAssembler.h
@@ +495,5 @@
> +        if (sps)
> +            sps->skipNextReenter();
> +        leaveBeforeCall();
> +        MacroAssemblerSpecific::handleException();
> +        reenterAfterCall();

Is the reenter needed after all, because this instruction will still produce some assembly.

@@ +559,5 @@
> +            return;
> +        GeneralRegisterSet regs(Registers::TempMask);
> +        Register r = regs.getAny();
> +        reserveStack(sizeof(void*));
> +        storePtr(r, Address(StackPointer, 0));

nit: see below.

@@ +562,5 @@
> +        reserveStack(sizeof(void*));
> +        storePtr(r, Address(StackPointer, 0));
> +        sps->leave(*pc_, *this, r);
> +        loadPtr(Address(StackPointer, 0), r);
> +        freeStack(sizeof(void*));

nit: see below.

@@ +574,5 @@
> +        if (regs.empty()) {
> +            regs = GeneralRegisterSet(Registers::TempMask);
> +            Register r = regs.getAny();
> +            reserveStack(sizeof(void*));
> +            storePtr(r, Address(StackPointer, 0));

nit: use push instead of reserveStack & storePtr.

@@ +577,5 @@
> +            reserveStack(sizeof(void*));
> +            storePtr(r, Address(StackPointer, 0));
> +            sps->reenter(*this, r);
> +            loadPtr(Address(StackPointer, 0), r);
> +            freeStack(sizeof(void*));

nit: and pop instead of loadPtr & freeStack.
Attachment #654269 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #13)
> Comment on attachment 654269 [details] [diff] [review]
> Part 2: Bring IonMonkey back up to speed (actual v3)
> 
> Review of attachment 654269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch sounds good, except that modifications made to HandleException
> are still lacking in this patch — hg queues issues ?
> Apply nits, include IonFrames.cpp modifications in this patch, ask :djvj to
> review IonBuilder.cpp and ask me again to review this patch with
> HandleException modifications.

Ok, my mistake, HandleException modifications are already in trunk.
Attachment #654269 - Flags: review+
Comment on attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

djvj, I'm forwarding the review of the inlining logic (ionBuilder.cpp), in case I miss something.
Attachment #654269 - Flags: review?(kvijayan)
Comment on attachment 654269 [details] [diff] [review]
Part 2: Bring IonMonkey back up to speed (actual v3)

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

The IonBuilder.cpp changes look good.  Good work.
Attachment #654269 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 17

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/2d190481b4a4
http://hg.mozilla.org/projects/ionmonkey/rev/01c69de69dc3
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
(Assignee)

Updated

5 years ago
Blocks: 785234

Comment 18

5 years ago
Resolved Fixed, not Worksforme, right?
(Assignee)

Comment 19

5 years ago
I apparently no longer have the option of FIXED, so I chose the next best one. Not sure why I lost it?
(Assignee)

Updated

5 years ago
Resolution: WORKSFORME → FIXED
Depends on: 791138
You need to log in before you can comment on or make changes to this bug.