Closed
Bug 1004726
Opened 10 years ago
Closed 10 years ago
Clean up profiler pseudo-stack representation
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: djvj, Assigned: vporof)
References
Details
Attachments
(1 file, 9 obsolete files)
37.15 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
This came up in a discussion with :victorporof about using the pseudo-stack to enable "category" information for pseudostack entries (e.g. "network", "graphics", ...) The current pseudostack representation can be cleaned up significantly. Currently it looks like the following: const char *string // the Descriptive string for the entry void *sp // Stack pointer for entry. Null for JS entries. JSScript *script // script for entry. Null for non-JS entries. uint32_t idx // lineno for non-js entries, pcOffset for js entries This can be changed to: const char *string // same as before void *spOrScript // non-js entries: stack pointer. js entries: script pointer. uint32_t idx // non-js entries: lineno. js entries: pcOffset uint32_t flags // flag bits The flag bits can store lots of flags: whether the entry is for JS or non-JS, the category of the entry, and still have many bits left over for future uses. On 64-bit systems, this will reduce the size of the entry by 8 bytes. On 32-bit systems, the profiler entry will remain the same size.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → vporof
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Here's a first wip that looks relatively sane to me. However, I do get an assertion in TableTicker.cpp, line 353, when trying to run the profiler: "Assertion failure: &entry == &stack->mStack[stack->stackSize() - 1]". Any hints?
Attachment #8417685 -
Flags: feedback?(kvijayan)
Assignee | ||
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•10 years ago
|
||
Moar WIP. This version of the patch starts using the flags field for everything except NullPCIndex, which seems to be manually set when entering/exiting C++ frames [0], so removing it seems a bit more problematic than expected (and possibly beyond the scope of this bug). Unfortunately, the previous assertion still fails [1], even though the spOrScript and lineOrPc fields are set exclusively for JS xor CPP frames. Moreover, [2] seems to fail as well. [0] http://dxr.mozilla.org/mozilla-central/source/js/src/vm/SPSProfiler.h?from=spsprofiler.h#440 [1] http://dxr.mozilla.org/mozilla-central/source/tools/profiler/TableTicker.cpp?from=tableticker.cpp#353 [2] http://dxr.mozilla.org/mozilla-central/source/js/src/vm/SPSProfiler.cpp?from=spsprofiler.cpp#165
Attachment #8417685 -
Attachment is obsolete: true
Attachment #8417685 -
Flags: feedback?(kvijayan)
Attachment #8421433 -
Flags: feedback?(kvijayan)
Assignee | ||
Comment 3•10 years ago
|
||
Actually removed NullPCIndex. Assertions from the previous comment still fail.
Attachment #8421433 -
Attachment is obsolete: true
Attachment #8421433 -
Flags: feedback?(kvijayan)
Attachment #8422002 -
Flags: feedback?(kvijayan)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8422002 [details] [diff] [review] v3 Review of attachment 8422002 [details] [diff] [review]: ----------------------------------------------------------------- Found one problem, and a bunch of nits. ::: js/public/ProfilingStack.h @@ +18,5 @@ > > namespace js { > > +// These traits are bit masks. Make sure they're powers of 2. > +enum ProfileEntryFlag { ProfileEntryFlag can probably just move into the ProfileEntry class, and just be called |Flag| or |Flags|. @@ +26,5 @@ > + IS_CPP_ENTRY = 1, > + > + // Indicate that copying the frame label is not necessary when taking a > + // sample of the pseudostack. > + FRAME_LABEL_COPY = 2 Nit: use hex notation for integer literals which are bit-masks. So 0x01 and 0x02. ::: js/src/jit/x64/MacroAssembler-x64.h @@ +571,5 @@ > } > void subPtr(const Register &src, const Address &dest) { > subq(src, Operand(dest)); > } > + void mulBy3(const Register &src, const Register &dest) { Also need implementation of mulBy3 for ARM macroassembler. ::: js/src/vm/SPSProfiler.cpp @@ +234,5 @@ > + if (sp != nullptr) { > + stack[current].setLabel(string); > + stack[current].setCppFrame(sp, 0); > + } else { > + stack[current].setLabel(string); Nit: the setLabel() can be pulled out and put before the conditional since it occurs in both branches. Then remove braces for single-line control flow statements (style guidelines) @@ +239,5 @@ > + stack[current].setJsFrame(script, pc); > + } > + > + // Track if mLabel needs a copy. > + if (copy) { Style nit: no braces on single-line conditionals @@ +338,5 @@ > { > + MOZ_ASSERT(isJs()); > + if (lineOrPc) > + return script()->offsetToPC(lineOrPc); > + return nullptr; It seems like you're using a pcOffset of 0 to indicate a null bytecode pointer, but 0 is a valid value for pcOffset (it references the bytecode pointer to the start of a bytecode for a JSScript). You still need NullPCIndex = -1 (but call it NullPCOffset instead, to be consistent with nomenclature), and check for that instead: if (lineOrPc == NullPCOffset) return nullptr; return script()->offsetToPC(lineOrPc); @@ +347,5 @@ > { > + MOZ_ASSERT(isJs()); > + if (pc) > + lineOrPc = script()->pcToOffset(pc); > + lineOrPc = 0; See comment for |pc()| above. This will become |lineOrPc = NullPCOffset| ::: tools/profiler/PseudoStack.h @@ +358,5 @@ > + // Track if mLabel needs a copy. > + if (aCopy) { > + mStack[mStackPointer].setFlag(js::ProfileEntryFlag::FRAME_LABEL_COPY); > + } else { > + mStack[mStackPointer].unsetFlag(js::ProfileEntryFlag::FRAME_LABEL_COPY); Just a nit, but I think it would be more hygenic to grab a reference: stackEntry = mStack[mStackPointer] at the top, and replace all the repeated array derefs.
Attachment #8422002 -
Flags: feedback?(kvijayan) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
Addressed comments, NullPCIndex is back in.
Attachment #8422002 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
All assertions seem to be passing again! Marty: I'm writing a "multiply by 3" operation to be used by the IonMacroAssember, because we need to multiply a pointer by 24. I've implemented the x86 part using `lea`, but I need help with implementing the ARM variant. Could you give me a few pointers? Thank you!
Attachment #8422777 -
Flags: feedback?(mrosenberg)
Assignee | ||
Updated•10 years ago
|
Attachment #8422724 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Things are looking good on try: https://tbpl.mozilla.org/?tree=Try&rev=db42539cee78 (except for ARM).
Assignee | ||
Comment 8•10 years ago
|
||
Added ARM implementation for mulBy3. I think this should be enough, so pushing to try. If all's good, I'm going to ask for review.
Attachment #8422777 -
Attachment is obsolete: true
Attachment #8422777 -
Flags: feedback?(mrosenberg)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8423227 [details] [diff] [review] v6 Yup, I'm still going to need a little bit of help for this. Marty: See comment 6. I (think I) managed to write an ARM implementation for "multiply by 3", but building fails with the following log: https://pastebin.mozilla.org/5167496 It might be the case that the methods in MacroAssembler-arm.cpp are expecting Addresses, whereas I'm passing Registers, or that I'm simply not writing the implementation where I should be. I have zero experience with this code so a few pointers would be much appreciated.
Attachment #8423227 -
Flags: feedback?(mrosenberg)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8423227 [details] [diff] [review] v6 Review of attachment 8423227 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/MacroAssembler-arm.cpp @@ +2594,5 @@ > > void > +MacroAssemblerARMCompat::mulBy3(const Register &src, const Register &dest) > +{ > + loadPtr(dest, ScratchRegister); you want |loadPtr(src, ScratchRegister);| here.
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #10) > Comment on attachment 8423227 [details] [diff] [review] > v6 > whoops, typo!
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8423227 [details] [diff] [review] v6 Review of attachment 8423227 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/MacroAssembler-arm.cpp @@ +2594,5 @@ > > void > +MacroAssemblerARMCompat::mulBy3(const Register &src, const Register &dest) > +{ > + loadPtr(dest, ScratchRegister); Also, register => register moves are done with movePointer, not loadPointer. So you want movePtr(src, ScratchRegister) at the start and movePtr(ScratchRegister, dest) at the end.
Comment 13•10 years ago
|
||
Comment on attachment 8423227 [details] [diff] [review] v6 Review of attachment 8423227 [details] [diff] [review]: ----------------------------------------------------------------- I'll try thinking of a better way to do *24 rather than t <- x << 2; d <- t + (t<<1), but I don't think there is one. ::: js/src/jit/arm/MacroAssembler-arm.cpp @@ +2596,5 @@ > +MacroAssemblerARMCompat::mulBy3(const Register &src, const Register &dest) > +{ > + loadPtr(dest, ScratchRegister); > + lshiftPtr(Imm32(1), ScratchRegister); > + addPtr(src, ScratchRegister); the movePtr; lshiftPtr, addPtr; storePtr sequence can be done in a single instruction, but you need to use the bare assembler, rather than the macro assembler: as_add(dest, src, lsl(src, 1)) N.B. the dest is the first argument on the underlying assembler.
Attachment #8423227 -
Flags: feedback?(mrosenberg) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
Updated patch.
Attachment #8423227 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=b09dc98e232f
Assignee | ||
Comment 16•10 years ago
|
||
Alright, everything seems to look good (modulo a clobber): https://tbpl.mozilla.org/?tree=Try&rev=ba69e6b76437
Attachment #8423402 -
Attachment is obsolete: true
Attachment #8423995 -
Flags: review?(kvijayan)
Attachment #8423995 -
Flags: review?(bgirard)
Comment 17•10 years ago
|
||
Comment on attachment 8423995 [details] [diff] [review] v8 Review of attachment 8423995 [details] [diff] [review]: ----------------------------------------------------------------- Profiler side changes look good, minus this one part I don't understand. ::: tools/profiler/TableTicker.cpp @@ +369,5 @@ > aProfile.addTag(ProfileEntry('c', sampleLabel)); > + > + // XXX: Bug 1010578. Don't assume a CPP entry and try to get the > + // line for js entries as well. > + if (entry.isCpp()) { I don't understand this. We already checked and know this isn't a JS entry.
Attachment #8423995 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #17) > Comment on attachment 8423995 [details] [diff] [review] > v8 > > Review of attachment 8423995 [details] [diff] [review]: > ----------------------------------------------------------------- > > Profiler side changes look good, minus this one part I don't understand. > > ::: tools/profiler/TableTicker.cpp > @@ +369,5 @@ > > aProfile.addTag(ProfileEntry('c', sampleLabel)); > > + > > + // XXX: Bug 1010578. Don't assume a CPP entry and try to get the > > + // line for js entries as well. > > + if (entry.isCpp()) { > > I don't understand this. We already checked and know this isn't a JS entry. We check *now*. The previous variant of this code didn't have the conditional and assumed that this was always a cpp entry.
Comment 19•10 years ago
|
||
Ohh opps, read the indentation wrong.
Reporter | ||
Comment 20•10 years ago
|
||
Comment on attachment 8423995 [details] [diff] [review] v8 Review of attachment 8423995 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/ProfilingStack.h @@ +68,5 @@ > // instances are volatile. These methods would not be available unless they > // were marked as volatile as well. > > + bool isCpp() const volatile { return hasFlag(IS_CPP_ENTRY); } > + bool isJs() const volatile { return !isCpp(); } I suggest adding a |isCopyLabel| method here as an alias to hasFlag(FRAME_LABEL_COPY), and using it in all the places where we are manually checking hasFlag(FRAME_LABEL_COPY). ::: js/src/vm/SPSProfiler.cpp @@ +166,4 @@ > } > #endif > > + push(str, nullptr, script, script->code(), true); Nit: comment the last parameter. i.e. "/* copy = */ true" @@ -198,4 @@ > JS_ASSERT(stack_[*size_].script() == script); > JS_ASSERT(strcmp((const char*) stack_[*size_].label(), str) == 0); > - stack_[*size_].setLabel(nullptr); > - stack_[*size_].setPC(nullptr); I'm not sure why these two lines were removed. Don't we still want to set the label and pc to nullptr in debug mode when popping a frame? @@ +320,5 @@ > profiler = nullptr; > return; > } > size_before = *profiler->size_; > + profiler->push("js::RunScript", this, nullptr, nullptr, false); Nit: label last parameter. i.e. "/* copy = */ false" ::: tools/profiler/PseudoStack.h @@ +358,5 @@ > + entry.setCppFrame(aStackAddress, line); > + > + // Track if mLabel needs a copy. > + if (aCopy) { > + entry.setFlag(js::ProfileEntry::FRAME_LABEL_COPY); Style nit: no braces on single-line conditionals. ::: tools/profiler/TableTicker.cpp @@ +341,5 @@ > > // First entry has tagName 's' (start) > // Check for magic pointer bit 1 to indicate copy > const char* sampleLabel = entry.label(); > + if (entry.hasFlag(js::ProfileEntry::FRAME_LABEL_COPY)) { |isCopyLabel| reads better.
Attachment #8423995 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #20) > Comment on attachment 8423995 [details] [diff] [review] > v8 > > @@ -198,4 @@ > > JS_ASSERT(stack_[*size_].script() == script); > > JS_ASSERT(strcmp((const char*) stack_[*size_].label(), str) == 0); > > - stack_[*size_].setLabel(nullptr); > > - stack_[*size_].setPC(nullptr); > > I'm not sure why these two lines were removed. Don't we still want to set > the label and pc to nullptr in debug mode when popping a frame? > I'm not sure why they were there in the first place. It doesn't seem like that's needed, but I can add it back (via a call to setJsFrame or something, instead of those two granular methods).
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #21) > (In reply to Kannan Vijayan [:djvj] from comment #20) > > I'm not sure why they were there in the first place. It doesn't seem like > that's needed, but I can add it back (via a call to setJsFrame or something, > instead of those two granular methods). Actually, scratch that, we still have setPC.
Assignee | ||
Comment 24•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=34750c33ec20
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Attachment #8423995 -
Attachment is obsolete: true
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c2429d6c41fb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Assignee | ||
Comment 26•10 years ago
|
||
Backed out for breaking (apparently untested stuff in) B2G: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa72130d50b
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 27•10 years ago
|
||
The patch changed the order of StackEntry::push's arguments, but didn't change some of the callers (e.g. in GeckoProfilerImpl.h)
Assignee | ||
Updated•10 years ago
|
Attachment #8425725 -
Attachment is obsolete: true
Comment 28•10 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #26) > Backed out for breaking (apparently untested stuff in) B2G: > https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa72130d50b Merge of backout: https://hg.mozilla.org/mozilla-central/rev/9fa72130d50b
Assignee | ||
Comment 29•10 years ago
|
||
Reverted to the original arguments order in StackEntry::push.
Attachment #8429527 -
Flags: review?(nchen)
Comment 30•10 years ago
|
||
Comment on attachment 8429527 [details] [diff] [review] v10 Review of attachment 8429527 [details] [diff] [review]: ----------------------------------------------------------------- Looks okay, but I think BenWa should review it.
Attachment #8429527 -
Flags: review?(nchen) → review?(bgirard)
Updated•10 years ago
|
Attachment #8429527 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 31•10 years ago
|
||
Relanded: https://hg.mozilla.org/integration/fx-team/rev/163cef53a92c
Whiteboard: [fixed-in-fx-team]
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/163cef53a92c
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•