Closed Bug 736299 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: u443197)

References

(Blocks 3 open bugs)

Details

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

Attachments

(1 file, 2 obsolete files)

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]
Whiteboard: [snappy] → [snappy:p2]
Blocks: 761253
I'll be working on profiling in general, and this looks highly relevant to what I'll be doing soon.
Assignee: general → acrichton
Attached patch v1 diff (obsolete) — Splinter Review
Currently hidden behind the --enable-profiling flags, but still passes all tests.
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?
Depends on: 775782
Whiteboard: [snappy:p2] → [snappy:p2][ion:t]
Blocks: 781979
No longer blocks: 781979
Blocks: 781979
Attached patch v2 - enable/disable at runtime (obsolete) — Splinter Review
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)
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)
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)
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+
(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)'
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Resolution: WORKSFORME → FIXED
Blocks: 802798
You need to log in before you can comment on or make changes to this bug.