Bug 1369276 (part 2) - Convert ProfileEntry::Flags to Kind. r=shu.
authorNicholas Nethercote <nnethercote@mozilla.com>
Fri, 02 Jun 2017 12:46:09 +1000
changeset 362248 8726e36dafe1c875d86b33b5885f6b9e6d1a5a12
parent 362247 70e36d9d546e22b44ec55eb5906987d95bfcbcc7
child 362249 94a3fb9eb7e4c933ae318ff9b0a5bc9a3ac28e75
push id31966
push userarchaeopteryx@coole-files.de
push dateMon, 05 Jun 2017 09:06:08 +0000
treeherdermozilla-central@275588f4d852 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersshu
bugs1369276
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1369276 (part 2) - Convert ProfileEntry::Flags to Kind. r=shu. There are three flags in ProfileEntry::Flags, which suggests there are 2**3 = 8 combinations. But there are only actually 4 valid combinations. This patch converts the three flags to a single "kind" enum, which makes things clearer. Note also that the patch moves the condition at the start of AddPseudoEntry() to its callsite, for consistency with the earlier JS_OSR entry kind check.
js/public/ProfilingStack.h
js/src/vm/GeckoProfiler.cpp
tools/profiler/core/platform.cpp
tools/profiler/public/GeckoProfiler.h
--- a/js/public/ProfilingStack.h
+++ b/js/public/ProfilingStack.h
@@ -50,124 +50,118 @@ class ProfileEntry
     const char * volatile dynamicString_;
 
     // Stack pointer for non-JS entries, the script pointer otherwise.
     void * volatile spOrScript;
 
     // Line number for non-JS entries, the bytecode offset otherwise.
     int32_t volatile lineOrPcOffset;
 
-    // Flags are in the low bits. The category is in the high bits.
-    uint32_t volatile flagsAndCategory_;
+    // Bits 0..1 hold the Kind. Bits 2..3 are unused. Bits 4..12 hold the
+    // Category.
+    uint32_t volatile kindAndCategory_;
 
     static int32_t pcToOffset(JSScript* aScript, jsbytecode* aPc);
 
   public:
-    // These traits are bit masks. Make sure they're powers of 2.
-    enum Flags : uint32_t {
-        // Indicate whether a profile entry represents a CPP frame. If not set,
-        // a JS frame is assumed by default. You're not allowed to publicly
-        // change the frame type. Instead, initialize the ProfileEntry as either
-        // a JS or CPP frame with `initJsFrame` or `initCppFrame` respectively.
-        IS_CPP_ENTRY = 1u << 0,
+    enum class Kind : uint32_t {
+        // A normal C++ frame.
+        CPP_NORMAL = 0,
+
+        // A special C++ frame indicating the start of a run of JS pseudostack
+        // entries. CPP_MARKER_FOR_JS frames are ignored, except for the sp
+        // field.
+        CPP_MARKER_FOR_JS = 1,
 
-        // This ProfileEntry is a dummy entry indicating the start of a run
-        // of JS pseudostack entries.
-        BEGIN_PSEUDO_JS = 1u << 1,
+        // A normal JS frame.
+        JS_NORMAL = 2,
 
-        // This flag is used to indicate that an interpreter JS entry has OSR-ed
-        // into baseline.
-        OSR = 1u << 2,
+        // An interpreter JS frame that has OSR-ed into baseline. JS_NORMAL
+        // frames can be converted to JS_OSR and back. JS_OSR frames are
+        // ignored.
+        JS_OSR = 3,
 
-        // Union of all flags.
-        ALL = IS_CPP_ENTRY|BEGIN_PSEUDO_JS|OSR,
-
-        // Mask for removing all flags except the category information.
-        CATEGORY_MASK = ~ALL
+        KIND_MASK = 0x3,
     };
 
     // Keep these in sync with devtools/client/performance/modules/categories.js
     enum class Category : uint32_t {
         OTHER    = 1u << 4,
         CSS      = 1u << 5,
         JS       = 1u << 6,
         GC       = 1u << 7,
         CC       = 1u << 8,
         NETWORK  = 1u << 9,
         GRAPHICS = 1u << 10,
         STORAGE  = 1u << 11,
         EVENTS   = 1u << 12,
 
         FIRST    = OTHER,
-        LAST     = EVENTS
+        LAST     = EVENTS,
+
+        CATEGORY_MASK = ~uint32_t(Kind::KIND_MASK),
     };
 
-    static_assert((static_cast<int>(Category::FIRST) & Flags::ALL) == 0,
-                  "The category bitflags should not intersect with the other flags!");
+    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 { return hasFlag(IS_CPP_ENTRY); }
-    bool isJs() const volatile { return !isCpp(); }
+    bool isCpp() const volatile
+    {
+        Kind k = kind();
+        return k == Kind::CPP_NORMAL || k == Kind::CPP_MARKER_FOR_JS;
+    }
+
+    bool isJs() const volatile
+    {
+        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_; }
 
     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
+                      Kind aKind, Category aCategory) volatile
     {
         label_ = aLabel;
         dynamicString_ = aDynamicString;
         spOrScript = sp;
         lineOrPcOffset = static_cast<int32_t>(aLine);
-        flagsAndCategory_ = aFlags | js::ProfileEntry::IS_CPP_ENTRY | uint32_t(aCategory);
+        kindAndCategory_ = uint32_t(aKind) | uint32_t(aCategory);
+        MOZ_ASSERT(isCpp());
     }
 
     void initJsFrame(const char* aLabel, const char* aDynamicString, JSScript* aScript,
                      jsbytecode* aPc) volatile
     {
         label_ = aLabel;
         dynamicString_ = aDynamicString;
         spOrScript = aScript;
         lineOrPcOffset = pcToOffset(aScript, aPc);
-        flagsAndCategory_ = uint32_t(js::ProfileEntry::Category::JS);  // No flags needed.
-    }
-
-    void setFlag(uint32_t flag) volatile {
-        MOZ_ASSERT(flag != IS_CPP_ENTRY);
-        flagsAndCategory_ |= flag;
-    }
-    void unsetFlag(uint32_t flag) volatile {
-        MOZ_ASSERT(flag != IS_CPP_ENTRY);
-        flagsAndCategory_ &= ~flag;
-    }
-    bool hasFlag(uint32_t flag) const volatile {
-        return bool(flagsAndCategory_ & flag);
+        kindAndCategory_ = uint32_t(Kind::JS_NORMAL) | uint32_t(Category::JS);
+        MOZ_ASSERT(isJs());
     }
 
-    uint32_t category() const volatile {
-        return flagsAndCategory_ & CATEGORY_MASK;
+    void setKind(Kind aKind) volatile {
+        kindAndCategory_ = uint32_t(aKind) | uint32_t(category());
     }
 
-    void setOSR() volatile {
-        MOZ_ASSERT(isJs());
-        setFlag(OSR);
+    Kind kind() const volatile {
+        return Kind(kindAndCategory_ & uint32_t(Kind::KIND_MASK));
     }
-    void unsetOSR() volatile {
-        MOZ_ASSERT(isJs());
-        unsetFlag(OSR);
-    }
-    bool isOSR() const volatile {
-        return hasFlag(OSR);
+
+    Category category() const volatile {
+        return Category(kindAndCategory_ & uint32_t(Category::CATEGORY_MASK));
     }
 
     void* stackAddress() const volatile {
         MOZ_ASSERT(!isJs());
         return spOrScript;
     }
 
     JS_PUBLIC_API(JSScript*) script() const volatile;
@@ -219,20 +213,19 @@ class PseudoStack
     ~PseudoStack() {
         // The label macros keep a reference to the PseudoStack to avoid a TLS
         // access. If these are somehow not all cleared we will get a
         // use-after-free so better to crash now.
         MOZ_RELEASE_ASSERT(stackPointer == 0);
     }
 
     void pushCppFrame(const char* label, const char* dynamicString, void* sp, uint32_t line,
-                      js::ProfileEntry::Category category,
-                      js::ProfileEntry::Flags flags = js::ProfileEntry::Flags(0)) {
+                      js::ProfileEntry::Kind kind, js::ProfileEntry::Category category) {
         if (stackPointer < MaxEntries) {
-            entries[stackPointer].initCppFrame(label, dynamicString, sp, line, flags, category);
+            entries[stackPointer].initCppFrame(label, dynamicString, sp, line, kind, category);
         }
 
         // This must happen at the end! The compiler will not reorder this
         // update because stackPointer is Atomic.
         stackPointer++;
     }
 
     void pushJsFrame(const char* label, const char* dynamicString, JSScript* script,
--- a/js/src/vm/GeckoProfiler.cpp
+++ b/js/src/vm/GeckoProfiler.cpp
@@ -370,19 +370,20 @@ GeckoProfilerEntryMarker::GeckoProfilerE
     MOZ_GUARD_OBJECT_NOTIFIER_INIT;
     if (!profiler->installed()) {
         profiler = nullptr;
         return;
     }
     spBefore_ = profiler->stackPointer();
 
     // We want to push a CPP frame so the profiler can correctly order JS and native stacks.
+    // Only the sp value is important.
     profiler->pseudoStack_->pushCppFrame(
-        "js::RunScript", /* dynamicString = */ nullptr, /* sp = */ this, /* line = */ 0,
-        js::ProfileEntry::Category::OTHER, js::ProfileEntry::BEGIN_PSEUDO_JS);
+        /* label = */ "", /* dynamicString = */ nullptr, /* sp = */ this, /* line = */ 0,
+        ProfileEntry::Kind::CPP_MARKER_FOR_JS, ProfileEntry::Category::OTHER);
 
     profiler->pseudoStack_->pushJsFrame(
         "js::RunScript", /* dynamicString = */ nullptr, script, script->code());
 }
 
 GeckoProfilerEntryMarker::~GeckoProfilerEntryMarker()
 {
     if (profiler == nullptr)
@@ -401,17 +402,18 @@ AutoGeckoProfilerEntry::AutoGeckoProfile
     MOZ_GUARD_OBJECT_NOTIFIER_INIT;
     if (!profiler_->installed()) {
         profiler_ = nullptr;
         return;
     }
     spBefore_ = profiler_->stackPointer();
 
     profiler_->pseudoStack_->pushCppFrame(
-        label, /* dynamicString = */ nullptr, /* sp = */ this, /* line = */ 0, category);
+        label, /* dynamicString = */ nullptr, /* sp = */ this, /* line = */ 0,
+        ProfileEntry::Kind::CPP_NORMAL, category);
 }
 
 AutoGeckoProfilerEntry::~AutoGeckoProfilerEntry()
 {
     if (!profiler_)
         return;
 
     profiler_->pseudoStack_->pop();
@@ -434,33 +436,33 @@ GeckoProfilerBaselineOSRMarker::GeckoPro
         return;
     }
 
     spBefore_ = sp;
     if (sp == 0)
         return;
 
     volatile ProfileEntry& entry = profiler->pseudoStack_->entries[sp - 1];
-    MOZ_ASSERT(entry.isJs());
-    entry.setOSR();
+    MOZ_ASSERT(entry.kind() == ProfileEntry::Kind::JS_NORMAL);
+    entry.setKind(ProfileEntry::Kind::JS_OSR);
 }
 
 GeckoProfilerBaselineOSRMarker::~GeckoProfilerBaselineOSRMarker()
 {
     if (profiler == nullptr)
         return;
 
     uint32_t sp = profiler->stackPointer();
     MOZ_ASSERT(spBefore_ == sp);
     if (sp == 0)
         return;
 
     volatile ProfileEntry& entry = profiler->stack()[sp - 1];
-    MOZ_ASSERT(entry.isJs());
-    entry.unsetOSR();
+    MOZ_ASSERT(entry.kind() == ProfileEntry::Kind::JS_OSR);
+    entry.setKind(ProfileEntry::Kind::JS_NORMAL);
 }
 
 JS_PUBLIC_API(JSScript*)
 ProfileEntry::script() const volatile
 {
     MOZ_ASSERT(isJs());
     auto script = reinterpret_cast<JSScript*>(spOrScript);
     if (!script)
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -722,21 +722,18 @@ AddDynamicCodeLocationTag(ProfileBuffer*
   }
 }
 
 static void
 AddPseudoEntry(PSLockRef aLock, ProfileBuffer* aBuffer,
                volatile js::ProfileEntry& entry,
                NotNull<RacyThreadInfo*> aRacyInfo)
 {
-  // Pseudo-frames with the BEGIN_PSEUDO_JS flag are just annotations and
-  // should not be recorded in the profile.
-  if (entry.hasFlag(js::ProfileEntry::BEGIN_PSEUDO_JS)) {
-    return;
-  }
+  MOZ_ASSERT(entry.kind() == js::ProfileEntry::Kind::CPP_NORMAL ||
+             entry.kind() == js::ProfileEntry::Kind::JS_NORMAL);
 
   int lineno = -1;
 
   // First entry has kind CodeLocation.
   const char* label = entry.label();
   bool includeDynamicString = !ActivePS::FeaturePrivacy(aLock);
   const char* dynamicString =
     includeDynamicString ? entry.dynamicString() : nullptr;
@@ -792,17 +789,17 @@ AddPseudoEntry(PSLockRef aLock, ProfileB
       lineno = entry.line();
     }
   }
 
   if (lineno != -1) {
     aBuffer->addTag(ProfileBufferEntry::LineNumber(lineno));
   }
 
-  aBuffer->addTag(ProfileBufferEntry::Category(entry.category()));
+  aBuffer->addTag(ProfileBufferEntry::Category(uint32_t(entry.category())));
 }
 
 struct NativeStack
 {
   void** pc_array;
   void** sp_array;
   size_t size;
   size_t count;
@@ -906,23 +903,22 @@ MergeStacksIntoProfile(PSLockRef aLock, 
 
     if (pseudoIndex != pseudoCount) {
       volatile js::ProfileEntry& pseudoEntry = pseudoEntries[pseudoIndex];
 
       if (pseudoEntry.isCpp()) {
         lastPseudoCppStackAddr = (uint8_t*) pseudoEntry.stackAddress();
       }
 
-      // Skip any pseudo-stack JS frames which are marked isOSR. Pseudostack
-      // frames are marked isOSR when the JS interpreter enters a jit frame on
-      // a loop edge (via on-stack-replacement, or OSR). To avoid both the
-      // pseudoframe and jit frame being recorded (and showing up twice), the
-      // interpreter marks the interpreter pseudostack entry with the OSR flag
-      // to ensure that it doesn't get counted.
-      if (pseudoEntry.isJs() && pseudoEntry.isOSR()) {
+      // Skip any JS_OSR frames. Such frames are used when the JS interpreter
+      // enters a jit frame on a loop edge (via on-stack-replacement, or OSR).
+      // To avoid both the pseudoframe and jit frame being recorded (and
+      // showing up twice), the interpreter marks the interpreter pseudostack
+      // frame as JS_OSR to ensure that it doesn't get counted.
+      if (pseudoEntry.kind() == js::ProfileEntry::Kind::JS_OSR) {
           pseudoIndex++;
           continue;
       }
 
       MOZ_ASSERT(lastPseudoCppStackAddr);
       pseudoStackAddr = lastPseudoCppStackAddr;
     }
 
@@ -952,17 +948,22 @@ MergeStacksIntoProfile(PSLockRef aLock, 
                                jsStackAddr != nativeStackAddr);
     MOZ_ASSERT_IF(nativeStackAddr, nativeStackAddr != pseudoStackAddr &&
                                    nativeStackAddr != jsStackAddr);
 
     // Check to see if pseudoStack frame is top-most.
     if (pseudoStackAddr > jsStackAddr && pseudoStackAddr > nativeStackAddr) {
       MOZ_ASSERT(pseudoIndex < pseudoCount);
       volatile js::ProfileEntry& pseudoEntry = pseudoEntries[pseudoIndex];
-      AddPseudoEntry(aLock, aBuffer, pseudoEntry, racyInfo);
+
+      // Pseudo-frames with the CPP_MARKER_FOR_JS kind are just annotations and
+      // should not be recorded in the profile.
+      if (pseudoEntry.kind() != js::ProfileEntry::Kind::CPP_MARKER_FOR_JS) {
+        AddPseudoEntry(aLock, aBuffer, pseudoEntry, racyInfo);
+      }
       pseudoIndex++;
       continue;
     }
 
     // Check to see if JS jit stack frame is top-most
     if (jsStackAddr > nativeStackAddr) {
       MOZ_ASSERT(jsIndex >= 0);
       const JS::ProfilingFrameIterator::Frame& jsFrame = jsFrames[jsIndex];
--- a/tools/profiler/public/GeckoProfiler.h
+++ b/tools/profiler/public/GeckoProfiler.h
@@ -424,17 +424,18 @@ profiler_call_enter(const char* aLabel, 
                     const char* aDynamicString = nullptr)
 {
   // This function runs both on and off the main thread.
 
   PseudoStack* pseudoStack = sPseudoStack.get();
   if (!pseudoStack) {
     return pseudoStack;
   }
-  pseudoStack->pushCppFrame(aLabel, aDynamicString, aFrameAddress, aLine, aCategory);
+  pseudoStack->pushCppFrame(aLabel, aDynamicString, aFrameAddress, aLine,
+                            js::ProfileEntry::Kind::CPP_NORMAL, aCategory);
 
   // The handle is meant to support future changes but for now it is simply
   // used to avoid having to call TLSInfo::RacyInfo() in profiler_call_exit().
   return pseudoStack;
 }
 
 inline void
 profiler_call_exit(void* aHandle)