Bug 1814908 - Do not collect the marker stacks if NoMarkerStacks feature is set r=julienw,mstange

Differential Revision: https://phabricator.services.mozilla.com/D169112

mozglue/baseprofiler/core/platform.cpp

@@ -1126,6 +1126,12 @@ bool RacyFeatures::IsActiveWithFeature(uint32_t aFeature) {
   return (af & Active) && (af & aFeature);
 }
 
+/* static */
+bool RacyFeatures::IsActiveWithoutFeature(uint32_t aFeature) {
+  uint32_t af = sActiveAndFeatures;  // copy it first
+  return (af & Active) && !(af & aFeature);
+}
+
 /* static */
 bool RacyFeatures::IsActiveAndUnpaused() {
   uint32_t af = sActiveAndFeatures;  // copy it first
@@ -3416,6 +3422,15 @@ bool profiler_feature_active(uint32_t aFeature) {
   return RacyFeatures::IsActiveWithFeature(aFeature);
 }
 
+bool profiler_active_without_feature(uint32_t aFeature) {
+  // This function runs both on and off the main thread.
+
+  MOZ_RELEASE_ASSERT(CorePS::Exists());
+
+  // This function is hot enough that we use RacyFeatures, not ActivePS.
+  return RacyFeatures::IsActiveWithoutFeature(aFeature);
+}
+
 void profiler_add_sampled_counter(BaseProfilerCount* aCounter) {
   DEBUG_LOG("profiler_add_sampled_counter(%s)", aCounter->mLabel);
   PSAutoLock lock;
@@ -3670,7 +3685,8 @@ UniquePtr<ProfileChunkedBuffer> profiler_capture_backtrace() {
                            PROFILER);
 
   // Quick is-active check before allocating a buffer.
-  if (!profiler_is_active()) {
+  // If NoMarkerStacks is set, we don't want to capture a backtrace.
+  if (!profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)) {
     return nullptr;
   }
 

mozglue/baseprofiler/public/BaseProfilerMarkers.h

@@ -60,7 +60,10 @@ ProfileBufferBlockIndex AddMarkerToBuffer(
   AUTO_BASE_PROFILER_LABEL("baseprofiler::AddMarkerToBuffer", PROFILER);
   return base_profiler_markers_detail::AddMarkerToBuffer<MarkerType>(
       aBuffer, aName, aCategory, std::move(aOptions),
-      ::mozilla::baseprofiler::profiler_capture_backtrace_into,
+      // Do not capture a stack if the NoMarkerStacks feature is set.
+      profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)
+          ? ::mozilla::baseprofiler::profiler_capture_backtrace_into
+          : nullptr,
       aPayloadArguments...);
 }
 

mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h

@@ -258,8 +258,8 @@ static ProfileBufferBlockIndex AddMarkerWithOptionalStackToBuffer(
 
 // Pointer to a function that can capture a backtrace into the provided
 // `ProfileChunkedBuffer`, and returns true when successful.
-using BacktraceCaptureFunction = bool (*)(ProfileChunkedBuffer&,
+using OptionalBacktraceCaptureFunction = bool (*)(ProfileChunkedBuffer&,
-                                          StackCaptureOptions);
+                                                  StackCaptureOptions);
 
 // Use a pre-allocated and cleared chunked buffer in the main thread's
 // `AddMarkerToBuffer()`.
@@ -276,7 +276,8 @@ template <typename MarkerType, typename... Ts>
 ProfileBufferBlockIndex AddMarkerToBuffer(
     ProfileChunkedBuffer& aBuffer, const ProfilerString8View& aName,
     const MarkerCategory& aCategory, MarkerOptions&& aOptions,
-    BacktraceCaptureFunction aBacktraceCaptureFunction, const Ts&... aTs) {
+    OptionalBacktraceCaptureFunction aOptionalBacktraceCaptureFunction,
+    const Ts&... aTs) {
   if (aOptions.ThreadId().IsUnspecified()) {
     // If yet unspecified, set thread to this thread where the marker is added.
     aOptions.Set(MarkerThreadId::CurrentThread());
@@ -288,14 +289,17 @@ ProfileBufferBlockIndex AddMarkerToBuffer(
   }
 
   StackCaptureOptions captureOptions = aOptions.Stack().CaptureOptions();
-  if (captureOptions != StackCaptureOptions::NoStack) {
+  if (captureOptions != StackCaptureOptions::NoStack &&
+      // Backtrace capture function will be nullptr if the profiler
+      // NoMarkerStacks feature is set.
+      aOptionalBacktraceCaptureFunction != nullptr) {
     // 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)
+          aOptionalBacktraceCaptureFunction(aChunkedBuffer, captureOptions)
               ? &aChunkedBuffer
               : nullptr);
       // This call must be made from here, while chunkedBuffer is in scope.

mozglue/baseprofiler/public/BaseProfilerState.h

@@ -277,10 +277,20 @@ class RacyFeatures {
 
   MFBT_API static void SetSamplingUnpaused();
 
+  [[nodiscard]] MFBT_API static mozilla::Maybe<uint32_t> FeaturesIfActive() {
+    if (uint32_t af = sActiveAndFeatures; af & Active) {
+      // Active, remove the Active&Paused bits to get all features.
+      return Some(af & ~(Active | Paused | SamplingPaused));
+    }
+    return Nothing();
+  }
+
   [[nodiscard]] MFBT_API static bool IsActive();
 
   [[nodiscard]] MFBT_API static bool IsActiveWithFeature(uint32_t aFeature);
 
+  [[nodiscard]] MFBT_API static bool IsActiveWithoutFeature(uint32_t aFeature);
+
   // True if profiler is active, and not fully paused.
   // Note that periodic sampling *could* be paused!
   [[nodiscard]] MFBT_API static bool IsActiveAndUnpaused();
@@ -364,12 +374,24 @@ MFBT_API bool IsThreadBeingProfiled();
 // not.
 [[nodiscard]] MFBT_API uint32_t profiler_get_available_features();
 
+// Returns the full feature set if the profiler is active.
+// Note: the return value can become immediately out-of-date, much like the
+// return value of profiler_is_active().
+[[nodiscard]] inline mozilla::Maybe<uint32_t> profiler_features_if_active() {
+  return baseprofiler::detail::RacyFeatures::FeaturesIfActive();
+}
+
 // Check if a profiler feature (specified via the ProfilerFeature type) is
 // active. Returns false if the profiler is inactive. Note: the return value
 // can become immediately out-of-date, much like the return value of
 // profiler_is_active().
 [[nodiscard]] MFBT_API bool profiler_feature_active(uint32_t aFeature);
 
+// Check if the profiler is active without a feature (specified via the
+// ProfilerFeature type). Note: the return value can become immediately
+// out-of-date, much like the return value of profiler_is_active().
+[[nodiscard]] MFBT_API bool profiler_active_without_feature(uint32_t aFeature);
+
 // Returns true if any of the profiler mutexes are currently locked *on the
 // current thread*. This may be used by re-entrant code that may call profiler
 // functions while the same of a different profiler mutex is locked, which could

mozglue/tests/TestBaseProfiler.cpp

@@ -4821,9 +4821,6 @@ void TestProfiler() {
   TestProfilerDependencies();
 
   {
-    printf("profiler_init()...\n");
-    AUTO_BASE_PROFILER_INIT;
-
     MOZ_RELEASE_ASSERT(!baseprofiler::profiler_is_active());
     MOZ_RELEASE_ASSERT(!baseprofiler::profiler_thread_is_being_profiled());
     MOZ_RELEASE_ASSERT(!baseprofiler::profiler_thread_is_sleeping());
@@ -5723,8 +5720,13 @@ int main()
   TestProgressLogger();
   // Note that there are two `TestProfiler{,Markers}` functions above, depending
   // on whether MOZ_GECKO_PROFILER is #defined.
-  TestProfiler();
+  {
-  TestProfilerMarkers();
+    printf("profiler_init()...\n");
+    AUTO_BASE_PROFILER_INIT;
+
+    TestProfiler();
+    TestProfilerMarkers();
+  }
 
   return 0;
 }

tools/profiler/core/ProfilerBindings.cpp

@@ -346,7 +346,9 @@ void gecko_profiler_add_marker(
 
   mozilla::StackCaptureOptions captureOptions =
       markerOptions.Stack().CaptureOptions();
-  if (captureOptions != mozilla::StackCaptureOptions::NoStack) {
+  if (captureOptions != mozilla::StackCaptureOptions::NoStack &&
+      // Do not capture a stack if the NoMarkerStacks feature is set.
+      profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)) {
     // 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.

tools/profiler/core/platform.cpp

@@ -6286,6 +6286,15 @@ bool profiler_feature_active(uint32_t aFeature) {
   return RacyFeatures::IsActiveWithFeature(aFeature);
 }
 
+bool profiler_active_without_feature(uint32_t aFeature) {
+  // This function runs both on and off the main thread.
+
+  MOZ_RELEASE_ASSERT(CorePS::Exists());
+
+  // This function is hot enough that we use RacyFeatures, not ActivePS.
+  return RacyFeatures::IsActiveWithoutFeature(aFeature);
+}
+
 void profiler_write_active_configuration(JSONWriter& aWriter) {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
   PSAutoLock lock;
@@ -6786,8 +6795,9 @@ UniquePtr<ProfileChunkedBuffer> profiler_capture_backtrace() {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
   AUTO_PROFILER_LABEL("profiler_capture_backtrace", PROFILER);
 
-  // Quick is-active check before allocating a buffer.
+  // Quick is-active and feature check before allocating a buffer.
-  if (!profiler_is_active()) {
+  // If NoMarkerStacks is set, we don't want to capture a backtrace.
+  if (!profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)) {
     return nullptr;
   }
 

tools/profiler/public/ProfilerMarkers.h

@@ -106,7 +106,10 @@ mozilla::ProfileBufferBlockIndex AddMarkerToBuffer(
   mozilla::Unused << aMarkerType;  // Only the empty object type is useful.
   return mozilla::base_profiler_markers_detail::AddMarkerToBuffer<MarkerType>(
       aBuffer, aName, aCategory, std::move(aOptions),
-      ::profiler_capture_backtrace_into, aPayloadArguments...);
+      profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)
+          ? ::profiler_capture_backtrace_into
+          : nullptr,
+      aPayloadArguments...);
 }
 
 // Add a marker (without payload) to a given buffer.

tools/profiler/public/ProfilerState.h

@@ -266,6 +266,11 @@ class RacyFeatures {
     return (af & Active) && (af & aFeature);
   }
 
+  [[nodiscard]] static bool IsActiveWithoutFeature(uint32_t aFeature) {
+    uint32_t af = sActiveAndFeatures;  // copy it first
+    return (af & Active) && !(af & aFeature);
+  }
+
   // True if profiler is active, and not fully paused.
   // Note that periodic sampling *could* be paused!
   // This implementation must be kept in sync with
@@ -367,6 +372,11 @@ profiler_features_if_active_and_unpaused() {
 // profiler_is_active().
 [[nodiscard]] bool profiler_feature_active(uint32_t aFeature);
 
+// Check if the profiler is active without a feature (specified via the
+// ProfilerFeature type). Note: the return value can become immediately
+// out-of-date, much like the return value of profiler_is_active().
+[[nodiscard]] bool profiler_active_without_feature(uint32_t aFeature);
+
 // Returns true if any of the profiler mutexes are currently locked *on the
 // current thread*. This may be used by re-entrant code that may call profiler
 // functions while the same of a different profiler mutex is locked, which could

tools/profiler/tests/gtest/GeckoProfiler.cpp

@@ -4978,4 +4978,102 @@ TEST(GeckoProfiler, FailureHandling)
   ASSERT_EQ(profile.get(), nullptr);
 }
 
+TEST(GeckoProfiler, NoMarkerStacks)
+{
+  uint32_t features = ProfilerFeature::NoMarkerStacks;
+  const char* filters[] = {"GeckoMain"};
+
+  ASSERT_TRUE(!profiler_get_profile());
+
+  // Make sure that profiler_capture_backtrace returns nullptr when the profiler
+  // is not active.
+  ASSERT_TRUE(!profiler_capture_backtrace());
+
+  {
+    // Start the profiler without the NoMarkerStacks feature and make sure we
+    // capture stacks.
+    profiler_start(PROFILER_DEFAULT_ENTRIES, PROFILER_DEFAULT_INTERVAL,
+                   /* features */ 0, filters, MOZ_ARRAY_LENGTH(filters), 0);
+
+    ASSERT_TRUE(profiler_capture_backtrace());
+    profiler_stop();
+  }
+
+  // Start the profiler without the NoMarkerStacks feature and make sure we
+  // don't capture stacks.
+  profiler_start(PROFILER_DEFAULT_ENTRIES, PROFILER_DEFAULT_INTERVAL, features,
+                 filters, MOZ_ARRAY_LENGTH(filters), 0);
+
+  // Make sure that the active features has the NoMarkerStacks feature.
+  mozilla::Maybe<uint32_t> activeFeatures = profiler_features_if_active();
+  ASSERT_TRUE(activeFeatures.isSome());
+  ASSERT_TRUE(ProfilerFeature::HasNoMarkerStacks(*activeFeatures));
+
+  // Make sure we don't capture stacks.
+  ASSERT_TRUE(!profiler_capture_backtrace());
+
+  // Add a marker with a stack to test.
+  EXPECT_TRUE(profiler_add_marker(
+      "Text with stack", geckoprofiler::category::OTHER, MarkerStack::Capture(),
+      geckoprofiler::markers::TextMarker{}, ""));
+
+  UniquePtr<char[]> profile = profiler_get_profile();
+  JSONOutputCheck(profile.get(), [&](const Json::Value& aRoot) {
+    // Check that the meta.configuration.features array contains
+    // "nomarkerstacks".
+    GET_JSON(meta, aRoot["meta"], Object);
+    {
+      GET_JSON(configuration, meta["configuration"], Object);
+      {
+        GET_JSON(features, configuration["features"], Array);
+        {
+          EXPECT_EQ(features.size(), 1u);
+          EXPECT_JSON_ARRAY_CONTAINS(features, String, "nomarkerstacks");
+        }
+      }
+    }
+
+    // Make sure that the marker we captured doesn't have a stack.
+    GET_JSON(threads, aRoot["threads"], Array);
+    {
+      ASSERT_EQ(threads.size(), 1u);
+      GET_JSON(thread0, threads[0], Object);
+      {
+        GET_JSON(markers, thread0["markers"], Object);
+        {
+          GET_JSON(data, markers["data"], Array);
+          {
+            const unsigned int NAME = 0u;
+            const unsigned int PAYLOAD = 5u;
+            bool foundMarker = false;
+            GET_JSON(stringTable, thread0["stringTable"], Array);
+
+            for (const Json::Value& marker : data) {
+              // Even though we only added one marker, some markers like
+              // NotifyObservers are being added as well. Let's iterate over
+              // them and make sure that we have the one we added explicitly and
+              // check its stack doesn't exist.
+              GET_JSON(name, stringTable[marker[NAME].asUInt()], String);
+              std::string nameString = name.asString();
+
+              if (nameString == "Text with stack") {
+                // Make sure that the marker doesn't have a stack.
+                foundMarker = true;
+                EXPECT_FALSE(marker[PAYLOAD].isNull());
+                EXPECT_TRUE(marker[PAYLOAD]["stack"].isNull());
+              }
+            }
+
+            EXPECT_TRUE(foundMarker);
+          }
+        }
+      }
+    }
+  });
+
+  profiler_stop();
+
+  ASSERT_TRUE(!profiler_get_profile());
+}
+
 #endif  // MOZ_GECKO_PROFILER