Bug 1373436 (part 4) - Use UniquePtr with profile_add_marker(). r=mstange.
authorNicholas Nethercote <nnethercote@mozilla.com>
Fri, 16 Jun 2017 12:26:26 +1000
changeset 364581 07071624cc92f889943c93ebdfb5091d043a1ec7
parent 364580 1658ad154d6686c64348d5bf64802add53f826ca
child 364582 f55086b153eb88ec9620c26f278384dad26343c0
push id91576
push usernnethercote@mozilla.com
push dateMon, 19 Jun 2017 03:28:18 +0000
treeherdermozilla-inbound@07071624cc92 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1373436
milestone56.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 1373436 (part 4) - Use UniquePtr with profile_add_marker(). r=mstange. Once the |aPayload| argument to profile_add_marker() became a UniquePtr the default value of nullptr caused compilation difficulties that could only be fixed by #including ProfilerMarkerPayload.h into lots of additional places (because the UniquePtr<T> instantiation required the T to be fully defined). To get around this I just split profile_add_marker() into two functions, one with 1 argument and one with 2 arguments. The patch also removes the definition of PROFILER_MARKER_PAYLOAD in the case where MOZ_GECKO_PROFILER isn't defined. A comment explains why.
dom/events/EventListenerManager.cpp
dom/performance/Performance.cpp
gfx/layers/composite/ContainerLayerComposite.cpp
gfx/layers/ipc/CompositorBridgeParent.cpp
gfx/thebes/ContextStateTracker.cpp
tools/profiler/core/platform.cpp
tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
tools/profiler/public/GeckoProfiler.h
tools/profiler/tests/gtest/GeckoProfiler.cpp
widget/VsyncDispatcher.cpp
xpcom/base/CycleCollectedJSContext.cpp
xpcom/base/CycleCollectedJSRuntime.cpp
--- a/dom/events/EventListenerManager.cpp
+++ b/dom/events/EventListenerManager.cpp
@@ -1294,20 +1294,20 @@ EventListenerManager::HandleEventInterna
                                      typeCStr.get());
               TimeStamp startTime = TimeStamp::Now();
 
               rv = HandleEventSubType(listener, *aDOMEvent, aCurrentTarget);
 
               TimeStamp endTime = TimeStamp::Now();
               uint16_t phase;
               (*aDOMEvent)->GetEventPhase(&phase);
-              PROFILER_MARKER_PAYLOAD("DOMEvent",
-                                      new DOMEventMarkerPayload(typeStr, phase,
-                                                                startTime,
-                                                                endTime));
+              PROFILER_MARKER_PAYLOAD(
+                "DOMEvent",
+                MakeUnique<DOMEventMarkerPayload>(typeStr, phase,
+                                                  startTime, endTime));
 #else
               MOZ_CRASH("Gecko Profiler is N/A but profiler_is_active() returned true");
 #endif
             } else {
               rv = HandleEventSubType(listener, *aDOMEvent, aCurrentTarget);
             }
 
             if (NS_FAILED(rv)) {
--- a/dom/performance/Performance.cpp
+++ b/dom/performance/Performance.cpp
@@ -294,19 +294,19 @@ Performance::Mark(const nsAString& aName
   }
 
   RefPtr<PerformanceMark> performanceMark =
     new PerformanceMark(GetAsISupports(), aName, Now());
   InsertUserEntry(performanceMark);
 
 #ifdef MOZ_GECKO_PROFILER
   if (profiler_is_active()) {
-    PROFILER_MARKER_PAYLOAD("UserTiming",
-                            new UserTimingMarkerPayload(aName,
-                                                        TimeStamp::Now()));
+    PROFILER_MARKER_PAYLOAD(
+      "UserTiming",
+      MakeUnique<UserTimingMarkerPayload>(aName, TimeStamp::Now()));
   }
 #endif
 }
 
 void
 Performance::ClearMarks(const Optional<nsAString>& aName)
 {
   ClearUserEntries(aName, NS_LITERAL_STRING("mark"));
@@ -392,19 +392,19 @@ Performance::Measure(const nsAString& aN
   InsertUserEntry(performanceMeasure);
 
 #ifdef MOZ_GECKO_PROFILER
   if (profiler_is_active()) {
     TimeStamp startTimeStamp = CreationTimeStamp() +
                                TimeDuration::FromMilliseconds(startTime);
     TimeStamp endTimeStamp = CreationTimeStamp() +
                              TimeDuration::FromMilliseconds(endTime);
-    PROFILER_MARKER_PAYLOAD("UserTiming",
-                            new UserTimingMarkerPayload(aName, startTimeStamp,
-                                                        endTimeStamp));
+    PROFILER_MARKER_PAYLOAD(
+      "UserTiming",
+      MakeUnique<UserTimingMarkerPayload>(aName, startTimeStamp, endTimeStamp));
   }
 #endif
 }
 
 void
 Performance::ClearMeasures(const Optional<nsAString>& aName)
 {
   ClearUserEntries(aName, NS_LITERAL_STRING("measure"));
--- a/gfx/layers/composite/ContainerLayerComposite.cpp
+++ b/gfx/layers/composite/ContainerLayerComposite.cpp
@@ -95,18 +95,19 @@ PrintUniformityInfo(Layer* aLayer)
   }
 
   Matrix4x4 transform = aLayer->AsHostLayer()->GetShadowBaseTransform();
   if (!transform.Is2D()) {
     return;
   }
 
   Point translation = transform.As2D().GetTranslation();
-  LayerTranslationPayload* payload = new LayerTranslationPayload(aLayer, translation);
-  PROFILER_MARKER_PAYLOAD("LayerTranslation", payload);
+  PROFILER_MARKER_PAYLOAD(
+    "LayerTranslation",
+    MakeUnique<LayerTranslationPayload>(aLayer, translation));
 #endif
 }
 
 static Maybe<gfx::Polygon>
 SelectLayerGeometry(const Maybe<gfx::Polygon>& aParentGeometry,
                     const Maybe<gfx::Polygon>& aChildGeometry)
 {
   // Both the parent and the child layer were split.
--- a/gfx/layers/ipc/CompositorBridgeParent.cpp
+++ b/gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -1758,18 +1758,19 @@ CompositorBridgeParent::GetAPZCTreeManag
   return apzctm.forget();
 }
 
 static void
 InsertVsyncProfilerMarker(TimeStamp aVsyncTimestamp)
 {
 #ifdef MOZ_GECKO_PROFILER
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
-  VsyncPayload* payload = new VsyncPayload(aVsyncTimestamp);
-  PROFILER_MARKER_PAYLOAD("VsyncTimestamp", payload);
+  PROFILER_MARKER_PAYLOAD(
+    "VsyncTimestamp",
+    MakeUnique<VsyncPayload>(aVsyncTimestamp));
 #endif
 }
 
 /*static */ void
 CompositorBridgeParent::PostInsertVsyncProfilerMarker(TimeStamp aVsyncTimestamp)
 {
   // Called in the vsync thread
   if (profiler_is_active() && CompositorThreadHolder::IsActive()) {
--- a/gfx/thebes/ContextStateTracker.cpp
+++ b/gfx/thebes/ContextStateTracker.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ContextStateTracker.h"
 #include "GLContext.h"
+#include "GeckoProfiler.h"
 #ifdef MOZ_GECKO_PROFILER
 #include "ProfilerMarkerPayload.h"
 #endif
 
 namespace mozilla {
 
 void
 ContextStateTrackerOGL::PushOGLSection(GLContext* aGL, const char* aSectionName)
@@ -106,22 +107,21 @@ ContextStateTrackerOGL::Flush(GLContext*
     }
 
     GLuint gpuTime = 0;
     aGL->fGetQueryObjectuiv(handle, LOCAL_GL_QUERY_RESULT, &gpuTime);
 
     aGL->fDeleteQueries(1, &handle);
 
 #ifdef MOZ_GECKO_PROFILER
-    PROFILER_MARKER_PAYLOAD("gpu_timer_query", new GPUMarkerPayload(
-      mCompletedSections[0].mCpuTimeStart,
-      mCompletedSections[0].mCpuTimeEnd,
-      0,
-      gpuTime
-    ));
+    PROFILER_MARKER_PAYLOAD(
+      "gpu_timer_query",
+      MakeUnique<GPUMarkerPayload>(mCompletedSections[0].mCpuTimeStart,
+                                   mCompletedSections[0].mCpuTimeEnd,
+                                   0, gpuTime));
 #endif
 
     mCompletedSections.RemoveElementAt(0);
   }
 }
 
 void
 ContextStateTrackerOGL::DestroyOGL(GLContext* aGL)
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -2940,29 +2940,33 @@ racy_profiler_add_marker(const char* aMa
                    ? aPayload->GetStartTime()
                    : TimeStamp::Now();
   TimeDuration delta = origin - CorePS::ProcessStartTime();
   racyInfo->AddPendingMarker(aMarkerName, Move(aPayload),
                              delta.ToMilliseconds());
 }
 
 void
-profiler_add_marker(const char* aMarkerName, ProfilerMarkerPayload* aPayload)
+profiler_add_marker(const char* aMarkerName,
+                    UniquePtr<ProfilerMarkerPayload> aPayload)
 {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  // aPayload must be freed if we return early.
-  UniquePtr<ProfilerMarkerPayload> payload(aPayload);
-
-  // This function is hot enough that we use RacyFeatures, notActivePS.
+  // This function is hot enough that we use RacyFeatures, not ActivePS.
   if (!RacyFeatures::IsActiveWithoutPrivacy()) {
     return;
   }
 
-  racy_profiler_add_marker(aMarkerName, Move(payload));
+  racy_profiler_add_marker(aMarkerName, Move(aPayload));
+}
+
+void
+profiler_add_marker(const char* aMarkerName)
+{
+  profiler_add_marker(aMarkerName, nullptr);
 }
 
 void
 profiler_tracing(const char* aCategory, const char* aMarkerName,
                  TracingKind aKind)
 {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
--- a/tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
+++ b/tools/profiler/gecko/ProfilerIOInterposeObserver.cpp
@@ -16,15 +16,14 @@ void ProfilerIOInterposeObserver::Observ
 
   UniqueProfilerBacktrace stack = profiler_get_backtrace();
 
   nsCString filename;
   if (aObservation.Filename()) {
     filename = NS_ConvertUTF16toUTF8(aObservation.Filename());
   }
 
-  IOMarkerPayload* markerPayload = new IOMarkerPayload(aObservation.Reference(),
-                                                       filename.get(),
-                                                       aObservation.Start(),
-                                                       aObservation.End(),
-                                                       Move(stack));
-  PROFILER_MARKER_PAYLOAD(aObservation.ObservedOperationString(), markerPayload);
+  PROFILER_MARKER_PAYLOAD(
+    aObservation.ObservedOperationString(),
+    MakeUnique<IOMarkerPayload>(aObservation.Reference(), filename.get(),
+                                aObservation.Start(), aObservation.End(),
+                                Move(stack)));
 }
--- a/tools/profiler/public/GeckoProfiler.h
+++ b/tools/profiler/public/GeckoProfiler.h
@@ -98,20 +98,25 @@ using UniqueProfilerBacktrace =
 #define PROFILER_LABEL_DYNAMIC(name_space, info, category, dynamicStr) \
   do {} while (0)
 
 // Insert a marker in the profile timeline. This is useful to delimit something
 // important happening such as the first paint. Unlike profiler_label that are
 // only recorded if a sample is collected while it is active, marker will always
 // be collected.
 #define PROFILER_MARKER(marker_name) do {} while (0)
-#define PROFILER_MARKER_PAYLOAD(marker_name, payload) \
-  do { \
-    mozilla::UniquePtr<ProfilerMarkerPayload> payloadDeletor(payload); \
-  } while (0)
+
+// Like PROFILER_MARKER, but with an additional payload.
+//
+// Note: this is deliberately not defined when MOZ_GECKO_PROFILER is undefined.
+// This macro should not be used in that case -- i.e. all uses of this macro
+// should be guarded by a MOZ_GECKO_PROFILER check -- because payload creation
+// requires allocation, which is something we should not do in builds that
+// don't contain the profiler.
+//#define PROFILER_MARKER_PAYLOAD(marker_name, payload) /* undefined */
 
 #else   // defined(MOZ_GECKO_PROFILER)
 
 #if defined(__GNUC__) || defined(_MSC_VER)
 # define PROFILER_FUNCTION_NAME __FUNCTION__
 #else
   // From C99, supported by some C++ compilers. Just the raw function name.
 # define PROFILER_FUNCTION_NAME __func__
@@ -414,18 +419,19 @@ class nsISupports;
 class ProfilerMarkerPayload;
 
 // This exists purely for AutoProfilerLabel. See the comment on the
 // definition in platform.cpp for details.
 extern MOZ_THREAD_LOCAL(PseudoStack*) sPseudoStack;
 
 // Adds a marker to the PseudoStack. A no-op if the profiler is inactive or in
 // privacy mode.
+void profiler_add_marker(const char* aMarkerName);
 void profiler_add_marker(const char* aMarkerName,
-                         ProfilerMarkerPayload* aPayload = nullptr);
+                         mozilla::UniquePtr<ProfilerMarkerPayload> aPayload);
 
 #define PROFILER_APPEND_LINE_NUMBER_PASTE(id, line) id ## line
 #define PROFILER_APPEND_LINE_NUMBER_EXPAND(id, line) \
   PROFILER_APPEND_LINE_NUMBER_PASTE(id, line)
 #define PROFILER_APPEND_LINE_NUMBER(id) \
   PROFILER_APPEND_LINE_NUMBER_EXPAND(id, __LINE__)
 
 // Uncomment this to turn on systrace or build with
--- a/tools/profiler/tests/gtest/GeckoProfiler.cpp
+++ b/tools/profiler/tests/gtest/GeckoProfiler.cpp
@@ -376,25 +376,26 @@ TEST(GeckoProfiler, Markers)
 
   {
     AutoProfilerTracing tracing("C", "A");
 
     profiler_log("X");  // Just a specialized form of profiler_tracing().
   }
 
   profiler_add_marker("M1");
-  profiler_add_marker(
-    "M2", new ProfilerMarkerTracing("C", TRACING_EVENT));
+  profiler_add_marker("M2",
+                      MakeUnique<ProfilerMarkerTracing>("C", TRACING_EVENT));
   PROFILER_MARKER("M3");
   PROFILER_MARKER_PAYLOAD(
-    "M4", new ProfilerMarkerTracing("C", TRACING_EVENT,
-                                    profiler_get_backtrace()));
+    "M4",
+    MakeUnique<ProfilerMarkerTracing>("C", TRACING_EVENT,
+                                      profiler_get_backtrace()));
 
   for (int i = 0; i < 10; i++) {
-    PROFILER_MARKER_PAYLOAD("M5", new GTestPayload(i));
+    PROFILER_MARKER_PAYLOAD("M5", MakeUnique<GTestPayload>(i));
   }
 
   // Sleep briefly to ensure a sample is taken and the pending markers are
   // processed.
   PR_Sleep(PR_MillisecondsToInterval(500));
 
   SpliceableChunkedJSONWriter w;
   ASSERT_TRUE(profiler_stream_json_for_this_process(w));
@@ -413,17 +414,17 @@ TEST(GeckoProfiler, Markers)
   }
 
   profiler_stop();
 
   // The GTestPayloads should have been destroyed.
   ASSERT_TRUE(GTestPayload::sNumDestroyed == 10);
 
   for (int i = 0; i < 10; i++) {
-    PROFILER_MARKER_PAYLOAD("M5", new GTestPayload(i));
+    PROFILER_MARKER_PAYLOAD("M5", MakeUnique<GTestPayload>(i));
   }
 
   profiler_start(PROFILER_DEFAULT_ENTRIES, PROFILER_DEFAULT_INTERVAL,
                  features, filters, MOZ_ARRAY_LENGTH(filters));
 
   ASSERT_TRUE(profiler_stream_json_for_this_process(w));
 
   profiler_stop();
--- a/widget/VsyncDispatcher.cpp
+++ b/widget/VsyncDispatcher.cpp
@@ -6,21 +6,16 @@
 #include "MainThreadUtils.h"
 #include "VsyncDispatcher.h"
 #include "VsyncSource.h"
 #include "gfxPlatform.h"
 #include "mozilla/layers/Compositor.h"
 #include "mozilla/layers/CompositorBridgeParent.h"
 #include "mozilla/layers/CompositorThread.h"
 
-#ifdef MOZ_GECKO_PROFILER
-#include "GeckoProfiler.h"
-#include "ProfilerMarkerPayload.h"
-#endif
-
 using namespace mozilla::layers;
 
 namespace mozilla {
 
 CompositorVsyncDispatcher::CompositorVsyncDispatcher()
   : mCompositorObserverLock("CompositorObserverLock")
   , mDidShutdown(false)
 {
--- a/xpcom/base/CycleCollectedJSContext.cpp
+++ b/xpcom/base/CycleCollectedJSContext.cpp
@@ -41,20 +41,16 @@
 #endif
 
 #include "nsIException.h"
 #include "nsIPlatformInfo.h"
 #include "nsThread.h"
 #include "nsThreadUtils.h"
 #include "xpcpublic.h"
 
-#ifdef MOZ_GECKO_PROFILER
-#include "ProfilerMarkerPayload.h"
-#endif
-
 using namespace mozilla;
 using namespace mozilla::dom;
 
 namespace mozilla {
 
 CycleCollectedJSContext::CycleCollectedJSContext()
   : mIsPrimaryContext(true)
   , mRuntime(nullptr)
--- a/xpcom/base/CycleCollectedJSRuntime.cpp
+++ b/xpcom/base/CycleCollectedJSRuntime.cpp
@@ -79,16 +79,20 @@
 #include "nsContentUtils.h"
 #include "nsCycleCollectionNoteRootCallback.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsCycleCollector.h"
 #include "nsDOMJSUtils.h"
 #include "nsJSUtils.h"
 #include "nsWrapperCache.h"
 #include "nsStringBuffer.h"
+#include "GeckoProfiler.h"
+#ifdef MOZ_GECKO_PROFILER
+#include "ProfilerMarkerPayload.h"
+#endif
 
 #ifdef MOZ_CRASHREPORTER
 #include "nsExceptionHandler.h"
 #endif
 
 #include "nsIException.h"
 #include "nsIPlatformInfo.h"
 #include "nsThread.h"
@@ -828,25 +832,27 @@ CycleCollectedJSRuntime::GCSliceCallback
                                          const JS::GCDescription& aDesc)
 {
   CycleCollectedJSRuntime* self = CycleCollectedJSRuntime::Get();
   MOZ_ASSERT(CycleCollectedJSContext::Get()->Context() == aContext);
 
 #ifdef MOZ_GECKO_PROFILER
   if (profiler_is_active()) {
     if (aProgress == JS::GC_CYCLE_END) {
-      auto payload = new GCMajorMarkerPayload(aDesc.startTime(aContext),
-                                              aDesc.endTime(aContext),
-                                              aDesc.summaryToJSON(aContext));
-      PROFILER_MARKER_PAYLOAD("GCMajor", payload);
+      PROFILER_MARKER_PAYLOAD(
+        "GCMajor",
+        MakeUnique<GCMajorMarkerPayload>(aDesc.startTime(aContext),
+                                         aDesc.endTime(aContext),
+                                         aDesc.summaryToJSON(aContext)));
     } else if (aProgress == JS::GC_SLICE_END) {
-      auto payload = new GCSliceMarkerPayload(aDesc.lastSliceStart(aContext),
-                                              aDesc.lastSliceEnd(aContext),
-                                              aDesc.sliceToJSON(aContext));
-      PROFILER_MARKER_PAYLOAD("GCSlice", payload);
+      PROFILER_MARKER_PAYLOAD(
+        "GCSlice",
+        MakeUnique<GCSliceMarkerPayload>(aDesc.lastSliceStart(aContext),
+                                         aDesc.lastSliceEnd(aContext),
+                                         aDesc.sliceToJSON(aContext)));
     }
   }
 #endif
 
   if (aProgress == JS::GC_CYCLE_END) {
     JS::gcreason::Reason reason = aDesc.reason_;
     Unused <<
       NS_WARN_IF(NS_FAILED(DebuggerOnGCRunnable::Enqueue(aContext, aDesc)) &&
@@ -927,19 +933,19 @@ CycleCollectedJSRuntime::GCNurseryCollec
 #ifdef MOZ_GECKO_PROFILER
   if (aProgress == JS::GCNurseryProgress::GC_NURSERY_COLLECTION_START) {
     self->mLatestNurseryCollectionStart = TimeStamp::Now();
   } else if ((aProgress == JS::GCNurseryProgress::GC_NURSERY_COLLECTION_END) &&
              profiler_is_active())
   {
     PROFILER_MARKER_PAYLOAD(
       "GCMinor",
-      new GCMinorMarkerPayload(self->mLatestNurseryCollectionStart,
-                               TimeStamp::Now(),
-                               JS::MinorGcToJSON(aContext)));
+      MakeUnique<GCMinorMarkerPayload>(self->mLatestNurseryCollectionStart,
+                                       TimeStamp::Now(),
+                                       JS::MinorGcToJSON(aContext)));
   }
 #endif
 
   if (self->mPrevGCNurseryCollectionCallback) {
     self->mPrevGCNurseryCollectionCallback(aContext, aProgress, aReason);
   }
 }