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
--- 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