From dc70e7f9d2020dc475ae88ed321772038ca01420 Mon Sep 17 00:00:00 2001 From: Butkovits Atila Date: Thu, 16 Dec 2021 18:02:35 +0200 Subject: [PATCH] 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 | 13 ++--- .../baseprofiler/core/ProfileBufferEntry.cpp | 4 +- mozglue/baseprofiler/core/ProfilerMarkers.cpp | 47 +------------------ mozglue/baseprofiler/core/platform.cpp | 4 -- .../public/BaseProfilerMarkersDetail.h | 36 ++++---------- tools/profiler/core/ProfileBuffer.h | 14 ++---- tools/profiler/core/ProfileBufferEntry.cpp | 12 ++--- tools/profiler/core/platform.cpp | 6 --- 8 files changed, 23 insertions(+), 113 deletions(-) diff --git a/mozglue/baseprofiler/core/ProfileBuffer.h b/mozglue/baseprofiler/core/ProfileBuffer.h index f77b429df82a..8102cb7e793a 100644 --- a/mozglue/baseprofiler/core/ProfileBuffer.h +++ b/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 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; diff --git a/mozglue/baseprofiler/core/ProfileBufferEntry.cpp b/mozglue/baseprofiler/core/ProfileBufferEntry.cpp index e85c6a665491..4bb9fc731853 100644 --- a/mozglue/baseprofiler/core/ProfileBufferEntry.cpp +++ b/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, diff --git a/mozglue/baseprofiler/core/ProfilerMarkers.cpp b/mozglue/baseprofiler/core/ProfilerMarkers.cpp index 089199adfabb..840beb259f04 100644 --- a/mozglue/baseprofiler/core/ProfilerMarkers.cpp +++ b/mozglue/baseprofiler/core/ProfilerMarkers.cpp @@ -5,7 +5,7 @@ #include "mozilla/BaseProfilerMarkers.h" -#include "mozilla/BaseProfilerUtils.h" +#include "mozilla/Likely.h" #include @@ -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( - 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, diff --git a/mozglue/baseprofiler/core/platform.cpp b/mozglue/baseprofiler/core/platform.cpp index cc6b3e0d4c20..12d78aa60d05 100644 --- a/mozglue/baseprofiler/core/platform.cpp +++ b/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; } diff --git a/mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h b/mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h index 97a17bd4ae31..3d073aeb7f04 100644 --- a/mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h +++ b/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( - 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( + aBuffer, aName, aCategory, std::move(aOptions), aTs...); } return AddMarkerWithOptionalStackToBuffer( diff --git a/tools/profiler/core/ProfileBuffer.h b/tools/profiler/core/ProfileBuffer.h index 56e114098b90..6f007bba7398 100644 --- a/tools/profiler/core/ProfileBuffer.h +++ b/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 - 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 diff --git a/tools/profiler/core/ProfileBufferEntry.cpp b/tools/profiler/core/ProfileBufferEntry.cpp index 5cbf841b336b..8205e7cd4069 100644 --- a/tools/profiler/core/ProfileBufferEntry.cpp +++ b/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, diff --git a/tools/profiler/core/platform.cpp b/tools/profiler/core/platform.cpp index d1f9fed7b570..8359c8f63867 100644 --- a/tools/profiler/core/platform.cpp +++ b/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 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; }