Backed out 3 changesets (bug 1734867) for causing assertion failures at ProfilerMarkers.cpp. CLOSED TREE

Backed out changeset bcddb167725c (bug 1734867)
Backed out changeset 7e01973ce58a (bug 1734867)
Backed out changeset 8b3b1ab8d743 (bug 1734867)

mozglue/baseprofiler/core/ProfileBuffer.h

@@ -135,17 +135,10 @@ class ProfileBuffer final {
   // - Adding JIT info.
   // - Streaming stacks to JSON.
   // Mutable because it's accessed from non-multithreaded const methods.
-  mutable Maybe<ProfileBufferChunkManagerSingle> mMaybeWorkerChunkManager;
-  ProfileBufferChunkManagerSingle& WorkerChunkManager() const {
-    if (mMaybeWorkerChunkManager.isNothing()) {
-      // Only actually allocate it on first use. (Some ProfileBuffers are
-      // temporary and don't actually need this.)
-      mMaybeWorkerChunkManager.emplace(
+  mutable ProfileBufferChunkManagerSingle mWorkerChunkManager{
+      ProfileBufferChunk::Create(
           ProfileBufferChunk::SizeofChunkMetadata() +
-          ProfileBufferChunkManager::scExpectedMaximumStackSize);
-    }
-    return *mMaybeWorkerChunkManager;
-  }
+          ProfileBufferChunkManager::scExpectedMaximumStackSize)};
 
   // Time from launch (us) when first sampling was recorded.
   double mFirstSamplingTimeUs = 0.0;

mozglue/baseprofiler/core/ProfileBufferEntry.cpp

@@ -1210,10 +1210,10 @@ bool ProfileBuffer::DuplicateLastSample(BaseProfilerThreadId aThreadId,
   }
 
   ProfileChunkedBuffer tempBuffer(
-      ProfileChunkedBuffer::ThreadSafety::WithoutMutex, WorkerChunkManager());
+      ProfileChunkedBuffer::ThreadSafety::WithoutMutex, mWorkerChunkManager);
 
   auto retrieveWorkerChunk = MakeScopeExit(
-      [&]() { WorkerChunkManager().Reset(tempBuffer.GetAllChunks()); });
+      [&]() { mWorkerChunkManager.Reset(tempBuffer.GetAllChunks()); });
 
   const bool ok = mEntries.Read([&](ProfileChunkedBuffer::Reader* aReader) {
     MOZ_ASSERT(aReader,

mozglue/baseprofiler/core/ProfilerMarkers.cpp

@@ -5,7 +5,7 @@
 
 #include "mozilla/BaseProfilerMarkers.h"
 
-#include "mozilla/BaseProfilerUtils.h"
+#include "mozilla/Likely.h"
 
 #include <limits>
 
@@ -74,51 +74,6 @@ Streaming::MarkerTypeFunctionsArray() {
   return {sMarkerTypeFunctions1Based, sDeserializerCount};
 }
 
-// Only accessed on the main thread.
-// Both profilers (Base and Gecko) could be active at the same time, so keep a
-// ref-count to only allocate at most one buffer at any time.
-static int sBufferForMainThreadAddMarkerRefCount = 0;
-static ProfileChunkedBuffer* sBufferForMainThreadAddMarker = nullptr;
-
-ProfileChunkedBuffer* GetClearedBufferForMainThreadAddMarker() {
-  if (!mozilla::baseprofiler::profiler_is_main_thread()) {
-    return nullptr;
-  }
-
-  if (sBufferForMainThreadAddMarker) {
-    sBufferForMainThreadAddMarker->Clear();
-  }
-
-  return sBufferForMainThreadAddMarker;
-}
-
-MFBT_API void EnsureBufferForMainThreadAddMarker() {
-  if (!mozilla::baseprofiler::profiler_is_main_thread()) {
-    return;
-  }
-
-  if (sBufferForMainThreadAddMarkerRefCount++ == 0) {
-    MOZ_ASSERT(!sBufferForMainThreadAddMarker);
-    sBufferForMainThreadAddMarker = new ProfileChunkedBuffer(
-        ProfileChunkedBuffer::ThreadSafety::WithoutMutex,
-        MakeUnique<ProfileBufferChunkManagerSingle>(
-            ProfileBufferChunkManager::scExpectedMaximumStackSize));
-  }
-}
-
-MFBT_API void ReleaseBufferForMainThreadAddMarker() {
-  if (!mozilla::baseprofiler::profiler_is_main_thread()) {
-    return;
-  }
-
-  MOZ_ASSERT(sBufferForMainThreadAddMarkerRefCount > 0);
-  MOZ_ASSERT(sBufferForMainThreadAddMarker);
-  if (--sBufferForMainThreadAddMarkerRefCount == 0) {
-    delete sBufferForMainThreadAddMarker;
-    sBufferForMainThreadAddMarker = nullptr;
-  }
-}
-
 }  // namespace base_profiler_markers_detail
 
 void MarkerSchema::Stream(JSONWriter& aWriter,

mozglue/baseprofiler/core/platform.cpp

@@ -3065,8 +3065,6 @@ static void locked_profiler_start(PSLockRef aLock, PowerOfTwo32 aCapacity,
 
   MOZ_RELEASE_ASSERT(CorePS::Exists() && !ActivePS::Exists(aLock));
 
-  mozilla::base_profiler_markers_detail::EnsureBufferForMainThreadAddMarker();
-
 #if defined(GP_PLAT_amd64_windows)
   InitializeWin64ProfilerHooks();
 #endif
@@ -3227,8 +3225,6 @@ void profiler_ensure_started(PowerOfTwo32 aCapacity, double aInterval,
   SamplerThread* samplerThread = ActivePS::Destroy(aLock);
   samplerThread->Stop(aLock);
 
-  mozilla::base_profiler_markers_detail::ReleaseBufferForMainThreadAddMarker();
-
   return samplerThread;
 }
 

mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h

@@ -253,14 +253,6 @@ static ProfileBufferBlockIndex AddMarkerWithOptionalStackToBuffer(
 using BacktraceCaptureFunction = bool (*)(ProfileChunkedBuffer&,
                                           StackCaptureOptions);
 
-// Use a pre-allocated and cleared chunked buffer in the main thread's
-// `AddMarkerToBuffer()`.
-// Null if not the main thread, or if profilers are not active.
-MFBT_API ProfileChunkedBuffer* GetClearedBufferForMainThreadAddMarker();
-// Called by the profiler(s) when starting/stopping. Safe to nest.
-MFBT_API void EnsureBufferForMainThreadAddMarker();
-MFBT_API void ReleaseBufferForMainThreadAddMarker();
-
 // Add a marker with the given name, options, and arguments to the given buffer.
 // Because this may be called from either Base or Gecko Profiler functions, the
 // appropriate backtrace-capturing function must also be provided.
@@ -284,31 +276,19 @@ ProfileBufferBlockIndex AddMarkerToBuffer(
     // A capture was requested, let's attempt to do it here&now. This avoids a
     // lot of allocations that would be necessary if capturing a backtrace
     // separately.
-    // TODO reduce internal profiler stack levels, see bug 1659872.
-    auto CaptureStackAndAddMarker = [&](ProfileChunkedBuffer& aChunkedBuffer) {
-      aOptions.StackRef().UseRequestedBacktrace(
-          aBacktraceCaptureFunction(aChunkedBuffer, captureOptions)
-              ? &aChunkedBuffer
-              : nullptr);
-      // This call must be made from here, while chunkedBuffer is in scope.
-      return AddMarkerWithOptionalStackToBuffer<MarkerType>(
-          aBuffer, aName, aCategory, std::move(aOptions), aTs...);
-    };
-
-    if (ProfileChunkedBuffer* buffer = GetClearedBufferForMainThreadAddMarker();
-        buffer) {
-      // Use a pre-allocated buffer for the main thread (because it's the most
-      // used thread, and most sensitive to overhead), so it's only allocated
-      // once. It could be null if this is not the main thread, or no profilers
-      // are currently active.
-      return CaptureStackAndAddMarker(*buffer);
-    }
     // TODO use a local on-stack byte buffer to remove last allocation.
+    // TODO reduce internal profiler stack levels, see bug 1659872.
     ProfileBufferChunkManagerSingle chunkManager(
         ProfileBufferChunkManager::scExpectedMaximumStackSize);
     ProfileChunkedBuffer chunkedBuffer(
         ProfileChunkedBuffer::ThreadSafety::WithoutMutex, chunkManager);
-    return CaptureStackAndAddMarker(chunkedBuffer);
+    aOptions.StackRef().UseRequestedBacktrace(
+        aBacktraceCaptureFunction(chunkedBuffer, captureOptions)
+            ? &chunkedBuffer
+            : nullptr);
+    // This call must be made from here, while chunkedBuffer is in scope.
+    return AddMarkerWithOptionalStackToBuffer<MarkerType>(
+        aBuffer, aName, aCategory, std::move(aOptions), aTs...);
   }
 
   return AddMarkerWithOptionalStackToBuffer<MarkerType>(

tools/profiler/core/ProfileBuffer.h

@@ -178,18 +178,10 @@ class ProfileBuffer final {
   // - Adding JIT info.
   // - Streaming stacks to JSON.
   // Mutable because it's accessed from non-multithreaded const methods.
-  mutable mozilla::Maybe<mozilla::ProfileBufferChunkManagerSingle>
-      mMaybeWorkerChunkManager;
-  mozilla::ProfileBufferChunkManagerSingle& WorkerChunkManager() const {
-    if (mMaybeWorkerChunkManager.isNothing()) {
-      // Only actually allocate it on first use. (Some ProfileBuffers are
-      // temporary and don't actually need this.)
-      mMaybeWorkerChunkManager.emplace(
+  mutable mozilla::ProfileBufferChunkManagerSingle mWorkerChunkManager{
+      mozilla::ProfileBufferChunk::Create(
           mozilla::ProfileBufferChunk::SizeofChunkMetadata() +
-          mozilla::ProfileBufferChunkManager::scExpectedMaximumStackSize);
-    }
-    return *mMaybeWorkerChunkManager;
-  }
+          mozilla::ProfileBufferChunkManager::scExpectedMaximumStackSize)};
 
   // GetStreamingParametersForThreadCallback:
   //   (ProfilerThreadId) -> Maybe<StreamingParametersForThread>

tools/profiler/core/ProfileBufferEntry.cpp

@@ -1184,7 +1184,7 @@ ProfilerThreadId ProfileBuffer::DoStreamSamplesAndMarkersToJSON(
           if (kind == ProfileBufferEntry::Kind::CompactStack) {
             ProfileChunkedBuffer tempBuffer(
                 ProfileChunkedBuffer::ThreadSafety::WithoutMutex,
-                WorkerChunkManager());
+                mWorkerChunkManager);
             er.ReadIntoObject(tempBuffer);
             tempBuffer.Read([&](ProfileChunkedBuffer::Reader* aReader) {
               MOZ_ASSERT(aReader,
@@ -1195,7 +1195,7 @@ ProfilerThreadId ProfileBuffer::DoStreamSamplesAndMarkersToJSON(
                         it.CurrentBlockIndex().ConvertToProfileBufferIndex(),
                         unresponsiveDuration, runningTimes);
             });
-            WorkerChunkManager().Reset(tempBuffer.GetAllChunks());
+            mWorkerChunkManager.Reset(tempBuffer.GetAllChunks());
             break;
           }
 
@@ -1394,7 +1394,7 @@ void ProfileBuffer::AddJITInfoForRange(uint64_t aRangeStart,
                 if (kind == ProfileBufferEntry::Kind::CompactStack) {
                   ProfileChunkedBuffer tempBuffer(
                       ProfileChunkedBuffer::ThreadSafety::WithoutMutex,
-                      WorkerChunkManager());
+                      mWorkerChunkManager);
                   er.ReadIntoObject(tempBuffer);
                   tempBuffer.Read([&](ProfileChunkedBuffer::Reader* aReader) {
                     MOZ_ASSERT(
@@ -1408,7 +1408,7 @@ void ProfileBuffer::AddJITInfoForRange(uint64_t aRangeStart,
                       stackEntryGetter.Next();
                     }
                   });
-                  WorkerChunkManager().Reset(tempBuffer.GetAllChunks());
+                  mWorkerChunkManager.Reset(tempBuffer.GetAllChunks());
                   break;
                 }
 
@@ -1939,10 +1939,10 @@ bool ProfileBuffer::DuplicateLastSample(ProfilerThreadId aThreadId,
   AUTO_PROFILER_STATS(DuplicateLastSample_copy);
 
   ProfileChunkedBuffer tempBuffer(
-      ProfileChunkedBuffer::ThreadSafety::WithoutMutex, WorkerChunkManager());
+      ProfileChunkedBuffer::ThreadSafety::WithoutMutex, mWorkerChunkManager);
 
   auto retrieveWorkerChunk = MakeScopeExit(
-      [&]() { WorkerChunkManager().Reset(tempBuffer.GetAllChunks()); });
+      [&]() { mWorkerChunkManager.Reset(tempBuffer.GetAllChunks()); });
 
   const bool ok = mEntries.Read([&](ProfileChunkedBuffer::Reader* aReader) {
     MOZ_ASSERT(aReader,

tools/profiler/core/platform.cpp

@@ -5281,10 +5281,6 @@ static void locked_profiler_start(PSLockRef aLock, PowerOfTwo32 aCapacity,
 
   MOZ_RELEASE_ASSERT(CorePS::Exists() && !ActivePS::Exists(aLock));
 
-  // Do this before the Base Profiler is stopped, to keep the existing buffer
-  // (if any) alive for our use.
-  mozilla::base_profiler_markers_detail::EnsureBufferForMainThreadAddMarker();
-
   UniquePtr<char[]> baseprofile;
   if (baseprofiler::profiler_is_active()) {
     // Note that we still hold the lock, so the sampler cannot run yet and
@@ -5598,8 +5594,6 @@ void profiler_ensure_started(PowerOfTwo32 aCapacity, double aInterval,
   SamplerThread* samplerThread = ActivePS::Destroy(aLock);
   samplerThread->Stop(aLock);
 
-  mozilla::base_profiler_markers_detail::ReleaseBufferForMainThreadAddMarker();
-
   return samplerThread;
 }