Bug 1598992 - More precise way to decide when to delete expired Base Profiler data - r=gregtatum
authorGerald Squelart <gsquelart@mozilla.com>
Tue, 26 Nov 2019 23:01:12 +0000
changeset 503948 98015236fc64f3c1b7362120ca4e25f3cfcc6ba7
parent 503947 bf4148a5bc47be85cd64c34d88e3ed33c3f4b32a
child 503949 0e6c7dbb2abb4bdecdd00c1bf2d599bfa67d427e
push id101644
push usergsquelart@mozilla.com
push dateWed, 27 Nov 2019 00:11:13 +0000
treeherderautoland@ad4df3796e18 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgregtatum
bugs1598992
milestone72.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 1598992 - More precise way to decide when to delete expired Base Profiler data - r=gregtatum In practice the previous test that was deleting Base Profiler data when the index became greater that 1 was correct for Firefox, because the Base Profiler *always* starts before the very first Gecko Profiler active instance. However in tests (like the one in the following patch) this may not be true, because each test may start and stop the profiler, and the recent storage update means that the index doesn't go back to 1. So when a test (apart from the first test to use the profiler) attemps to use the Base Profiler, that Base Profiler data will be immediately discarded because the index is already greater than 1 (from previous tests). This change is more future-proof as well, in case we later want to use the Base Profiler more than once in a Firefox instance. Differential Revision: https://phabricator.services.mozilla.com/D54445
tools/profiler/core/platform.cpp
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -1043,41 +1043,65 @@ class ActivePS {
     sInstance->mDeadProfiledPages.clear();
   }
 
   static void ClearExpiredExitProfiles(PSLockRef) {
     MOZ_ASSERT(sInstance);
     uint64_t bufferRangeStart = sInstance->mProfileBuffer.BufferRangeStart();
     // Discard exit profiles that were gathered before our buffer RangeStart.
 #ifdef MOZ_BASE_PROFILER
-    // The buffer range starts at 1 (the first valid entry, 0 is reserved as
-    // null marker). So if it now starts *after* 1, it means we have started to
-    // overwrite our oldest data, and we should get rid of Base profiles if any.
-    if (bufferRangeStart > 1 && sInstance->mBaseProfileThreads) {
+    // If we have started to overwrite our data from when the Base profile was
+    // added, we should get rid of that Base profile because it's now older than
+    // our oldest Gecko profile data.
+    //
+    // When adding: (In practice the starting buffer should be empty)
+    // v Start == End
+    // |                 <-- Buffer range, initially empty.
+    // ^ mGeckoIndexWhenBaseProfileAdded < Start FALSE -> keep it
+    //
+    // Later, still in range:
+    // v Start   v End
+    // |=========|       <-- Buffer range growing.
+    // ^ mGeckoIndexWhenBaseProfileAdded < Start FALSE -> keep it
+    //
+    // Even later, now out of range:
+    //       v Start      v End
+    //       |============|       <-- Buffer range full and sliding.
+    // ^ mGeckoIndexWhenBaseProfileAdded < Start TRUE! -> Discard it
+    if (sInstance->mBaseProfileThreads &&
+        sInstance->mGeckoIndexWhenBaseProfileAdded <
+            CorePS::CoreBlocksRingBuffer().GetState().mRangeStart) {
+      DEBUG_LOG("ClearExpiredExitProfiles() - Discarding base profile %p",
+                sInstance->mBaseProfileThreads.get());
       sInstance->mBaseProfileThreads.reset();
     }
 #endif
     sInstance->mExitProfiles.eraseIf(
         [bufferRangeStart](const ExitProfile& aExitProfile) {
           return aExitProfile.mBufferPositionAtGatherTime < bufferRangeStart;
         });
   }
 
 #ifdef MOZ_BASE_PROFILER
   static void AddBaseProfileThreads(PSLockRef aLock,
                                     UniquePtr<char[]> aBaseProfileThreads) {
     MOZ_ASSERT(sInstance);
+    DEBUG_LOG("AddBaseProfileThreads(%p)", aBaseProfileThreads.get());
     sInstance->mBaseProfileThreads = std::move(aBaseProfileThreads);
+    sInstance->mGeckoIndexWhenBaseProfileAdded =
+        CorePS::CoreBlocksRingBuffer().GetState().mRangeEnd;
   }
 
   static UniquePtr<char[]> MoveBaseProfileThreads(PSLockRef aLock) {
     MOZ_ASSERT(sInstance);
 
     ClearExpiredExitProfiles(aLock);
 
+    DEBUG_LOG("MoveBaseProfileThreads() - Consuming base profile %p",
+              sInstance->mBaseProfileThreads.get());
     return std::move(sInstance->mBaseProfileThreads);
   }
 #endif
 
   static void AddExitProfile(PSLockRef aLock, const nsCString& aExitProfile) {
     MOZ_ASSERT(sInstance);
 
     ClearExpiredExitProfiles(aLock);
@@ -1172,16 +1196,17 @@ class ActivePS {
   // Used to record whether the profiler was paused just before forking. False
   // at all times except just before/after forking.
   bool mWasPaused;
 #endif
 
 #ifdef MOZ_BASE_PROFILER
   // Optional startup profile thread array from BaseProfiler.
   UniquePtr<char[]> mBaseProfileThreads;
+  BlocksRingBuffer::BlockIndex mGeckoIndexWhenBaseProfileAdded;
 #endif
 
   struct ExitProfile {
     nsCString mJSON;
     uint64_t mBufferPositionAtGatherTime;
   };
   Vector<ExitProfile> mExitProfiles;
 };