Last Comment Bug 736299 - IonMonkey: Add a frame pointer for profiling & debugging mode.
: IonMonkey: Add a frame pointer for profiling & debugging mode.
Status: RESOLVED FIXED
[snappy:p2][ion:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: u443197
:
: Jason Orendorff [:jorendorff]
Mentors:
: 774767 (view as bug list)
Depends on: 775782
Blocks: IonMonkey 761253 802798 781979
  Show dependency treegraph
 
Reported: 2012-03-15 16:01 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-11-02 07:21 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 diff (3.54 KB, patch)
2012-06-08 18:41 PDT, u443197
no flags Details | Diff | Splinter Review
v2 - enable/disable at runtime (8.30 KB, patch)
2012-08-15 16:33 PDT, u443197
no flags Details | Diff | Splinter Review
v3 - enable/disable at runtime (15.36 KB, patch)
2012-08-20 14:43 PDT, u443197
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-03-15 16:01:02 PDT
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.
Comment 1 Benoit Girard (:BenWa) 2012-03-15 16:15:16 PDT
Once patches are posted I can create a custom build with the profiler to give early feedback.
Comment 2 u443197 2012-06-05 14:15:22 PDT
I'll be working on profiling in general, and this looks highly relevant to what I'll be doing soon.
Comment 3 u443197 2012-06-08 18:41:01 PDT
Created attachment 631589 [details] [diff] [review]
v1 diff

Currently hidden behind the --enable-profiling flags, but still passes all tests.
Comment 4 Nicolas B. Pierron [:nbp] 2012-07-17 11:35:13 PDT
*** Bug 774767 has been marked as a duplicate of this bug. ***
Comment 5 Nicolas B. Pierron [:nbp] 2012-07-17 11:38:55 PDT
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?
Comment 6 u443197 2012-08-15 16:33:26 PDT
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.
Comment 7 Nicolas B. Pierron [:nbp] 2012-08-16 11:16:11 PDT
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.
Comment 8 u443197 2012-08-20 14:43:56 PDT
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.
Comment 9 Nicolas B. Pierron [:nbp] 2012-08-21 15:54:29 PDT
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.
Comment 10 u443197 2012-08-21 19:02:36 PDT
(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)'

Note You need to log in before you can comment on or make changes to this bug.