Bug 1379565 - Overhaul ProfileBuffer::StreamSamplesToJSON. r=mstange.
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 05 Jul 2017 21:29:29 +1000
changeset 368204 903992e46e072238158b289a6bf494280e478a13
parent 368203 824f3b09c46f3107e1f7f657ebee98461f33e4ff
child 368205 f6232176ba57ffdd3c06b201e0e7562256936c76
push id32159
push usercbook@mozilla.com
push dateTue, 11 Jul 2017 10:52:11 +0000
treeherdermozilla-central@b07db5d650b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1379565
milestone56.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 1379565 - Overhaul ProfileBuffer::StreamSamplesToJSON. r=mstange. The profiler writes ProfileBuffer entries in a particular order, and then later has to parse them, mostly in StreamSamplesToJSON(). That function's parsing code is poorly structured and rather gross, at least partly because no explicit grammar is identified. This patch identifies the grammar in a comment, and in the same comment also includes some examples of the more complicated subsequences. Once written down, the grammar is obviously suboptimal -- the |Sample| entries serve no useful purpose, for example -- but I will leave grammar improvements as follow-ups. The patch also rewrites the parser in a more typical fashion that obviously matches the grammar. The new parser is slightly more verbose but far easier to understand.
tools/profiler/core/ProfileBuffer.cpp
tools/profiler/core/ProfileBuffer.h
tools/profiler/core/ProfileBufferEntry.cpp
tools/profiler/core/platform.cpp
--- a/tools/profiler/core/ProfileBuffer.cpp
+++ b/tools/profiler/core/ProfileBuffer.cpp
@@ -81,37 +81,8 @@ ProfileBuffer::SizeOfIncludingThis(mozil
   // Measurement of the following members may be added later if DMD finds it
   // is worthwhile:
   // - memory pointed to by the elements within mEntries
   // - mStoredMarkers
 
   return n;
 }
 
-#define DYNAMIC_MAX_STRING 8192
-
-char*
-ProfileBuffer::processEmbeddedString(int aReadAheadPos, int* aEntriesConsumed,
-                                     char* aStrBuf)
-{
-  int strBufPos = 0;
-
-  // Read the string stored in mChars until the null character is seen.
-  bool seenNullByte = false;
-  while (aReadAheadPos != mWritePos && !seenNullByte) {
-    (*aEntriesConsumed)++;
-    ProfileBufferEntry readAheadEntry = mEntries[aReadAheadPos];
-    for (size_t pos = 0; pos < ProfileBufferEntry::kNumChars; pos++) {
-      aStrBuf[strBufPos] = readAheadEntry.u.mChars[pos];
-      if (aStrBuf[strBufPos] == '\0' || strBufPos == DYNAMIC_MAX_STRING-2) {
-        seenNullByte = true;
-        break;
-      }
-      strBufPos++;
-    }
-    if (!seenNullByte) {
-      aReadAheadPos = (aReadAheadPos + 1) % mEntrySize;
-    }
-  }
-  return aStrBuf;
-}
-
-
--- a/tools/profiler/core/ProfileBuffer.h
+++ b/tools/profiler/core/ProfileBuffer.h
@@ -37,16 +37,20 @@ public:
 
   // Add |aEntry| to the buffer, ignoring what kind of entry it is.
   void addEntry(const ProfileBufferEntry& aEntry);
 
   // Add to the buffer a sample start (ThreadId) entry for aThreadId. Also,
   // record the resulting generation and index in |aLS| if it's non-null.
   void addThreadIdEntry(int aThreadId, LastSample* aLS = nullptr);
 
+  // Maximum size of a dynamic string (including the terminating '\0' char)
+  // that we'll write to the ProfileBuffer.
+  static const size_t kMaxDynamicStringLength = 8192;
+
   void StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                            double aSinceTime, JSContext* cx,
                            UniqueStacks& aUniqueStacks);
   void StreamMarkersToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                            const mozilla::TimeStamp& aProcessStartTime,
                            double aSinceTime,
                            UniqueStacks& aUniqueStacks);
 
@@ -59,19 +63,17 @@ public:
   void addStoredMarker(ProfilerMarker* aStoredMarker);
 
   // The following two methods are not signal safe! They delete markers.
   void deleteExpiredStoredMarkers();
   void reset();
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
 
-protected:
-  char* processEmbeddedString(int aReadaheadPos, int* aEntriesConsumed,
-                              char* aStrBuf);
+private:
   int FindLastSampleOfThread(int aThreadId, const LastSample& aLS);
 
 public:
   // Circular buffer 'Keep One Slot Open' implementation for simplicity
   mozilla::UniquePtr<ProfileBufferEntry[]> mEntries;
 
   // Points to the next entry we will write to, which is also the one at which
   // we need to stop reading.
--- a/tools/profiler/core/ProfileBufferEntry.cpp
+++ b/tools/profiler/core/ProfileBufferEntry.cpp
@@ -2,16 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include <ostream>
 #include "platform.h"
 #include "mozilla/HashFunctions.h"
+#include "mozilla/Sprintf.h"
 
 #include "nsThreadUtils.h"
 #include "nsXULAppAPI.h"
 
 // JS
 #include "jsapi.h"
 #include "jsfriendapi.h"
 #include "js/TrackedOptimizationInfo.h"
@@ -528,17 +529,17 @@ void UniqueStacks::StreamFrame(const OnS
       mFrameTableWriter.EndObject();
     }
   }
 }
 
 struct ProfileSample
 {
   uint32_t mStack;
-  Maybe<double> mTime;
+  double mTime;
   Maybe<double> mResponsiveness;
   Maybe<double> mRSS;
   Maybe<double> mUSS;
 };
 
 static void WriteSample(SpliceableJSONWriter& aWriter, ProfileSample& aSample)
 {
   enum Schema : uint32_t {
@@ -548,195 +549,363 @@ static void WriteSample(SpliceableJSONWr
     RSS = 3,
     USS = 4
   };
 
   AutoArraySchemaWriter writer(aWriter);
 
   writer.IntElement(STACK, aSample.mStack);
 
-  if (aSample.mTime.isSome()) {
-    writer.DoubleElement(TIME, *aSample.mTime);
-  }
+  writer.DoubleElement(TIME, aSample.mTime);
 
   if (aSample.mResponsiveness.isSome()) {
     writer.DoubleElement(RESPONSIVENESS, *aSample.mResponsiveness);
   }
 
   if (aSample.mRSS.isSome()) {
     writer.DoubleElement(RSS, *aSample.mRSS);
   }
 
   if (aSample.mUSS.isSome()) {
     writer.DoubleElement(USS, *aSample.mUSS);
   }
 }
 
-void ProfileBuffer::StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
-                                        double aSinceTime, JSContext* aContext,
-                                        UniqueStacks& aUniqueStacks)
+class EntryGetter
 {
-  Maybe<ProfileSample> sample;
-  int readPos = mReadPos;
-  int currentThreadID = -1;
-  Maybe<double> currentTime;
-  UniquePtr<char[]> strbuf = MakeUnique<char[]>(DYNAMIC_MAX_STRING);
+public:
+  explicit EntryGetter(ProfileBuffer* aBuffer)
+    : mEntries(aBuffer->mEntries.get())
+    , mReadPos(aBuffer->mReadPos)
+    , mWritePos(aBuffer->mWritePos)
+    , mEntrySize(aBuffer->mEntrySize)
+  {}
+
+  bool Has() const { return mReadPos != mWritePos; }
+  const ProfileBufferEntry& Get() const { return mEntries[mReadPos]; }
+  void Next() { mReadPos = (mReadPos + 1) % mEntrySize; }
+
+private:
+  const ProfileBufferEntry* const mEntries;
+  int mReadPos;
+  const int mWritePos;
+  const int mEntrySize;
+};
+
+// Each sample is made up of multiple ProfileBuffer entries. The following
+// grammar shows legal sequences.
+//
+// (
+//   ThreadId
+//   Time
+//   Sample
+//   ( NativeLeafAddr
+//   | CodeLocation EmbeddedString* LineNumber? Category?
+//   | JitReturnAddr
+//   )+
+//   Marker*
+//   Responsiveness?
+//   ResidentMemory?
+//   UnsharedMemory?
+// )*
+//
+// The most complicated part is the stack entry sequence that begins with
+// CodeLocation. Here are some examples.
+//
+// - PseudoStack entries without a dynamic string (or with privacy enabled so
+//   that the dynamic string is hidden):
+//
+//     CodeLocation("js::RunScript")
+//     Category(ProfileEntry::Category::JS)
+//
+//     CodeLocation("XREMain::XRE_main")
+//     LineNumber(4660)
+//     Category(ProfileEntry::Category::OTHER)
+//
+//     CodeLocation("ElementRestyler::ComputeStyleChangeFor")
+//     LineNumber(3003)
+//     Category(ProfileEntry::Category::CSS)
+//
+// - PseudoStack entries with a dynamic string:
+//
+//     CodeLocation("")
+//     EmbeddedString("nsObserv")
+//     EmbeddedString("erServic")
+//     EmbeddedString("e::Notif")
+//     EmbeddedString("yObserve")
+//     EmbeddedString("rs profi")
+//     EmbeddedString("ler-star")
+//     EmbeddedString "ted")
+//     LineNumber(291)
+//     Category(ProfileEntry::Category::OTHER)
+//
+//     CodeLocation("")
+//     EmbeddedString("closeWin")
+//     EmbeddedString("dow (chr")
+//     EmbeddedString("ome://gl")
+//     EmbeddedString("obal/con")
+//     EmbeddedString("tent/glo")
+//     EmbeddedString("balOverl")
+//     EmbeddedString("ay.js:5)")
+//     EmbeddedString("")         # this string holds the closing '\0'
+//     LineNumber(25)
+//     Category(ProfileEntry::Category::JS)
+//
+//     CodeLocation("")
+//     EmbeddedString("bound (s")
+//     EmbeddedString("elf-host")
+//     EmbeddedString("ed:914)")
+//     LineNumber(945)
+//     Category(ProfileEntry::Category::JS)
+//
+// - A wasm JIT frame entry:
+//
+//     CodeLocation("")
+//     EmbeddedString("wasm-fun")
+//     EmbeddedString("ction[87")
+//     EmbeddedString("36] (blo")
+//     EmbeddedString("b:http:/")
+//     EmbeddedString("/webasse")
+//     EmbeddedString("mbly.org")
+//     EmbeddedString("/3dc5759")
+//     EmbeddedString("4-ce58-4")
+//     EmbeddedString("626-975b")
+//     EmbeddedString("-08ad116")
+//     EmbeddedString("30bc1:38")
+//     EmbeddedString("29856)")
+//
+// - A JS frame entry in a synchronous sample:
+//
+//     CodeLocation("")
+//     EmbeddedString("u (https")
+//     EmbeddedString("://perf-")
+//     EmbeddedString("html.io/")
+//     EmbeddedString("ac0da204")
+//     EmbeddedString("aaa44d75")
+//     EmbeddedString("a800.bun")
+//     EmbeddedString("dle.js:2")
+//     EmbeddedString("5)")
+//
+void
+ProfileBuffer::StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
+                                   double aSinceTime, JSContext* aContext,
+                                   UniqueStacks& aUniqueStacks)
+{
+  UniquePtr<char[]> strbuf = MakeUnique<char[]>(kMaxDynamicStringLength);
+
+  // Because this is a format entirely internal to the Profiler, any parsing
+  // error indicates a bug in the ProfileBuffer writing or the parser itself,
+  // or possibly flaky hardware.
+  #define ERROR_AND_SKIP_TO_NEXT_SAMPLE(msg) \
+    do { \
+      fprintf(stderr, "ProfileBuffer parse error: %s", msg); \
+      MOZ_ASSERT(false, msg); \
+      goto skip_to_next_sample; \
+    } while (0)
+
+  EntryGetter e(this);
 
-  while (readPos != mWritePos) {
-    ProfileBufferEntry entry = mEntries[readPos];
-    if (entry.isThreadId()) {
-      currentThreadID = entry.u.mInt;
-      currentTime.reset();
-      int readAheadPos = (readPos + 1) % mEntrySize;
-      if (readAheadPos != mWritePos) {
-        ProfileBufferEntry readAheadEntry = mEntries[readAheadPos];
-        if (readAheadEntry.isTime()) {
-          currentTime = Some(readAheadEntry.u.mDouble);
+  // This block skips entries until we find the start of the next sample. This
+  // is useful in two situations.
+  //
+  // - The circular buffer overwrites old entries, so when we start parsing we
+  //   might be in the middle of a sample, and we must skip forward to the
+  //   start of the next sample.
+  //
+  // - We skip samples that don't have an appropriate ThreadId or Time.
+  //
+skip_to_next_sample:
+  while (e.Has()) {
+    if (e.Get().isThreadId()) {
+      break;
+    } else {
+      e.Next();
+    }
+  }
+
+  while (e.Has()) {
+    if (e.Get().isThreadId()) {
+      int threadId = e.Get().u.mInt;
+      e.Next();
+
+      // Ignore samples that are for the wrong thread.
+      if (threadId != aThreadId) {
+        goto skip_to_next_sample;
+      }
+    } else {
+      // Due to the skip_to_next_sample block above, if we have an entry here
+      // it must be a ThreadId entry.
+      MOZ_CRASH();
+    }
+
+    ProfileSample sample;
+
+    if (e.Has() && e.Get().isTime()) {
+      sample.mTime = e.Get().u.mDouble;
+      e.Next();
+
+      // Ignore samples that are too old.
+      if (sample.mTime < aSinceTime) {
+        goto skip_to_next_sample;
+      }
+    } else {
+      ERROR_AND_SKIP_TO_NEXT_SAMPLE("expected a Time entry");
+    }
+
+    if (e.Has() && e.Get().isSample()) {
+      e.Next();
+    } else {
+      ERROR_AND_SKIP_TO_NEXT_SAMPLE("expected a Sample entry");
+    }
+
+    UniqueStacks::Stack 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));
+        e.Next();
+
+      } else if (e.Get().isCodeLocation()) {
+        numFrames++;
+        const char* string = e.Get().u.mString;
+        e.Next();
+
+        strbuf[0] = '\0';
+        char* p = &strbuf[0];
+        while (e.Has()) {
+          if (e.Get().isEmbeddedString()) {
+            if (p < &strbuf[kMaxDynamicStringLength]) {
+              // Copy 8 chars at a time. This won't overflow strbuf so long as
+              // its length is a multiple of 8.
+              static_assert(kMaxDynamicStringLength % 8 == 0, "bad strbuf sz");
+              memcpy(p, e.Get().u.mChars, ProfileBufferEntry::kNumChars);
+              p += ProfileBufferEntry::kNumChars;
+            }
+
+            e.Next();
+          } else {
+            break;
+          }
         }
+        strbuf[kMaxDynamicStringLength - 1] = '\0';
+
+        if (strbuf[0] != '\0') {
+          // When we have EmbeddedStrings, the preceding CodeLocation's string
+          // is always empty.
+          MOZ_ASSERT(string[0] == '\0');
+          string = strbuf.get();
+        }
+
+        UniqueStacks::OnStackFrameKey frameKey(string);
+
+        if (e.Has() && e.Get().isLineNumber()) {
+          frameKey.mLine = Some(unsigned(e.Get().u.mInt));
+          e.Next();
+        }
+
+        if (e.Has() && e.Get().isCategory()) {
+          frameKey.mCategory = Some(unsigned(e.Get().u.mInt));
+          e.Next();
+        }
+
+        stack.AppendFrame(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) {
+          StreamJSFramesOp framesOp(pc, stack);
+          MOZ_RELEASE_ASSERT(aContext);
+          JS::ForEachProfiledFrame(aContext, pc, framesOp);
+          aUniqueStacks.AddJITFrameDepth(pc, framesOp.depth());
+        } else {
+          for (unsigned i = 0; i < depth; i++) {
+            UniqueStacks::OnStackFrameKey inlineFrameKey(pc, i);
+            stack.AppendFrame(inlineFrameKey);
+          }
+        }
+
+        e.Next();
+
+      } else {
+        break;
       }
     }
-    if (currentThreadID == aThreadId && (currentTime.isNothing() || *currentTime >= aSinceTime)) {
-      switch (entry.kind()) {
-      case ProfileBufferEntry::Kind::Responsiveness:
-        if (sample.isSome()) {
-          sample->mResponsiveness = Some(entry.u.mDouble);
-        }
-        break;
-      case ProfileBufferEntry::Kind::ResidentMemory:
-        if (sample.isSome()) {
-          sample->mRSS = Some(entry.u.mDouble);
-        }
-        break;
-      case ProfileBufferEntry::Kind::UnsharedMemory:
-        if (sample.isSome()) {
-          sample->mUSS = Some(entry.u.mDouble);
-         }
+
+    if (numFrames == 0) {
+      ERROR_AND_SKIP_TO_NEXT_SAMPLE("expected one or more frame entries");
+    }
+
+    sample.mStack = stack.GetOrAddIndex();
+
+    // Skip over the markers. We process them in StreamMarkersToJSON().
+    while (e.Has()) {
+      if (e.Get().isMarker()) {
+        e.Next();
+      } else {
         break;
-      case ProfileBufferEntry::Kind::Sample:
-        {
-          // end the previous sample if there was one
-          if (sample.isSome()) {
-            WriteSample(aWriter, *sample);
-            sample.reset();
-          }
-          // begin the next sample
-          sample.emplace();
-          sample->mTime = currentTime;
-
-          // Process all the remaining entries within this sample.
-
-          UniqueStacks::Stack stack =
-            aUniqueStacks.BeginStack(UniqueStacks::OnStackFrameKey("(root)"));
-
-          int entryPos = (readPos + 1) % mEntrySize;
-          ProfileBufferEntry entry = mEntries[entryPos];
-          while (entryPos != mWritePos && !entry.isSample() &&
-                 !entry.isThreadId()) {
-            int incBy = 1;
-            entry = mEntries[entryPos];
-
-            // Read ahead to the next entry. If it's an EmbeddedString entry
-            // process it now.
-            const char* string = entry.u.mString;
-            int readAheadPos = (entryPos + 1) % mEntrySize;
-            // Make sure the string is always null terminated if it fills up
-            // DYNAMIC_MAX_STRING-2
-            strbuf[DYNAMIC_MAX_STRING-1] = '\0';
-
-            if (readAheadPos != mWritePos &&
-                mEntries[readAheadPos].isEmbeddedString()) {
-              string =
-                processEmbeddedString(readAheadPos, &incBy, strbuf.get());
-            }
+      }
+    }
 
-            // Write one entry. It can have either
-            // 1. only location - a NativeLeafAddr containing a memory address
-            // 2. location and line number - a CodeLocation followed by
-            //    EmbeddedStrings, an optional LineNumber and an
-            //    optional Category
-            // 3. a JitReturnAddress containing a native code address
-            if (entry.isNativeLeafAddr()) {
-              // 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)entry.u.mPtr;
-              snprintf(strbuf.get(), DYNAMIC_MAX_STRING, "%#llx", pc);
-              stack.AppendFrame(UniqueStacks::OnStackFrameKey(strbuf.get()));
+    if (e.Has() && e.Get().isResponsiveness()) {
+      sample.mResponsiveness = Some(e.Get().u.mDouble);
+      e.Next();
+    }
 
-            } else if (entry.isCodeLocation()) {
-              UniqueStacks::OnStackFrameKey frameKey(string);
-              readAheadPos = (entryPos + incBy) % mEntrySize;
-              if (readAheadPos != mWritePos &&
-                  mEntries[readAheadPos].isLineNumber()) {
-                frameKey.mLine = Some((unsigned) mEntries[readAheadPos].u.mInt);
-                incBy++;
-              }
-              readAheadPos = (entryPos + incBy) % mEntrySize;
-              if (readAheadPos != mWritePos &&
-                  mEntries[readAheadPos].isCategory()) {
-                frameKey.mCategory =
-                  Some((unsigned) mEntries[readAheadPos].u.mInt);
-                incBy++;
-              }
-              stack.AppendFrame(frameKey);
+    if (e.Has() && e.Get().isResidentMemory()) {
+      sample.mRSS = Some(e.Get().u.mDouble);
+      e.Next();
+    }
 
-            } else if (entry.isJitReturnAddr()) {
-              // A JIT frame may expand to multiple frames due to inlining.
-              void* pc = entry.u.mPtr;
-              unsigned depth = aUniqueStacks.LookupJITFrameDepth(pc);
-              if (depth == 0) {
-                StreamJSFramesOp framesOp(pc, stack);
-                MOZ_RELEASE_ASSERT(aContext);
-                JS::ForEachProfiledFrame(aContext, pc, framesOp);
-                aUniqueStacks.AddJITFrameDepth(pc, framesOp.depth());
-              } else {
-                for (unsigned i = 0; i < depth; i++) {
-                  UniqueStacks::OnStackFrameKey inlineFrameKey(pc, i);
-                  stack.AppendFrame(inlineFrameKey);
-                }
-              }
-            }
-            entryPos = (entryPos + incBy) % mEntrySize;
-          }
+    if (e.Has() && e.Get().isUnsharedMemory()) {
+      sample.mUSS = Some(e.Get().u.mDouble);
+      e.Next();
+    }
 
-          sample->mStack = stack.GetOrAddIndex();
-          break;
-        }
-      default:
-        break;
-      } /* switch (entry.kind()) */
-    }
-    readPos = (readPos + 1) % mEntrySize;
+    WriteSample(aWriter, sample);
   }
-  if (sample.isSome()) {
-    WriteSample(aWriter, *sample);
-  }
+
+  #undef ERROR_AND_SKIP_TO_NEXT_SAMPLE
 }
 
 void
 ProfileBuffer::StreamMarkersToJSON(SpliceableJSONWriter& aWriter,
                                    int aThreadId,
                                    const TimeStamp& aProcessStartTime,
                                    double aSinceTime,
                                    UniqueStacks& aUniqueStacks)
 {
-  int readPos = mReadPos;
+  EntryGetter e(this);
+
   int currentThreadID = -1;
-  while (readPos != mWritePos) {
-    ProfileBufferEntry entry = mEntries[readPos];
-    if (entry.isThreadId()) {
-      currentThreadID = entry.u.mInt;
-    } else if (currentThreadID == aThreadId && entry.isMarker()) {
-      const ProfilerMarker* marker = entry.u.mMarker;
+
+  // Stream all markers whose threadId matches aThreadId. All other entries are
+  // skipped, because we process them in StreamSamplesToJSON().
+  while (e.Has()) {
+    if (e.Get().isThreadId()) {
+      currentThreadID = e.Get().u.mInt;
+    } else if (currentThreadID == aThreadId && e.Get().isMarker()) {
+      const ProfilerMarker* marker = e.Get().u.mMarker;
       if (marker->GetTime() >= aSinceTime) {
-        entry.u.mMarker->StreamJSON(aWriter, aProcessStartTime, aUniqueStacks);
+        marker->StreamJSON(aWriter, aProcessStartTime, aUniqueStacks);
       }
     }
-    readPos = (readPos + 1) % mEntrySize;
+    e.Next();
   }
 }
 
 int
 ProfileBuffer::FindLastSampleOfThread(int aThreadId, const LastSample& aLS)
 {
   // |aLS| has a valid generation number if either it matches the buffer's
   // generation, or is one behind the buffer's generation, since the buffer's
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -1236,16 +1236,19 @@ DoNativeBacktrace(PSLockRef aLock, const
   lul->mStats.mFP      += framePointerFramesAcquired;
 }
 
 #endif
 
 // Writes some components shared by periodic and synchronous profiles to
 // ActivePS's ProfileBuffer. (This should only be called from DoSyncSample()
 // and DoPeriodicSample().)
+//
+// The grammar for entry sequences is in a comment above
+// ProfileBuffer::StreamSamplesToJSON.
 static inline void
 DoSharedSample(PSLockRef aLock, bool aIsSynchronous,
                ThreadInfo& aThreadInfo, const TimeStamp& aNow,
                const Registers& aRegs, ProfileBuffer::LastSample* aLS,
                ProfileBuffer* aBuffer)
 {
   // WARNING: this function runs within the profiler's "critical section".