From 6e54d85e3ba215af32388f9594005a2f1185fcf2 Mon Sep 17 00:00:00 2001 From: Norisz Fay Date: Tue, 14 Feb 2023 14:44:31 +0200 Subject: [PATCH] Backed out 4 changesets (bug 1814908) for causing cppunit failures on TestBaseProfiler CLOSED TREE Backed out changeset 4fd5e3f3d87b (bug 1814908) Backed out changeset 6a716b16e8e3 (bug 1814908) Backed out changeset a8557338bd74 (bug 1814908) Backed out changeset b1a9a1820b56 (bug 1814908) --- .../client/performance-new/shared/utils.js | 6 +- mozglue/baseprofiler/core/platform.cpp | 18 +--- .../baseprofiler/public/BaseProfilerMarkers.h | 5 +- .../public/BaseProfilerMarkersDetail.h | 14 +-- .../baseprofiler/public/BaseProfilerState.h | 26 +---- .../extensions/schemas/geckoProfiler.json | 2 +- tools/profiler/core/ProfilerBindings.cpp | 4 +- tools/profiler/core/platform.cpp | 14 +-- .../gecko/ProfilerIOInterposeObserver.cpp | 8 +- tools/profiler/public/ProfilerMarkers.h | 5 +- tools/profiler/public/ProfilerState.h | 14 +-- tools/profiler/tests/gtest/GeckoProfiler.cpp | 98 ------------------- 12 files changed, 24 insertions(+), 190 deletions(-) diff --git a/devtools/client/performance-new/shared/utils.js b/devtools/client/performance-new/shared/utils.js index 034e5721866b..1f78a257f1e1 100644 --- a/devtools/client/performance-new/shared/utils.js +++ b/devtools/client/performance-new/shared/utils.js @@ -442,9 +442,9 @@ const featureDescriptions = [ "Record File I/O markers from all threads, even unregistered threads.", }, { - name: "No Marker Stacks", - value: "nomarkerstacks", - title: "Do not capture stacks when recording markers, to reduce overhead.", + name: "No File IO Stack Sampling", + value: "noiostacks", + title: "Do not sample stacks when recording File I/O markers.", }, { name: "Sequential Styling", diff --git a/mozglue/baseprofiler/core/platform.cpp b/mozglue/baseprofiler/core/platform.cpp index 4a31d096e280..a56ce0ad46d1 100644 --- a/mozglue/baseprofiler/core/platform.cpp +++ b/mozglue/baseprofiler/core/platform.cpp @@ -1126,12 +1126,6 @@ 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 @@ -3422,15 +3416,6 @@ 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; @@ -3685,8 +3670,7 @@ UniquePtr profiler_capture_backtrace() { PROFILER); // Quick is-active check before allocating a buffer. - // If NoMarkerStacks is set, we don't want to capture a backtrace. - if (!profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)) { + if (!profiler_is_active()) { return nullptr; } diff --git a/mozglue/baseprofiler/public/BaseProfilerMarkers.h b/mozglue/baseprofiler/public/BaseProfilerMarkers.h index d706cefd4ab4..580527aae610 100644 --- a/mozglue/baseprofiler/public/BaseProfilerMarkers.h +++ b/mozglue/baseprofiler/public/BaseProfilerMarkers.h @@ -60,10 +60,7 @@ ProfileBufferBlockIndex AddMarkerToBuffer( AUTO_BASE_PROFILER_LABEL("baseprofiler::AddMarkerToBuffer", PROFILER); return base_profiler_markers_detail::AddMarkerToBuffer( aBuffer, aName, aCategory, std::move(aOptions), - // Do not capture a stack if the NoMarkerStacks feature is set. - profiler_active_without_feature(ProfilerFeature::NoMarkerStacks) - ? ::mozilla::baseprofiler::profiler_capture_backtrace_into - : nullptr, + ::mozilla::baseprofiler::profiler_capture_backtrace_into, aPayloadArguments...); } diff --git a/mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h b/mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h index 03c2afc1a0ba..174dd36bca35 100644 --- a/mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h +++ b/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 OptionalBacktraceCaptureFunction = bool (*)(ProfileChunkedBuffer&, - StackCaptureOptions); +using BacktraceCaptureFunction = bool (*)(ProfileChunkedBuffer&, + StackCaptureOptions); // Use a pre-allocated and cleared chunked buffer in the main thread's // `AddMarkerToBuffer()`. @@ -276,8 +276,7 @@ template ProfileBufferBlockIndex AddMarkerToBuffer( ProfileChunkedBuffer& aBuffer, const ProfilerString8View& aName, const MarkerCategory& aCategory, MarkerOptions&& aOptions, - OptionalBacktraceCaptureFunction aOptionalBacktraceCaptureFunction, - const Ts&... aTs) { + BacktraceCaptureFunction aBacktraceCaptureFunction, const Ts&... aTs) { if (aOptions.ThreadId().IsUnspecified()) { // If yet unspecified, set thread to this thread where the marker is added. aOptions.Set(MarkerThreadId::CurrentThread()); @@ -289,17 +288,14 @@ ProfileBufferBlockIndex AddMarkerToBuffer( } StackCaptureOptions captureOptions = aOptions.Stack().CaptureOptions(); - if (captureOptions != StackCaptureOptions::NoStack && - // Backtrace capture function will be nullptr if the profiler - // NoMarkerStacks feature is set. - aOptionalBacktraceCaptureFunction != nullptr) { + if (captureOptions != StackCaptureOptions::NoStack) { // 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( - aOptionalBacktraceCaptureFunction(aChunkedBuffer, captureOptions) + aBacktraceCaptureFunction(aChunkedBuffer, captureOptions) ? &aChunkedBuffer : nullptr); // This call must be made from here, while chunkedBuffer is in scope. diff --git a/mozglue/baseprofiler/public/BaseProfilerState.h b/mozglue/baseprofiler/public/BaseProfilerState.h index 15be2743a47e..a8f10672f515 100644 --- a/mozglue/baseprofiler/public/BaseProfilerState.h +++ b/mozglue/baseprofiler/public/BaseProfilerState.h @@ -181,8 +181,8 @@ class MOZ_RAII AutoProfilerStats { MACRO(4, "fileioall", FileIOAll, \ "Add file I/O from all threads, implies fileio") \ \ - MACRO(5, "nomarkerstacks", NoMarkerStacks, \ - "Markers do not capture stacks, to reduce overhead") \ + MACRO(5, "noiostacks", NoIOStacks, \ + "File I/O markers do not capture stacks, to reduce overhead") \ \ MACRO(6, "screenshots", Screenshots, \ "Take a snapshot of the window on every composition") \ @@ -277,20 +277,10 @@ class RacyFeatures { MFBT_API static void SetSamplingUnpaused(); - [[nodiscard]] MFBT_API static mozilla::Maybe 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(); @@ -374,24 +364,12 @@ 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 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 diff --git a/toolkit/components/extensions/schemas/geckoProfiler.json b/toolkit/components/extensions/schemas/geckoProfiler.json index bc1cec6e393d..50bb8dc8c6d0 100644 --- a/toolkit/components/extensions/schemas/geckoProfiler.json +++ b/toolkit/components/extensions/schemas/geckoProfiler.json @@ -28,7 +28,7 @@ "mainthreadio", "fileio", "fileioall", - "nomarkerstacks", + "noiostacks", "screenshots", "seqstyle", "stackwalk", diff --git a/tools/profiler/core/ProfilerBindings.cpp b/tools/profiler/core/ProfilerBindings.cpp index c3af5c5b56da..280580e80baa 100644 --- a/tools/profiler/core/ProfilerBindings.cpp +++ b/tools/profiler/core/ProfilerBindings.cpp @@ -346,9 +346,7 @@ void gecko_profiler_add_marker( mozilla::StackCaptureOptions captureOptions = markerOptions.Stack().CaptureOptions(); - if (captureOptions != mozilla::StackCaptureOptions::NoStack && - // Do not capture a stack if the NoMarkerStacks feature is set. - profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)) { + if (captureOptions != mozilla::StackCaptureOptions::NoStack) { // 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. diff --git a/tools/profiler/core/platform.cpp b/tools/profiler/core/platform.cpp index 0196dbb4e730..7b5d497af34e 100644 --- a/tools/profiler/core/platform.cpp +++ b/tools/profiler/core/platform.cpp @@ -6286,15 +6286,6 @@ 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; @@ -6795,9 +6786,8 @@ UniquePtr profiler_capture_backtrace() { MOZ_RELEASE_ASSERT(CorePS::Exists()); AUTO_PROFILER_LABEL("profiler_capture_backtrace", PROFILER); - // Quick is-active and feature check before allocating a buffer. - // If NoMarkerStacks is set, we don't want to capture a backtrace. - if (!profiler_active_without_feature(ProfilerFeature::NoMarkerStacks)) { + // Quick is-active check before allocating a buffer. + if (!profiler_is_active()) { return nullptr; } diff --git a/tools/profiler/gecko/ProfilerIOInterposeObserver.cpp b/tools/profiler/gecko/ProfilerIOInterposeObserver.cpp index cf33789f69d8..d675c87b5dd5 100644 --- a/tools/profiler/gecko/ProfilerIOInterposeObserver.cpp +++ b/tools/profiler/gecko/ProfilerIOInterposeObserver.cpp @@ -91,6 +91,7 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) { } AUTO_PROFILER_LABEL("ProfilerIOInterposeObserver", PROFILER); + const bool doCaptureStack = !(features & ProfilerFeature::NoIOStacks); if (IsMainThread()) { // This is the main thread. // Capture a marker if any "IO" feature is on. @@ -108,7 +109,7 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) { type, OTHER, MarkerOptions( MarkerTiming::Interval(aObservation.Start(), aObservation.End()), - MarkerStack::Capture()), + MarkerStack::MaybeCapture(doCaptureStack)), FileIOMarker, // aOperation ProfilerString8View::WrapNullTerminatedString( @@ -134,7 +135,8 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) { // Share a backtrace between the marker on this thread, and the marker on // the main thread. - UniquePtr backtrace = profiler_capture_backtrace(); + UniquePtr backtrace = + doCaptureStack ? profiler_capture_backtrace() : nullptr; // Store the marker in the current thread. PROFILER_MARKER( @@ -199,7 +201,7 @@ void ProfilerIOInterposeObserver::Observe(Observation& aObservation) { type, OTHER, MarkerOptions( MarkerTiming::Interval(aObservation.Start(), aObservation.End()), - MarkerStack::Capture(), + doCaptureStack ? MarkerStack::Capture() : MarkerStack::NoStack(), // Store this marker on the main thread. MarkerThreadId::MainThread()), FileIOMarker, diff --git a/tools/profiler/public/ProfilerMarkers.h b/tools/profiler/public/ProfilerMarkers.h index ca53c3f18922..a4a5ad024e03 100644 --- a/tools/profiler/public/ProfilerMarkers.h +++ b/tools/profiler/public/ProfilerMarkers.h @@ -106,10 +106,7 @@ mozilla::ProfileBufferBlockIndex AddMarkerToBuffer( mozilla::Unused << aMarkerType; // Only the empty object type is useful. return mozilla::base_profiler_markers_detail::AddMarkerToBuffer( aBuffer, aName, aCategory, std::move(aOptions), - profiler_active_without_feature(ProfilerFeature::NoMarkerStacks) - ? ::profiler_capture_backtrace_into - : nullptr, - aPayloadArguments...); + ::profiler_capture_backtrace_into, aPayloadArguments...); } // Add a marker (without payload) to a given buffer. diff --git a/tools/profiler/public/ProfilerState.h b/tools/profiler/public/ProfilerState.h index 7a9f3f5c7387..3806ad1ca4a4 100644 --- a/tools/profiler/public/ProfilerState.h +++ b/tools/profiler/public/ProfilerState.h @@ -61,8 +61,8 @@ MACRO(4, "fileioall", FileIOAll, \ "Add file I/O from all threads, implies fileio") \ \ - MACRO(5, "nomarkerstacks", NoMarkerStacks, \ - "Markers do not capture stacks, to reduce overhead") \ + MACRO(5, "noiostacks", NoIOStacks, \ + "File I/O markers do not capture stacks, to reduce overhead") \ \ MACRO(6, "screenshots", Screenshots, \ "Take a snapshot of the window on every composition") \ @@ -266,11 +266,6 @@ 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 @@ -372,11 +367,6 @@ 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 diff --git a/tools/profiler/tests/gtest/GeckoProfiler.cpp b/tools/profiler/tests/gtest/GeckoProfiler.cpp index 68c75c575256..20623ea50ea5 100644 --- a/tools/profiler/tests/gtest/GeckoProfiler.cpp +++ b/tools/profiler/tests/gtest/GeckoProfiler.cpp @@ -4978,102 +4978,4 @@ 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 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 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