Bug 1434965 - Take 'streaming generation' into account when de-duplicating JIT frames. r?njn draft
authorMarkus Stange <mstange@themasta.com>
Sat, 10 Feb 2018 20:03:36 -0500
changeset 754066 850390a5b33080d6291f765feeed127007b6806c
parent 754065 8b6e67d8fa47290d272e230aa9ffb51875aad2fb
push id98744
push userbmo:mstange@themasta.com
push dateMon, 12 Feb 2018 19:50:57 +0000
reviewersnjn
bugs1434965
milestone60.0a1
Bug 1434965 - Take 'streaming generation' into account when de-duplicating JIT frames. r?njn MozReview-Commit-ID: KlhSPY4ZrOR
tools/profiler/core/ProfileBufferEntry.cpp
tools/profiler/core/ProfileBufferEntry.h
tools/profiler/core/ThreadInfo.cpp
--- a/tools/profiler/core/ProfileBufferEntry.cpp
+++ b/tools/profiler/core/ProfileBufferEntry.cpp
@@ -283,30 +283,39 @@ UniqueStacks::BeginStack(const FrameKey&
 }
 
 UniqueStacks::StackKey
 UniqueStacks::AppendFrame(const StackKey& aStack, const FrameKey& aFrame)
 {
   return StackKey(aStack, GetOrAddStackIndex(aStack), GetOrAddFrameIndex(aFrame));
 }
 
+uint32_t
+UniqueStacks::JITAddress::Hash() const
+{
+  uint32_t hash = 0;
+  hash = AddToHash(hash, mAddress);
+  hash = AddToHash(hash, mStreamingGen);
+  return hash;
+}
+
 uint32_t UniqueStacks::FrameKey::Hash() const
 {
   uint32_t hash = 0;
   if (!mLocation.IsEmpty()) {
     hash = mozilla::HashString(mLocation.get());
   }
   if (mLine.isSome()) {
     hash = mozilla::AddToHash(hash, *mLine);
   }
   if (mCategory.isSome()) {
     hash = mozilla::AddToHash(hash, *mCategory);
   }
   if (mJITAddress.isSome()) {
-    hash = mozilla::AddToHash(hash, *mJITAddress);
+    hash = mozilla::AddToHash(hash, mJITAddress->Hash());
     if (mJITDepth.isSome()) {
       hash = mozilla::AddToHash(hash, *mJITDepth);
     }
   }
   return hash;
 }
 
 UniqueStacks::UniqueStacks()
@@ -326,27 +335,29 @@ uint32_t UniqueStacks::GetOrAddStackInde
   index = mStackToIndexMap.Count();
   mStackToIndexMap.Put(aStack, index);
   StreamStack(aStack);
   return index;
 }
 
 MOZ_MUST_USE nsTArray<UniqueStacks::FrameKey>
 UniqueStacks::GetOrAddJITFrameKeysForAddress(JSContext* aContext,
-                                             void* aJITAddress)
+                                             const JITAddress& aJITAddress)
 {
   nsTArray<FrameKey>& frameKeys =
     *mAddressToJITFrameKeysMap.LookupOrAdd(aJITAddress);
 
   if (frameKeys.IsEmpty()) {
-    for (JS::ProfiledFrameHandle handle : JS::GetProfiledFrames(aContext,
-                                                                aJITAddress)) {
+    for (JS::ProfiledFrameHandle handle :
+           JS::GetProfiledFrames(aContext, aJITAddress.mAddress)) {
       // JIT frames with the same canonical address should be treated as the
       // same frame, so set the frame key's address to the canonical address.
-      FrameKey frameKey(handle.canonicalAddress(), frameKeys.Length());
+      FrameKey frameKey(
+        JITAddress{ handle.canonicalAddress(), aJITAddress.mStreamingGen },
+        frameKeys.Length());
       MaybeAddJITFrameIndex(aContext, frameKey, handle);
       frameKeys.AppendElement(frameKey);
     }
     MOZ_ASSERT(frameKeys.Length() > 0);
   }
 
   // Return a copy of the array.
   return nsTArray<FrameKey>(frameKeys);
@@ -874,18 +885,19 @@ ProfileBuffer::StreamSamplesToJSON(Splic
       } else if (e.Get().IsJitReturnAddr()) {
         numFrames++;
 
         // We can only process JitReturnAddr entries if we have a JSContext.
         MOZ_RELEASE_ASSERT(aContext);
 
         // A JIT frame may expand to multiple frames due to inlining.
         void* pc = e.Get().u.mPtr;
+        UniqueStacks::JITAddress address = { pc, aUniqueStacks.CurrentGen() };
         nsTArray<UniqueStacks::FrameKey> frameKeys =
-          aUniqueStacks.GetOrAddJITFrameKeysForAddress(aContext, pc);
+          aUniqueStacks.GetOrAddJITFrameKeysForAddress(aContext, address);
         for (const UniqueStacks::FrameKey& frameKey : frameKeys) {
           stack = aUniqueStacks.AppendFrame(stack, frameKey);
         }
 
         e.Next();
 
       } else {
         break;
--- a/tools/profiler/core/ProfileBufferEntry.h
+++ b/tools/profiler/core/ProfileBufferEntry.h
@@ -140,23 +140,47 @@ public:
 private:
   SpliceableChunkedJSONWriter mStringTableWriter;
   nsDataHashtable<nsCStringHashKey, uint32_t> mStringToIndexMap;
 };
 
 class UniqueStacks
 {
 public:
+  // We de-duplicate information about JIT frames based on the return address
+  // of the frame. However, if the same UniqueStacks object is used to stream
+  // profiler buffer contents more than once, then in the time between the two
+  // stream attempts ("streaming generations"), JIT code can have been freed
+  // and reallocated in the same areas of memory. Consequently, during the next
+  // streaming generation, we may see JIT return addresses that we've already
+  // seen before, but which now represent completely different JS functions.
+  // So we need to make sure that any de-duplication takes the streaming
+  // generation into account and does not only compare the address itself.
+  // The JITAddress struct packages the two together and can be used as a hash
+  // key.
+  struct JITAddress
+  {
+    void* mAddress;
+    uint32_t mStreamingGen;
+
+    uint32_t Hash() const;
+    bool operator==(const JITAddress& aRhs) const
+    {
+      return mAddress == aRhs.mAddress && mStreamingGen == aRhs.mStreamingGen;
+    }
+    bool operator!=(const JITAddress& aRhs) const { return !(*this == aRhs); }
+  };
+
   struct FrameKey {
     // This cannot be a std::string, as it is not memmove compatible, which
     // is used by nsHashTable
     nsCString mLocation;
     mozilla::Maybe<unsigned> mLine;
     mozilla::Maybe<unsigned> mCategory;
-    mozilla::Maybe<void*> mJITAddress;
+    mozilla::Maybe<JITAddress> mJITAddress;
     mozilla::Maybe<uint32_t> mJITDepth;
 
     explicit FrameKey(const char* aLocation)
      : mLocation(aLocation)
     {
       mHash = Hash();
     }
 
@@ -165,17 +189,17 @@ public:
      , mLine(aToCopy.mLine)
      , mCategory(aToCopy.mCategory)
      , mJITAddress(aToCopy.mJITAddress)
      , mJITDepth(aToCopy.mJITDepth)
     {
       mHash = Hash();
     }
 
-    FrameKey(void* aJITAddress, uint32_t aJITDepth)
+    FrameKey(const JITAddress& aJITAddress, uint32_t aJITDepth)
      : mJITAddress(mozilla::Some(aJITAddress))
      , mJITDepth(mozilla::Some(aJITDepth))
     {
       mHash = Hash();
     }
 
     uint32_t Hash() const;
     bool operator==(const FrameKey& aOther) const;
@@ -215,25 +239,32 @@ public:
     }
 
   private:
     uint32_t mHash;
   };
 
   explicit UniqueStacks();
 
+  // Needs to be called when using a UniqueStacks object again after having
+  // streamed entries that are no longer in the buffer (and which could have
+  // been GC'ed and their memory reused).
+  void AdvanceStreamingGeneration() { mStreamingGeneration++; }
+  uint32_t CurrentGen() { return mStreamingGeneration; }
+
   // Return a StackKey for aFrame as the stack's root frame (no prefix).
   MOZ_MUST_USE StackKey BeginStack(const FrameKey& aFrame);
 
   // Return a new StackKey that is obtained by appending aFrame to aStack.
   MOZ_MUST_USE StackKey AppendFrame(const StackKey& aStack,
                                     const FrameKey& aFrame);
 
   MOZ_MUST_USE nsTArray<FrameKey>
-  GetOrAddJITFrameKeysForAddress(JSContext* aContext, void* aJITAddress);
+  GetOrAddJITFrameKeysForAddress(JSContext* aContext,
+                                 const JITAddress& aJITAddress);
 
   MOZ_MUST_USE uint32_t GetOrAddFrameIndex(const FrameKey& aFrame);
   MOZ_MUST_USE uint32_t GetOrAddStackIndex(const StackKey& aStack);
 
   void SpliceFrameTableElements(SpliceableJSONWriter& aWriter);
   void SpliceStackTableElements(SpliceableJSONWriter& aWriter);
 
 private:
@@ -250,23 +281,27 @@ private:
 
 public:
   UniqueJSONStrings mUniqueStrings;
 
 private:
   // To avoid incurring JitcodeGlobalTable lookup costs for every JIT frame,
   // we cache the frame keys of frames keyed by JIT code address. All FrameKeys
   // in mAddressToJITFrameKeysMap are guaranteed to be in mFrameToIndexMap.
-  nsClassHashtable<nsPtrHashKey<void>, nsTArray<FrameKey>> mAddressToJITFrameKeysMap;
+  nsClassHashtable<nsGenericHashKey<JITAddress>, nsTArray<FrameKey>> mAddressToJITFrameKeysMap;
 
   SpliceableChunkedJSONWriter mFrameTableWriter;
   nsDataHashtable<nsGenericHashKey<FrameKey>, uint32_t> mFrameToIndexMap;
 
   SpliceableChunkedJSONWriter mStackTableWriter;
   nsDataHashtable<nsGenericHashKey<StackKey>, uint32_t> mStackToIndexMap;
+
+  // Used to avoid collisions between JITAddresses that refer to different
+  // frames.
+  uint32_t mStreamingGeneration;
 };
 
 //
 // Thread profile JSON Format
 // --------------------------
 //
 // The profile contains much duplicate information. The output JSON of the
 // profile attempts to deduplicate strings, frames, and stack prefixes, to cut
--- a/tools/profiler/core/ThreadInfo.cpp
+++ b/tools/profiler/core/ThreadInfo.cpp
@@ -78,16 +78,18 @@ ThreadInfo::StreamJSON(const ProfileBuff
                        const TimeStamp& aProcessStartTime, double aSinceTime)
 {
   UniquePtr<PartialThreadProfile> partialProfile = Move(mPartialProfile);
 
   UniquePtr<UniqueStacks> uniqueStacks = partialProfile
     ? Move(partialProfile->mUniqueStacks)
     : MakeUnique<UniqueStacks>();
 
+  uniqueStacks->AdvanceStreamingGeneration();
+
   UniquePtr<char[]> partialSamplesJSON;
   UniquePtr<char[]> partialMarkersJSON;
   if (partialProfile) {
     partialSamplesJSON = Move(partialProfile->mSamplesJSON);
     partialMarkersJSON = Move(partialProfile->mMarkersJSON);
   }
 
   aWriter.Start();
@@ -244,16 +246,18 @@ ThreadInfo::FlushSamplesAndMarkers(const
   // an existing array.
   //
   // Note that the UniqueStacks instance is persisted so that the frame-index
   // mapping is stable across JS shutdown.
   UniquePtr<UniqueStacks> uniqueStacks = mPartialProfile
     ? Move(mPartialProfile->mUniqueStacks)
     : MakeUnique<UniqueStacks>();
 
+  uniqueStacks->AdvanceStreamingGeneration();
+
   UniquePtr<char[]> samplesJSON;
   UniquePtr<char[]> markersJSON;
 
   {
     SpliceableChunkedJSONWriter b;
     b.StartBareList();
     bool haveSamples = false;
     {