Clean up profiler pseudo-stack representation

RESOLVED FIXED in Firefox 32

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: djvj, Assigned: vporof)

Tracking

unspecified
Firefox 32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

Reporter

Description

5 years ago
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

5 years ago
Assignee: nobody → vporof
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Assignee

Comment 1

5 years ago
Posted patch v1 (obsolete) — Splinter Review
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

5 years ago
Blocks: 1007203
Assignee

Updated

5 years ago
Priority: -- → P2
Assignee

Comment 2

5 years ago
Posted patch v2 (obsolete) — Splinter Review
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

5 years ago
Posted patch v3 (obsolete) — Splinter Review
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

5 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

5 years ago
Posted patch v4 (obsolete) — Splinter Review
Addressed comments, NullPCIndex is back in.
Attachment #8422002 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Blocks: 1010578
Assignee

Comment 6

5 years ago
Posted patch v5 (obsolete) — Splinter Review
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

5 years ago
Attachment #8422724 - Attachment is obsolete: true
Assignee

Comment 7

5 years ago
Things are looking good on try: https://tbpl.mozilla.org/?tree=Try&rev=db42539cee78 (except for ARM).
Assignee

Comment 8

5 years ago
Posted patch v6 (obsolete) — Splinter Review
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

5 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

5 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

5 years ago
(In reply to Kannan Vijayan [:djvj] from comment #10)
> Comment on attachment 8423227 [details] [diff] [review]
> v6
> 

whoops, typo!
Reporter

Comment 12

5 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 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

5 years ago
Posted patch v7 (obsolete) — Splinter Review
Updated patch.
Attachment #8423227 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
Posted patch v8 (obsolete) — Splinter Review
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 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

5 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.
Ohh opps, read the indentation wrong.
Reporter

Comment 20

5 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

5 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

5 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 23

5 years ago
Posted patch v9 (obsolete) — Splinter Review
Addressed comments.
Attachment #8425725 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Updated

5 years ago
Attachment #8423995 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c2429d6c41fb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Assignee

Comment 26

5 years ago
Backed out for breaking (apparently untested stuff in) B2G: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa72130d50b
Assignee

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

5 years ago
Attachment #8425725 - Attachment is obsolete: true
(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

5 years ago
Posted patch v10Splinter Review
Reverted to the original arguments order in StackEntry::push.
Attachment #8429527 - Flags: review?(nchen)
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)
Attachment #8429527 - Flags: review?(bgirard) → review+
Assignee

Comment 31

5 years ago
Relanded: https://hg.mozilla.org/integration/fx-team/rev/163cef53a92c
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/163cef53a92c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 1020541

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.