Bug 1434965 - Remove Stack class and use intended-to-be-immutable StackKeys. r?njn draft
authorMarkus Stange <mstange@themasta.com>
Fri, 09 Feb 2018 18:18:34 -0500
changeset 753742 2a5e16ca1c1f3fdf2928c21071dc2976c24d20af
parent 753741 1d009b09578e00531cefc4cf9c5204c495f347ab
child 753743 4a0af31a0f4075ade93b2c501b2d421cffb33e70
child 754058 b8094547943730f7d3c1e582a3f13dbcb2ecabce
push id98666
push userbmo:mstange@themasta.com
push dateMon, 12 Feb 2018 06:22:14 +0000
reviewersnjn
bugs1434965
milestone60.0a1
Bug 1434965 - Remove Stack class and use intended-to-be-immutable StackKeys. r?njn I wasn't comfortable with how Stack had a reference to the UniqueStacks object. Now we compute new the new stack index after appending a frame by explicitly calling a method of the UniqueStacks object. MozReview-Commit-ID: KQAI9JNb4ag
tools/profiler/core/ProfileBufferEntry.cpp
tools/profiler/core/ProfileBufferEntry.h
--- a/tools/profiler/core/ProfileBufferEntry.cpp
+++ b/tools/profiler/core/ProfileBufferEntry.cpp
@@ -271,39 +271,26 @@ bool UniqueStacks::FrameKey::operator==(
 {
   return mLocation == aOther.mLocation &&
          mLine == aOther.mLine &&
          mCategory == aOther.mCategory &&
          mJITAddress == aOther.mJITAddress &&
          mJITDepth == aOther.mJITDepth;
 }
 
-bool UniqueStacks::StackKey::operator==(const StackKey& aOther) const
+UniqueStacks::StackKey
+UniqueStacks::BeginStack(const OnStackFrameKey& aFrame)
 {
-  MOZ_ASSERT_IF(mPrefix == aOther.mPrefix, mPrefixHash == aOther.mPrefixHash);
-  return mPrefix == aOther.mPrefix && mFrame == aOther.mFrame;
-}
-
-UniqueStacks::Stack::Stack(UniqueStacks& aUniqueStacks, const OnStackFrameKey& aRoot)
- : mUniqueStacks(aUniqueStacks)
- , mStack(aUniqueStacks.GetOrAddFrameIndex(aRoot))
-{
+  return StackKey(GetOrAddFrameIndex(aFrame));
 }
 
-void UniqueStacks::Stack::AppendFrame(const OnStackFrameKey& aFrame)
+UniqueStacks::StackKey
+UniqueStacks::AppendFrame(const StackKey& aStack, const OnStackFrameKey& aFrame)
 {
-  // Compute the prefix hash and index before mutating mStack.
-  uint32_t prefixHash = mStack.Hash();
-  uint32_t prefix = mUniqueStacks.GetOrAddStackIndex(mStack);
-  mStack.UpdateHash(prefixHash, prefix, mUniqueStacks.GetOrAddFrameIndex(aFrame));
-}
-
-uint32_t UniqueStacks::Stack::GetOrAddIndex() const
-{
-  return mUniqueStacks.GetOrAddStackIndex(mStack);
+  return StackKey(aStack, GetOrAddStackIndex(aStack), GetOrAddFrameIndex(aFrame));
 }
 
 uint32_t UniqueStacks::FrameKey::Hash() const
 {
   uint32_t hash = 0;
   if (!mLocation.IsEmpty()) {
     hash = mozilla::HashString(mLocation.get());
   }
@@ -317,29 +304,16 @@ uint32_t UniqueStacks::FrameKey::Hash() 
     hash = mozilla::AddToHash(hash, *mJITAddress);
     if (mJITDepth.isSome()) {
       hash = mozilla::AddToHash(hash, *mJITDepth);
     }
   }
   return hash;
 }
 
-uint32_t UniqueStacks::StackKey::Hash() const
-{
-  if (mPrefix.isNothing()) {
-    return mozilla::HashGeneric(mFrame);
-  }
-  return mozilla::AddToHash(*mPrefixHash, mFrame);
-}
-
-UniqueStacks::Stack UniqueStacks::BeginStack(const OnStackFrameKey& aRoot)
-{
-  return Stack(*this, aRoot);
-}
-
 UniqueStacks::UniqueStacks(JSContext* aContext)
  : mContext(aContext)
  , mFrameCount(0)
 {
   mFrameTableWriter.StartBareList();
   mStackTableWriter.StartBareList();
 }
 
@@ -417,20 +391,20 @@ void UniqueStacks::SpliceStackTableEleme
 void UniqueStacks::StreamStack(const StackKey& aStack)
 {
   enum Schema : uint32_t {
     PREFIX = 0,
     FRAME = 1
   };
 
   AutoArraySchemaWriter writer(mStackTableWriter, mUniqueStrings);
-  if (aStack.mPrefix.isSome()) {
-    writer.IntElement(PREFIX, *aStack.mPrefix);
+  if (aStack.mPrefixStackIndex.isSome()) {
+    writer.IntElement(PREFIX, *aStack.mPrefixStackIndex);
   }
-  writer.IntElement(FRAME, aStack.mFrame);
+  writer.IntElement(FRAME, aStack.mFrameIndex);
 }
 
 void UniqueStacks::StreamFrame(const OnStackFrameKey& aFrame)
 {
   enum Schema : uint32_t {
     LOCATION = 0,
     IMPLEMENTATION = 1,
     OPTIMIZATIONS = 2,
@@ -747,30 +721,30 @@ ProfileBuffer::StreamSamplesToJSON(Splic
       // Ignore samples that are too old.
       if (sample.mTime < aSinceTime) {
         continue;
       }
     } else {
       ERROR_AND_CONTINUE("expected a Time entry");
     }
 
-    UniqueStacks::Stack stack =
+    UniqueStacks::StackKey stack =
       aUniqueStacks.BeginStack(UniqueStacks::OnStackFrameKey("(root)"));
 
     int numFrames = 0;
     while (e.Has()) {
       if (e.Get().IsNativeLeafAddr()) {
         numFrames++;
 
         // Bug 753041: We need a double cast here to tell GCC that we don't
         // want to sign extend 32-bit addresses starting with 0xFXXXXXX.
         unsigned long long pc = (unsigned long long)(uintptr_t)e.Get().u.mPtr;
         char buf[20];
         SprintfLiteral(buf, "%#llx", pc);
-        stack.AppendFrame(UniqueStacks::OnStackFrameKey(buf));
+        stack = aUniqueStacks.AppendFrame(stack, UniqueStacks::OnStackFrameKey(buf));
         e.Next();
 
       } else if (e.Get().IsLabel()) {
         numFrames++;
 
         // Copy the label into strbuf.
         const char* label = e.Get().u.mString;
         strncpy(strbuf.get(), label, kMaxFrameKeyLength);
@@ -812,52 +786,52 @@ ProfileBuffer::StreamSamplesToJSON(Splic
           e.Next();
         }
 
         if (e.Has() && e.Get().IsCategory()) {
           frameKey.mCategory = Some(unsigned(e.Get().u.mInt));
           e.Next();
         }
 
-        stack.AppendFrame(frameKey);
+        stack = aUniqueStacks.AppendFrame(stack, frameKey);
 
       } else if (e.Get().IsJitReturnAddr()) {
         numFrames++;
 
         // A JIT frame may expand to multiple frames due to inlining.
         void* pc = e.Get().u.mPtr;
         unsigned depth = aUniqueStacks.LookupJITFrameDepth(pc);
         if (depth == 0) {
           MOZ_RELEASE_ASSERT(aContext);
           for (JS::ProfiledFrameHandle handle : JS::GetProfiledFrames(aContext, pc)) {
             UniqueStacks::OnStackFrameKey frameKey(pc, depth, handle);
-            stack.AppendFrame(frameKey);
+            stack = aUniqueStacks.AppendFrame(stack, frameKey);
             depth++;
           }
           MOZ_ASSERT(depth > 0);
           aUniqueStacks.AddJITFrameDepth(pc, depth);
         } else {
           for (unsigned i = 0; i < depth; i++) {
             UniqueStacks::OnStackFrameKey inlineFrameKey(pc, i);
-            stack.AppendFrame(inlineFrameKey);
+            stack = aUniqueStacks.AppendFrame(stack, inlineFrameKey);
           }
         }
 
         e.Next();
 
       } else {
         break;
       }
     }
 
     if (numFrames == 0) {
       ERROR_AND_CONTINUE("expected one or more frame entries");
     }
 
-    sample.mStack = stack.GetOrAddIndex();
+    sample.mStack = aUniqueStacks.GetOrAddStackIndex(stack);
 
     // Skip over the markers. We process them in StreamMarkersToJSON().
     while (e.Has()) {
       if (e.Get().IsMarker()) {
         e.Next();
       } else {
         break;
       }
--- a/tools/profiler/core/ProfileBufferEntry.h
+++ b/tools/profiler/core/ProfileBufferEntry.h
@@ -238,66 +238,64 @@ public:
     OnStackFrameKey(void* aJITAddress, unsigned aJITDepth,
                     const JS::ProfiledFrameHandle& aJITFrameHandle)
       : FrameKey(aJITAddress, aJITDepth)
       , mJITFrameHandle(&aJITFrameHandle)
     { }
   };
 
   struct StackKey {
-    mozilla::Maybe<uint32_t> mPrefixHash;
-    mozilla::Maybe<uint32_t> mPrefix;
-    uint32_t mFrame;
+    mozilla::Maybe<uint32_t> mPrefixStackIndex;
+    uint32_t mFrameIndex;
 
     explicit StackKey(uint32_t aFrame)
-     : mFrame(aFrame)
-    {
-      mHash = Hash();
-    }
+     : mFrameIndex(aFrame)
+     , mHash(mozilla::HashGeneric(aFrame))
+    {}
+
+    StackKey(const StackKey& aPrefix, uint32_t aPrefixStackIndex, uint32_t aFrame)
+      : mPrefixStackIndex(mozilla::Some(aPrefixStackIndex))
+      , mFrameIndex(aFrame)
+      , mHash(mozilla::AddToHash(aPrefix.mHash, aFrame))
+    {}
 
-    uint32_t Hash() const;
-    bool operator==(const StackKey& aOther) const;
-    bool operator<(const StackKey& aOther) const {
-      return mHash < aOther.mHash;
-    }
+    uint32_t Hash() const { return mHash; }
 
-    void UpdateHash(uint32_t aPrefixHash, uint32_t aPrefix, uint32_t aFrame) {
-      mPrefixHash = mozilla::Some(aPrefixHash);
-      mPrefix = mozilla::Some(aPrefix);
-      mFrame = aFrame;
-      mHash = Hash();
+    bool operator==(const StackKey& aOther) const
+    {
+      return mPrefixStackIndex == aOther.mPrefixStackIndex &&
+             mFrameIndex == aOther.mFrameIndex;
+    }
+    bool operator<(const StackKey& aOther) const
+    {
+      return mHash < aOther.mHash;
     }
 
   private:
     uint32_t mHash;
   };
 
-  class Stack {
-  public:
-    Stack(UniqueStacks& aUniqueStacks, const OnStackFrameKey& aRoot);
-
-    void AppendFrame(const OnStackFrameKey& aFrame);
-    uint32_t GetOrAddIndex() const;
-
-  private:
-    UniqueStacks& mUniqueStacks;
-    StackKey mStack;
-  };
-
   explicit UniqueStacks(JSContext* aContext);
 
-  Stack BeginStack(const OnStackFrameKey& aRoot);
+  // Return a StackKey for aFrame as the stack's root frame (no prefix).
+  MOZ_MUST_USE StackKey BeginStack(const OnStackFrameKey& aFrame);
+
+  // Return a new StackKey that is obtained by appending aFrame to aStack.
+  MOZ_MUST_USE StackKey AppendFrame(const StackKey& aStack,
+                                    const OnStackFrameKey& aFrame);
+
+  MOZ_MUST_USE uint32_t GetOrAddFrameIndex(const OnStackFrameKey& aFrame);
+  MOZ_MUST_USE uint32_t GetOrAddStackIndex(const StackKey& aStack);
+
   uint32_t LookupJITFrameDepth(void* aAddr);
   void AddJITFrameDepth(void* aAddr, unsigned depth);
   void SpliceFrameTableElements(SpliceableJSONWriter& aWriter);
   void SpliceStackTableElements(SpliceableJSONWriter& aWriter);
 
 private:
-  uint32_t GetOrAddFrameIndex(const OnStackFrameKey& aFrame);
-  uint32_t GetOrAddStackIndex(const StackKey& aStack);
   void StreamFrame(const OnStackFrameKey& aFrame);
   void StreamStack(const StackKey& aStack);
 
 public:
   UniqueJSONStrings mUniqueStrings;
 
 private:
   JSContext* mContext;