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)
Core
JavaScript Engine
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)
15.36 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Once patches are posted I can create a custom build with the profiler to give early feedback.
Updated•12 years ago
|
Whiteboard: [snappy]
Updated•12 years ago
|
Whiteboard: [snappy] → [snappy:p2]
I'll be working on profiling in general, and this looks highly relevant to what I'll be doing soon.
Assignee: general → acrichton
Currently hidden behind the --enable-profiling flags, but still passes all tests.
Reporter | ||
Comment 5•12 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?
Updated•12 years ago
|
Whiteboard: [snappy:p2] → [snappy:p2][ion:t]
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•12 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)
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•12 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•12 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•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/bd706422296f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•