Bug 1428076 - Fix bad JSON format when profiling workers that do nothing r=mstange
authorJulien Wajsberg <felash@gmail.com>
Mon, 08 Jan 2018 18:25:36 +0100
changeset 400737 fcf73947d244a6e44652435ad1eb5993ceab4de6
parent 400736 60a59819cf322121940c1de7f511483444e3199e
child 400738 167ef8dc0c60a49c9c7de5cdba48c276dec5d355
push id33315
push userrgurzau@mozilla.com
push dateThu, 25 Jan 2018 17:00:51 +0000
treeherdermozilla-central@53c5a199232a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1428076
milestone60.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 1428076 - Fix bad JSON format when profiling workers that do nothing r=mstange When the Gecko Profiler runs, we keep samples and markers for threads in some occasions (eg when a Worker ends). But we fail to account for the case where these threads have output no sample and no marker yet. MozReview-Commit-ID: 2IEghD0v5Qd
tools/profiler/core/ProfileBuffer.h
tools/profiler/core/ProfileBufferEntry.cpp
tools/profiler/core/ThreadInfo.cpp
--- a/tools/profiler/core/ProfileBuffer.h
+++ b/tools/profiler/core/ProfileBuffer.h
@@ -44,21 +44,21 @@ public:
 
   void CollectCodeLocation(
     const char* aLabel, const char* aStr, int aLineNumber,
     const mozilla::Maybe<js::ProfileEntry::Category>& aCategory);
 
   // Maximum size of a frameKey string that we'll handle.
   static const size_t kMaxFrameKeyLength = 512;
 
-  void StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
+  bool StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                            double aSinceTime, double* aOutFirstSampleTime,
                            JSContext* cx,
                            UniqueStacks& aUniqueStacks) const;
-  void StreamMarkersToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
+  bool StreamMarkersToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                            const mozilla::TimeStamp& aProcessStartTime,
                            double aSinceTime,
                            UniqueStacks& aUniqueStacks) const;
   void StreamPausedRangesToJSON(SpliceableJSONWriter& aWriter,
                                 double aSinceTime) const;
 
   // Find (via |aLS|) the most recent sample for the thread denoted by
   // |aThreadId| and clone it, patching in |aProcessStartTime| as appropriate.
--- a/tools/profiler/core/ProfileBufferEntry.cpp
+++ b/tools/profiler/core/ProfileBufferEntry.cpp
@@ -695,17 +695,18 @@ private:
 //     DynamicStringFragment("://perf-")
 //     DynamicStringFragment("html.io/")
 //     DynamicStringFragment("ac0da204")
 //     DynamicStringFragment("aaa44d75")
 //     DynamicStringFragment("a800.bun")
 //     DynamicStringFragment("dle.js:2")
 //     DynamicStringFragment("5)")
 //
-void
+// This method returns true if it wrote anything to the writer.
+bool
 ProfileBuffer::StreamSamplesToJSON(SpliceableJSONWriter& aWriter, int aThreadId,
                                    double aSinceTime,
                                    double* aOutFirstSampleTime,
                                    JSContext* aContext,
                                    UniqueStacks& aUniqueStacks) const
 {
   UniquePtr<char[]> strbuf = MakeUnique<char[]>(kMaxFrameKeyLength);
 
@@ -716,16 +717,17 @@ ProfileBuffer::StreamSamplesToJSON(Splic
     { \
       fprintf(stderr, "ProfileBuffer parse error: %s", msg); \
       MOZ_ASSERT(false, msg); \
       continue; \
     }
 
   EntryGetter e(*this);
   bool seenFirstSample = false;
+  bool haveSamples = false;
 
   for (;;) {
     // This block skips entries until we find the start of the next sample.
     // This is useful in three 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.
@@ -899,46 +901,53 @@ ProfileBuffer::StreamSamplesToJSON(Splic
     }
 
     if (e.Has() && e.Get().IsUnsharedMemory()) {
       sample.mUSS = Some(e.Get().u.mDouble);
       e.Next();
     }
 
     WriteSample(aWriter, sample);
+    haveSamples = true;
   }
 
+  return haveSamples;
   #undef ERROR_AND_CONTINUE
 }
 
-void
+// This method returns true if it wrote anything to the writer.
+bool
 ProfileBuffer::StreamMarkersToJSON(SpliceableJSONWriter& aWriter,
                                    int aThreadId,
                                    const TimeStamp& aProcessStartTime,
                                    double aSinceTime,
                                    UniqueStacks& aUniqueStacks) const
 {
   EntryGetter e(*this);
+  bool haveMarkers = false;
 
   // Stream all markers whose threadId matches aThreadId. We skip other entries,
   // because we process them in StreamSamplesToJSON().
   //
   // NOTE: The ThreadId of a marker is determined by its GetThreadId() method,
   // rather than ThreadId buffer entries, as markers can be added outside of
   // samples.
   while (e.Has()) {
     if (e.Get().IsMarker()) {
       const ProfilerMarker* marker = e.Get().u.mMarker;
       if (marker->GetTime() >= aSinceTime &&
           marker->GetThreadId() == aThreadId) {
         marker->StreamJSON(aWriter, aProcessStartTime, aUniqueStacks);
+        haveMarkers = true;
       }
     }
     e.Next();
   }
+
+  return haveMarkers;
 }
 
 static void
 AddPausedRange(SpliceableJSONWriter& aWriter, const char* aReason,
                const Maybe<double>& aStartTime, const Maybe<double>& aEndTime)
 {
   aWriter.Start();
   if (aStartTime) {
--- a/tools/profiler/core/ThreadInfo.cpp
+++ b/tools/profiler/core/ThreadInfo.cpp
@@ -90,19 +90,19 @@ ThreadInfo::StreamJSON(const ProfileBuff
                             aProcessStartTime,
                             mRegisterTime, mUnregisterTime,
                             aSinceTime, &firstSampleTime,
                             mContext,
                             mSavedStreamedSamples.get(),
                             mFirstSavedStreamedSampleTime,
                             mSavedStreamedMarkers.get(),
                             *mUniqueStacks);
-    mSavedStreamedSamples.reset();
+    mSavedStreamedSamples = nullptr;
     mFirstSavedStreamedSampleTime = 0.0;
-    mSavedStreamedMarkers.reset();
+    mSavedStreamedMarkers = nullptr;
 
     aWriter.StartObjectProperty("stackTable");
     {
       {
         JSONSchemaWriter schema(aWriter);
         schema.WriteField("prefix");
         schema.WriteField("frame");
       }
@@ -253,34 +253,74 @@ ThreadInfo::FlushSamplesAndMarkers(const
   //
   // Note that the UniqueStacks instance is persisted so that the frame-index
   // mapping is stable across JS shutdown.
   mUniqueStacks.emplace(mContext);
 
   {
     SpliceableChunkedJSONWriter b;
     b.StartBareList();
+    bool haveSamples = false;
     {
-      aBuffer.StreamSamplesToJSON(b, ThreadId(), /* aSinceTime = */ 0,
-                                  &mFirstSavedStreamedSampleTime,
-                                  mContext, *mUniqueStacks);
+      if (mSavedStreamedSamples) {
+        b.Splice(mSavedStreamedSamples.get());
+        haveSamples = true;
+      }
+
+      // We deliberately use a new variable instead of writing something like
+      // `haveSamples || aBuffer.StreamSamplesToJSON(...)` because we don't want
+      // to short-circuit the call.
+      bool streamedNewSamples =
+        aBuffer.StreamSamplesToJSON(b, ThreadId(), /* aSinceTime = */ 0,
+                                    &mFirstSavedStreamedSampleTime,
+                                    mContext, *mUniqueStacks);
+      haveSamples = haveSamples || streamedNewSamples;
     }
     b.EndBareList();
-    mSavedStreamedSamples = b.WriteFunc()->CopyData();
+
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1428076
+    // If we don't have any data, keep mSavedStreamSamples set to null. That
+    // way we won't try to splice it into the JSON later on, which would
+    // result in an invalid JSON due to stray commas.
+    if (haveSamples) {
+      mSavedStreamedSamples = b.WriteFunc()->CopyData();
+    } else {
+      mSavedStreamedSamples = nullptr;
+    }
   }
 
   {
     SpliceableChunkedJSONWriter b;
     b.StartBareList();
+    bool haveMarkers = false;
     {
-      aBuffer.StreamMarkersToJSON(b, ThreadId(), aProcessStartTime,
-                                  /* aSinceTime = */ 0, *mUniqueStacks);
+      if (mSavedStreamedMarkers) {
+        b.Splice(mSavedStreamedMarkers.get());
+        haveMarkers = true;
+      }
+
+      // We deliberately use a new variable instead of writing something like
+      // `haveMarkers || aBuffer.StreamMarkersToJSON(...)` because we don't want
+      // to short-circuit the call.
+      bool streamedNewMarkers =
+        aBuffer.StreamMarkersToJSON(b, ThreadId(), aProcessStartTime,
+                                    /* aSinceTime = */ 0, *mUniqueStacks);
+      haveMarkers = haveMarkers || streamedNewMarkers;
     }
     b.EndBareList();
-    mSavedStreamedMarkers = b.WriteFunc()->CopyData();
+
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1428076
+    // If we don't have any data, keep mSavedStreamMarkers set to null. That
+    // way we won't try to splice it into the JSON later on, which would
+    // result in an invalid JSON due to stray commas.
+    if (haveMarkers) {
+      mSavedStreamedMarkers = b.WriteFunc()->CopyData();
+    } else {
+      mSavedStreamedMarkers = nullptr;
+    }
   }
 
   // Reset the buffer. Attempting to symbolicate JS samples after mContext has
   // gone away will crash.
   aBuffer.Reset();
 }
 
 size_t