IonMonkey: Add a frame pointer for profiling & debugging mode.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: u443197)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p2][ion:t])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
The idea is to kill a register which will then be used to *fake* a frame pointer (possibly the same as the compiler used for compiling the JS engine) when profiling/debugging mode is enabled.  We will certainly lose performances, especially on x86, but this seems like a small addition to IonMonkey which can simplify the integration with the profiler.

Doing so implies that, we should discard all the code when the frame-pointer mode is enabled.  The term *fake* above is used to highlight that we have no will to use it instead of the frame descriptor which already exists and that we hope that this feature won't last long after the integration of the profiler and IonMonkey.

PS: Profiling & Debugging are about the C++ & JS, not only about JS.
Once patches are posted I can create a custom build with the profiler to give early feedback.
Whiteboard: [snappy]

Updated

5 years ago
Whiteboard: [snappy] → [snappy:p2]

Updated

5 years ago
Blocks: 761253
(Assignee)

Comment 2

5 years ago
I'll be working on profiling in general, and this looks highly relevant to what I'll be doing soon.
Assignee: general → acrichton
(Assignee)

Comment 3

5 years ago
Created attachment 631589 [details] [diff] [review]
v1 diff

Currently hidden behind the --enable-profiling flags, but still passes all tests.
(Reporter)

Updated

5 years ago
Duplicate of this bug: 774767
(Reporter)

Comment 5

5 years ago
Comment on attachment 631589 [details] [diff] [review]
v1 diff

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

This sounds like a good start, You might ask dvander about his patch to profile with vtune.
And you will also need to instrument the visitStart function in the code generator to produce the prologue of the function.

::: js/src/ion/arm/Architecture-arm.h
@@ +152,5 @@
>      // possibly the stack as well.
>      static const uint32 NonAllocatableMask =
> +#if defined(MOZ_PROFILING)
> +        (1 << Registers::r11) |
> +#endif

Do we use the frame pointer to walk on ARM frames?
(Assignee)

Updated

5 years ago
Depends on: 775782
Whiteboard: [snappy:p2] → [snappy:p2][ion:t]
(Assignee)

Updated

5 years ago
Blocks: 781979
(Assignee)

Updated

5 years ago
No longer blocks: 781979
(Assignee)

Updated

5 years ago
Blocks: 781979
(Assignee)

Comment 6

5 years ago
Created attachment 652271 [details] [diff] [review]
v2 - enable/disable at runtime

In order to make this useful for SPS, it can't be a compile-time constant whether to allocate the frame pointer register or not. The decision has to be made at runtime.

I looked for all of the places that the allocatable-ness of %ebp/%rbp can be leaked out and it turns out there's not a lot. Conditionally takes the register away from all allocatable registers if necessary. On ARM it doesn't do anything regardless of whether profiling is on or not.

This depends on code from bug 781979, so I'll be landing both of these bugs together when the time comes.
Attachment #631589 - Attachment is obsolete: true
Attachment #652271 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 7

5 years ago
Comment on attachment 652271 [details] [diff] [review]
v2 - enable/disable at runtime

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

This patch save the frame pointer by avoiding the allocation of the FramePointer.  Unfortunately, it does not handle cases where the frame pointer is erased by some pieces of code inlining directly registers such as Trampoline functions.

Good modification, but you need fix the trampoline functions on x86/x64 before to avoid having any bad frame pointer.

::: js/src/ion/LinearScan.cpp
@@ +522,5 @@
>          // point of definition, if found.
>          for (LInstructionReverseIterator ins = block->rbegin(); ins != block->rend(); ins++) {
>              // Calls may clobber registers, so force a spill and reload around the callsite.
>              if (ins->isCall()) {
> +                for (AnyRegisterIterator iter(allRegisters()); iter.more(); iter++) {

nit: allRegisters will always return the same result for one compilation.  You don't want to compute it everytime in an inner loop.

@@ +1620,5 @@
>  
>      // Compute free-until positions for all registers
>      CodePosition freeUntilPos[AnyRegister::Total];
>      bool needFloat = current->reg()->isDouble();
> +    for (AnyRegisterIterator regs(allRegisters()); regs.more(); regs++) {

nit: This loop should be splitted based on the needFloat and iterate on typedRegisters instead of AnyRegister.
nit: Make sure allRegisters is not re-computed everytime we enter this function.

@@ +1713,5 @@
>  
>      // Compute next-used positions for all registers
>      CodePosition nextUsePos[AnyRegister::Total];
>      bool needFloat = current->reg()->isDouble();
> +    for (AnyRegisterIterator regs(allRegisters()); regs.more(); regs++) {

nit: same here.

@@ +2186,5 @@
>      return result;
>  }
> +
> +RegisterSet
> +LinearScanAllocator::allRegisters()

nit: Don't you need a "const" ?
nit: Add "inline" keyword.

@@ +2189,5 @@
> +RegisterSet
> +LinearScanAllocator::allRegisters()
> +{
> +    RegisterSet regs = RegisterSet::All();
> +    if (lir->mir()->instrumentedProfiling() && FramePointer != InvalidReg)

nit: Change condition order.

       FramePointer != InvalidReg && lir->mir()->instrumentedProfiling()

With the previous ordering ARM will implement the left hand side of the condition knowing that the branch will never be taken.  On the other hand, the compiler will remove the call to instrumentedProfiling.
Attachment #652271 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 8

5 years ago
Created attachment 653527 [details] [diff] [review]
v3 - enable/disable at runtime

Updated both trampoline sites for x86/64 and took comments into account where necessary.
Attachment #652271 - Attachment is obsolete: true
Attachment #653527 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 9

5 years ago
Comment on attachment 653527 [details] [diff] [review]
v3 - enable/disable at runtime

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

This sounds good for me.
Does this help debugging under gdb, can you create an option  in a follow-up bug for enabling the instrumented profiling with the command line ?  This might help fuzzer to provide a better backtrace too if they can reproduce bugs with a frame pointer.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +227,5 @@
>      masm.movq(Operand(rsp, IonRectifierFrameLayout::offsetOfNumActualArgs()), rdx);
>  
>      masm.moveValue(UndefinedValue(), r10);
>  
> +    masm.movq(rsp, r9); // Save %rsp.

nit: This is not necessary, but you might want to push rbp and move rsp to rbp such as any call respect the frame pointer convention.
Attachment #653527 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 10

5 years ago
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #9)

> can you create an option  in a follow-up bug for enabling the instrumented
> profiling with the command line ?

This already exists as 'enableSPSProfilingAssertions(/* really slow = */ true)'
(Assignee)

Comment 11

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/bd706422296f
(Assignee)

Updated

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

Updated

5 years ago
Resolution: WORKSFORME → FIXED

Updated

5 years ago
Blocks: 802798
You need to log in before you can comment on or make changes to this bug.