Commit 1b418b1c authored by Nicholas Nethercote's avatar Nicholas Nethercote
Browse files

Bug 1369644 - Remove use of |volatile| from ProfileEntry. r=mstange,shu,jseward,froydnj. 

These annotations aren't doing anything useful. The important thing with
the PseudoStack is that, during pushes, the stack pointer incrementing happens
after the new entry is written, and this is ensured by the stack pointer being
Atomic.

The patch also improves the comments on PseudoStack.

--HG--
extra : rebase_source : 100f8a5e4b750c15fac66175550c4c284a141f16
parent 5141530f

js/public/ProfilingStack.h

+43 −42
Original line number Diff line number Diff line
@@ -29,35 +29,26 @@ namespace js { 
//

class ProfileEntry

{

    // All fields are marked volatile to prevent the compiler from re-ordering

    // instructions. Namely this sequence:

    //

    //    entry[size] = ...;

    //    size++;

    //

    // If the size modification were somehow reordered before the stores, then

    // if a sample were taken it would be examining bogus information.

    //

    // A ProfileEntry represents both a C++ profile entry and a JS one.

    // A ProfileEntry represents either a C++ profile entry or a JS one.



    // Descriptive label for this entry. Must be a static string! Can be an

    // empty string, but not a null pointer.

    const char * volatile label_;

    const char* label_;



    // An additional descriptive string of this entry which is combined with

    // |label_| in profiler output. Need not be (and usually isn't) static. Can

    // be null.

    const char * volatile dynamicString_;

    const char* dynamicString_;



    // Stack pointer for non-JS entries, the script pointer otherwise.

    void * volatile spOrScript;

    void* spOrScript;



    // Line number for non-JS entries, the bytecode offset otherwise.

    int32_t volatile lineOrPcOffset;

    int32_t lineOrPcOffset;



    // Bits 0..1 hold the Kind. Bits 2..3 are unused. Bits 4..12 hold the

    // Category.

    uint32_t volatile kindAndCategory_;

    uint32_t kindAndCategory_;



    static int32_t pcToOffset(JSScript* aScript, jsbytecode* aPc);
@@ -103,30 +94,25 @@ class ProfileEntry 
    static_assert((uint32_t(Category::FIRST) & uint32_t(Kind::KIND_MASK)) == 0,

                  "Category overlaps with Kind");



    // All of these methods are marked with the 'volatile' keyword because the

    // Gecko Profiler's representation of the stack is stored such that all

    // ProfileEntry instances are volatile. These methods would not be

    // available unless they were marked as volatile as well.



    bool isCpp() const volatile

    bool isCpp() const

    {

        Kind k = kind();

        return k == Kind::CPP_NORMAL || k == Kind::CPP_MARKER_FOR_JS;

    }



    bool isJs() const volatile

    bool isJs() const

    {

        Kind k = kind();

        return k == Kind::JS_NORMAL || k == Kind::JS_OSR;

    }



    void setLabel(const char* aLabel) volatile { label_ = aLabel; }

    const char* label() const volatile { return label_; }

    void setLabel(const char* aLabel) { label_ = aLabel; }

    const char* label() const { return label_; }



    const char* dynamicString() const volatile { return dynamicString_; }

    const char* dynamicString() const { return dynamicString_; }



    void initCppFrame(const char* aLabel, const char* aDynamicString, void* sp, uint32_t aLine,

                      Kind aKind, Category aCategory) volatile

                      Kind aKind, Category aCategory)

    {

        label_ = aLabel;

        dynamicString_ = aDynamicString;
@@ -137,7 +123,7 @@ class ProfileEntry 
    }



    void initJsFrame(const char* aLabel, const char* aDynamicString, JSScript* aScript,

                     jsbytecode* aPc) volatile

                     jsbytecode* aPc)

    {

        label_ = aLabel;

        dynamicString_ = aDynamicString;
@@ -147,41 +133,41 @@ class ProfileEntry 
        MOZ_ASSERT(isJs());

    }



    void setKind(Kind aKind) volatile {

    void setKind(Kind aKind) {

        kindAndCategory_ = uint32_t(aKind) | uint32_t(category());

    }



    Kind kind() const volatile {

    Kind kind() const {

        return Kind(kindAndCategory_ & uint32_t(Kind::KIND_MASK));

    }



    Category category() const volatile {

    Category category() const {

        return Category(kindAndCategory_ & uint32_t(Category::CATEGORY_MASK));

    }



    void* stackAddress() const volatile {

    void* stackAddress() const {

        MOZ_ASSERT(!isJs());

        return spOrScript;

    }



    JS_PUBLIC_API(JSScript*) script() const volatile;

    JS_PUBLIC_API(JSScript*) script() const;



    uint32_t line() const volatile {

    uint32_t line() const {

        MOZ_ASSERT(!isJs());

        return static_cast<uint32_t>(lineOrPcOffset);

    }



    // Note that the pointer returned might be invalid.

    JSScript* rawScript() const volatile {

    JSScript* rawScript() const {

        MOZ_ASSERT(isJs());

        return (JSScript*)spOrScript;

    }



    // We can't know the layout of JSScript, so look in vm/GeckoProfiler.cpp.

    JS_FRIEND_API(jsbytecode*) pc() const volatile;

    void setPC(jsbytecode* pc) volatile;

    JS_FRIEND_API(jsbytecode*) pc() const;

    void setPC(jsbytecode* pc);



    void trace(JSTracer* trc) volatile;

    void trace(JSTracer* trc);



    // The offset of a pc into a script's code can actually be 0, so to

    // signify a nullptr pc, use a -1 index. This is checked against in
@@ -200,9 +186,24 @@ RegisterContextProfilingEventMarker(JSContext* cx, void (*fn)(const char*)); 


} // 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.

// Each thread has its own PseudoStack. That thread modifies the PseudoStack,

// pushing and popping elements as necessary.

//

// The PseudoStack is also read periodically by the profiler's sampler thread.

// This happens only when the thread that owns the PseudoStack is suspended. So

// there are no genuine parallel accesses.

//

// However, it is possible for pushing/popping to be interrupted by a periodic

// sample. Because of this, we need pushing/popping to be effectively atomic.

//

// - When pushing a new entry, we increment the stack pointer -- making the new

//   entry visible to the sampler thread -- only after the new entry has been

//   fully written. The stack pointer is Atomic<> (with SequentiallyConsistent

//   semantics) to ensure the incrementing is not reordered before the writes.

//

// - When popping an old entry, the only operation is the decrementing of the

//   stack pointer, which is obviously atomic.

//

class PseudoStack

{

  public:
@@ -255,13 +256,13 @@ class PseudoStack 
    static const uint32_t MaxEntries = 1024;



    // The stack entries.

    js::ProfileEntry volatile entries[MaxEntries];

    js::ProfileEntry entries[MaxEntries];



    // This may exceed MaxEntries, so instead use the stackSize() method to

    // determine the number of valid samples in entries. When this is less

    // than MaxEntries, it refers to the first free entry past the top of the

    // in-use stack (i.e. entries[stackPointer - 1] is the top stack entry).

    mozilla::Atomic<uint32_t> stackPointer;

    mozilla::Atomic<uint32_t, mozilla::SequentiallyConsistent> stackPointer;

};



#endif  /* js_ProfilingStack_h */

js/src/vm/GeckoProfiler.cpp

+9 −9
Original line number Diff line number Diff line
@@ -239,7 +239,7 @@ GeckoProfiler::exit(JSScript* script, JSFunction* maybeFun) 
                            uint32_t(pseudoStack_->stackPointer),

                            PseudoStack::MaxEntries);

            for (int32_t i = sp; i >= 0; i--) {

                volatile ProfileEntry& entry = pseudoStack_->entries[i];

                ProfileEntry& entry = pseudoStack_->entries[i];

                if (entry.isJs())

                    fprintf(stderr, "  [%d] JS %s\n", i, entry.dynamicString());

                else
@@ -247,7 +247,7 @@ GeckoProfiler::exit(JSScript* script, JSFunction* maybeFun) 
            }

        }



        volatile ProfileEntry& entry = pseudoStack_->entries[sp];

        ProfileEntry& entry = pseudoStack_->entries[sp];

        MOZ_ASSERT(entry.isJs());

        MOZ_ASSERT(entry.script() == script);

        MOZ_ASSERT(strcmp((const char*) entry.dynamicString(), dynamicString) == 0);
@@ -310,7 +310,7 @@ GeckoProfiler::allocProfileString(JSScript* script, JSFunction* maybeFun) 
}



void

GeckoProfiler::trace(JSTracer* trc) volatile

GeckoProfiler::trace(JSTracer* trc)

{

    if (pseudoStack_) {

        size_t size = pseudoStack_->stackSize();
@@ -353,7 +353,7 @@ GeckoProfiler::checkStringsMapAfterMovingGC() 
#endif



void

ProfileEntry::trace(JSTracer* trc) volatile

ProfileEntry::trace(JSTracer* trc)

{

    if (isJs()) {

        JSScript* s = rawScript();
@@ -440,7 +440,7 @@ GeckoProfilerBaselineOSRMarker::GeckoProfilerBaselineOSRMarker(JSRuntime* rt, bo 
    if (sp == 0)

        return;



    volatile ProfileEntry& entry = profiler->pseudoStack_->entries[sp - 1];

    ProfileEntry& entry = profiler->pseudoStack_->entries[sp - 1];

    MOZ_ASSERT(entry.kind() == ProfileEntry::Kind::JS_NORMAL);

    entry.setKind(ProfileEntry::Kind::JS_OSR);

}
@@ -455,13 +455,13 @@ GeckoProfilerBaselineOSRMarker::~GeckoProfilerBaselineOSRMarker() 
    if (sp == 0)

        return;



    volatile ProfileEntry& entry = profiler->stack()[sp - 1];

    ProfileEntry& entry = profiler->stack()[sp - 1];

    MOZ_ASSERT(entry.kind() == ProfileEntry::Kind::JS_OSR);

    entry.setKind(ProfileEntry::Kind::JS_NORMAL);

}



JS_PUBLIC_API(JSScript*)

ProfileEntry::script() const volatile

ProfileEntry::script() const

{

    MOZ_ASSERT(isJs());

    auto script = reinterpret_cast<JSScript*>(spOrScript);
@@ -484,7 +484,7 @@ ProfileEntry::script() const volatile 
}



JS_FRIEND_API(jsbytecode*)

ProfileEntry::pc() const volatile

ProfileEntry::pc() const

{

    MOZ_ASSERT(isJs());

    if (lineOrPcOffset == NullPCOffset)
@@ -500,7 +500,7 @@ ProfileEntry::pcToOffset(JSScript* aScript, jsbytecode* aPc) { 
}



void

ProfileEntry::setPC(jsbytecode* pc) volatile

ProfileEntry::setPC(jsbytecode* pc)

{

    MOZ_ASSERT(isJs());

    JSScript* script = this->script();

js/src/vm/GeckoProfiler.h

+2 −2
Original line number Diff line number Diff line
@@ -142,7 +142,7 @@ class GeckoProfiler 
    bool init();



    uint32_t stackPointer() { MOZ_ASSERT(installed()); return pseudoStack_->stackPointer; }

    volatile ProfileEntry* stack() { return pseudoStack_->entries; }

    ProfileEntry* stack() { return pseudoStack_->entries; }



    /* management of whether instrumentation is on or off */

    bool enabled() { MOZ_ASSERT_IF(enabled_, installed()); return enabled_; }
@@ -189,7 +189,7 @@ class GeckoProfiler 
        return &enabled_;

    }



    void trace(JSTracer* trc) volatile;

    void trace(JSTracer* trc);

    void fixupStringsMapAfterMovingGC();

#ifdef JSGC_HASH_TABLE_CHECKS

    void checkStringsMapAfterMovingGC();

tools/profiler/core/platform.cpp

+6 −7
Original line number Diff line number Diff line
@@ -724,8 +724,7 @@ AddDynamicCodeLocationTag(ProfileBuffer* aBuffer, const char* aStr) 


static void

AddPseudoEntry(PSLockRef aLock, ProfileBuffer* aBuffer,

               volatile js::ProfileEntry& entry,

               NotNull<RacyThreadInfo*> aRacyInfo)

               js::ProfileEntry& entry, NotNull<RacyThreadInfo*> aRacyInfo)

{

  MOZ_ASSERT(entry.kind() == js::ProfileEntry::Kind::CPP_NORMAL ||

             entry.kind() == js::ProfileEntry::Kind::JS_NORMAL);
@@ -827,7 +826,7 @@ MergeStacksIntoProfile(PSLockRef aLock, ProfileBuffer* aBuffer, 
                       const TickSample& aSample, NativeStack& aNativeStack)

{

  NotNull<RacyThreadInfo*> racyInfo = aSample.mRacyInfo;

  volatile js::ProfileEntry* pseudoEntries = racyInfo->entries;

  js::ProfileEntry* pseudoEntries = racyInfo->entries;

  uint32_t pseudoCount = racyInfo->stackSize();

  JSContext* context = aSample.mJSContext;
@@ -902,7 +901,7 @@ MergeStacksIntoProfile(PSLockRef aLock, ProfileBuffer* aBuffer, 
    uint8_t* nativeStackAddr = nullptr;



    if (pseudoIndex != pseudoCount) {

      volatile js::ProfileEntry& pseudoEntry = pseudoEntries[pseudoIndex];

      js::ProfileEntry& pseudoEntry = pseudoEntries[pseudoIndex];



      if (pseudoEntry.isCpp()) {

        lastPseudoCppStackAddr = (uint8_t*) pseudoEntry.stackAddress();
@@ -952,7 +951,7 @@ MergeStacksIntoProfile(PSLockRef aLock, ProfileBuffer* aBuffer, 
    // Check to see if pseudoStack frame is top-most.

    if (pseudoStackAddr > jsStackAddr && pseudoStackAddr > nativeStackAddr) {

      MOZ_ASSERT(pseudoIndex < pseudoCount);

      volatile js::ProfileEntry& pseudoEntry = pseudoEntries[pseudoIndex];

      js::ProfileEntry& pseudoEntry = pseudoEntries[pseudoIndex];



      // Pseudo-frames with the CPP_MARKER_FOR_JS kind are just annotations and

      // should not be recorded in the profile.
@@ -1100,7 +1099,7 @@ DoNativeBacktrace(PSLockRef aLock, ProfileBuffer* aBuffer, 
  for (uint32_t i = racyInfo->stackSize(); i > 0; --i) {

    // The pseudostack grows towards higher indices, so we iterate

    // backwards (from callee to caller).

    volatile js::ProfileEntry& entry = racyInfo->entries[i - 1];

    js::ProfileEntry& entry = racyInfo->entries[i - 1];

    if (!entry.isJs() && strcmp(entry.label(), "EnterJIT") == 0) {

      // Found JIT entry frame.  Unwind up to that point (i.e., force

      // the stack walk to stop before the block of saved registers;
@@ -2857,7 +2856,7 @@ profiler_get_backtrace_noalloc(char *output, size_t outputSize) 


  bool includeDynamicString = !ActivePS::FeaturePrivacy(lock);



  volatile js::ProfileEntry* pseudoEntries = pseudoStack->entries;

  js::ProfileEntry* pseudoEntries = pseudoStack->entries;

  uint32_t pseudoCount = pseudoStack->stackSize();



  for (uint32_t i = 0; i < pseudoCount; i++) {

xpcom/threads/ThreadStackHelper.cpp

+3 −3
Original line number Diff line number Diff line
@@ -405,7 +405,7 @@ GetPathAfterComponent(const char* filename, const char (&component)[LEN]) { 
} // namespace



const char*

ThreadStackHelper::AppendJSEntry(const volatile js::ProfileEntry* aEntry,

ThreadStackHelper::AppendJSEntry(const js::ProfileEntry* aEntry,

                                 intptr_t& aAvailableBufferSize,

                                 const char* aPrevLabel)

{
@@ -492,8 +492,8 @@ ThreadStackHelper::FillStackBuffer() 
  intptr_t availableBufferSize = intptr_t(reservedBufferSize);



  // Go from front to back

  const volatile js::ProfileEntry* entry = mPseudoStack->entries;

  const volatile js::ProfileEntry* end = entry + mPseudoStack->stackSize();

  const js::ProfileEntry* entry = mPseudoStack->entries;

  const js::ProfileEntry* end = entry + mPseudoStack->stackSize();

  // Deduplicate identical, consecutive frames

  const char* prevLabel = nullptr;

  for (; reservedSize-- && entry != end; entry++) {