Closed Bug 1366650 Opened 3 years ago Closed 3 years ago

Move PseudoStack into SpiderMonkey

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files)

Pseudo-stack entries have the type ProfileEntry, which is defined in SpiderMonkey. But the PseudoStack class is defined in tools/profiler/, which SpiderMonkey can't see. As a result, SpiderMonkey interacts with the pseudo-stack  not via the PseudoStack class, but instead as a raw array, via GeckoProfiler::{stack_,size_,max}.

This split responsibility is silly. Fortunately it's fixable by moving PseudoStack into SpiderMonkey, so that both SpiderMonkey and the Gecko Profiler can see it.
This includes renaming its fields to match SpiderMonkey naming conventions
instead of Gecko naming conventions.

This patch is just about moving the code. The next patch will change
SpiderMonkey to actually use PseudoStack directly.
Attachment #8870388 - Flags: review?(shu)
Attachment #8870388 - Flags: review?(mstange)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
- The profiler gives the JS engine a reference to the pseudo-stack via
  SetContextProfiilngStack(). That function now takes a |PseudoStack*| instead
  of a |ProfileEntry*| and pointer to the stack pointer.

- PseudoStack now has a |kMaxEntries| field, which is easier to work with than
  |mozilla::ArrayLength(entries_)|.

- AddressOfStackPointer() is no longer needed.

- The patch also neatens up the push operations significantly. PseudoStack now
  has pushCppFrame() and pushJsFrame(), which nicely encapsulate the two main
  cases. These delegate to the updated initCppFrame() and initJsFrame()
  functions in ProfileEntry.

- Renames max_stck in testProfileStrings.cpp as peakStackPointer, which is a
  clearer name.

- Removes a couple of checks from testProfileStrings.cpp. These checks made
  sense when the pseudo-stack was accessed via raw manipulation, but are not
  applicable now because we can't artificially limit the maximum stack size so
  easily.
Attachment #8870391 - Flags: review?(shu)
Attachment #8870391 - Flags: review?(mstange)
Attachment #8870388 - Flags: review?(mstange) → review+
Comment on attachment 8870391 [details] [diff] [review]
(part 2) - In GeckoProfiler, do all pseudo-stack accesses via the PseudoStack class, instead of via raw array manipulation

Review of attachment 8870391 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I don't really understand the relevance of the volatile additions.

::: js/public/ProfilingStack.h
@@ +119,5 @@
> +                      volatile {
> +        label_ = aLabel;
> +        dynamicString_ = aDynamicString;
> +        spOrScript = sp;
> +        lineOrPcOffset = static_cast<int32_t>(aLine);

Aside: Do you know why these two fields don't end in an underscore?

@@ +190,5 @@
>      // We can't know the layout of JSScript, so look in vm/GeckoProfiler.cpp.
>      JS_FRIEND_API(jsbytecode*) pc() const volatile;
>      JS_FRIEND_API(void) setPC(jsbytecode* pc) volatile;
>  
> +    void trace(JSTracer* trc) volatile;

Why this change?

::: js/src/vm/GeckoProfiler.cpp
@@ +247,4 @@
>              }
>          }
>  
> +        volatile ProfileEntry& entry = pseudoStack_->entries_[sp];

Why is this volatile?
Attachment #8870391 - Flags: review?(mstange) → review+
> Looks good to me. I don't really understand the relevance of the volatile
> additions.

I just added them where the compiler complained about them being missing :)  volatile propagates a bit like const, and ProfileEntry has several volatile fields, so methods and references need volatile as well.
 
> > +        label_ = aLabel;
> > +        dynamicString_ = aDynamicString;
> > +        spOrScript = sp;
> > +        lineOrPcOffset = static_cast<int32_t>(aLine);
> 
> Aside: Do you know why these two fields don't end in an underscore?

SpiderMonkey style is a bit inconsistent about field naming. AIUI, generally underscores aren't used, except sometimes when there would be shadowing with getters of the same name, in which case the underscore is appended.
Blocks: 1348402
Comment on attachment 8870388 [details] [diff] [review]
(part 1) - Move PseudoStack into SpiderMonkey

Review of attachment 8870388 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/ProfilingStack.h
@@ +210,5 @@
>  } // namespace js
>  
> +// The PseudoStack members are accessed in parallel by multiple threads: the
> +// profiler's sampler thread reads these members while other threads modify
> +// them.

I find it strange that the old comments talk about reentrant *mutation*. What did that mean? In any case this comment makes more sense.

@@ +213,5 @@
> +// profiler's sampler thread reads these members while other threads modify
> +// them.
> +class PseudoStack
> +{
> +public:

Nit: SM style has half-indent for public/private/protected. Don't shoot the messenger!

class C {
  public:
    ...
};

@@ +260,5 @@
> +    void operator=(const PseudoStack&) = delete;
> +
> +public:
> +    // The stack entries.
> +    js::ProfileEntry volatile entries_[1024];

Style nit: the _ suffix is for private members, AFAIK. This should just be |entries|.
Attachment #8870388 - Flags: review?(shu) → review+
Comment on attachment 8870391 [details] [diff] [review]
(part 2) - In GeckoProfiler, do all pseudo-stack accesses via the PseudoStack class, instead of via raw array manipulation

Review of attachment 8870391 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/ProfilingStack.h
@@ +115,5 @@
>      const char* dynamicString() const volatile { return dynamicString_; }
>  
> +    void initCppFrame(const char* aLabel, const char* aDynamicString, void* sp, uint32_t aLine,
> +                      js::ProfileEntry::Flags aFlags, js::ProfileEntry::Category aCategory)
> +                      volatile {

Style nit: I'd put the { on its own line for multi-line parameters. We don't do this 'a' prefixing stuff either.

@@ +256,3 @@
>      }
>  
> +    void pop()

Style nit: for inline function bodies *without* multiline params, { goes on the same line.

@@ +268,5 @@
>      PseudoStack(const PseudoStack&) = delete;
>      void operator=(const PseudoStack&) = delete;
>  
>  public:
> +    static const uint32_t kMaxEntries = 1024;

MaxEntries without the k prefix

::: js/src/jsapi-tests/testProfileStrings.cpp
@@ -137,5 @@
> -        CHECK(JS_CallFunctionName(cx, global, "check", JS::HandleValueArray::empty(),
> -                                  &rval));
> -        CHECK((size_t) pstack[3].label() == 1234);
> -        CHECK(max_stack >= 8);
> -        CHECK(psize == 0);

Just so I understand, these tests are now removed because it's no longer to set a non-MaxEntries sized pseudo stack.
Attachment #8870391 - Flags: review?(shu) → review+
> We don't do this 'a' prefixing stuff either.

In this case it's necessary because there are getters named label(), dynamicString(), etc., and GCC complains about shadowing if the parameters have the same name. (There were already some aFoo params in this file before these patches.)
> I find it strange that the old comments talk about reentrant *mutation*.
> What did that mean? In any case this comment makes more sense.

I'm not 100% sure. Two possibilities:

- The profiler used to do more stuff within signal handlers. That's not really the case any more, but there are a bunch of out-of-date comments about signal handlers and re-entrancy.

- Until recent improvements, the quality of the profiler's code and comments was... uneven. It could well have just been wrong, or badly expressed.
> ::: js/src/jsapi-tests/testProfileStrings.cpp
> @@ -137,5 @@
> > -        CHECK(JS_CallFunctionName(cx, global, "check", JS::HandleValueArray::empty(),
> > -                                  &rval));
> > -        CHECK((size_t) pstack[3].label() == 1234);
> > -        CHECK(max_stack >= 8);
> > -        CHECK(psize == 0);
> 
> Just so I understand, these tests are now removed because it's no longer to
> set a non-MaxEntries sized pseudo stack.

No longer possible, correct.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4945a4dcc9652bd831e0d1db89efaa871660f6
Bug 1366650 (part 1) - Move PseudoStack into SpiderMonkey. r=mstange,shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/edab8a23147dc530ec191b23b880b36df18ffbdd
Bug 1366650 (part 2) - In GeckoProfiler, do all pseudo-stack accesses via the PseudoStack class, instead of via raw array manipulation. r=mstange,shu.
https://hg.mozilla.org/mozilla-central/rev/ff4945a4dcc9
https://hg.mozilla.org/mozilla-central/rev/edab8a23147d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See Also: → 1396365
See Also: 1396365
You need to log in before you can comment on or make changes to this bug.