Closed Bug 1373436 Opened 2 years ago Closed 2 years ago

Use UniquePtr more in the profiler

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(4 files, 1 obsolete file)

No description provided.
Summary: Use MakeUnique more in the profiler → Use UniquePtr more in the profiler
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8878951 - Flags: review?(mstange)
Attachment #8878951 - Attachment is obsolete: true
Attachment #8878951 - Flags: review?(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.
Attachment #8878954 - Flags: review?(mstange)
Attachment #8878950 - Flags: review?(mstange) → review+
Attachment #8878952 - Flags: review?(mstange) → review+
Attachment #8878953 - Flags: review?(mstange) → review+
Comment on attachment 8878954 [details] [diff] [review]
(part 4) - Use UniquePtr with profile_add_marker()

Review of attachment 8878954 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/GeckoProfiler.h
@@ +109,5 @@
> +// Note: this is deliberately not defined when MOZ_GECKO_PROFILER is defined.
> +// 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.

But if the payload creation is within the parentheses of the macro invocation, then it won't happen if the PROFILER_MARKER_PAYLOAD define doesn't make use of the payload parameter.

So this argument isn't entirely convincing to me.

However, this macro not being defined does make it fairly obvious that users of it need to be deliberate about how much data gathering work is necessary in builds with and without the profiler. But then again, any unnecessary data gathering work would show up as "unused variable" warnings in builds without the profiler.

I can't decide which way to go, so I suppose it doesn't really matter. I'll leave it up to you what to do (if anything) about my comment here.

::: tools/profiler/tests/gtest/GeckoProfiler.cpp
@@ +379,5 @@
>  
>      profiler_log("X");  // Just a specialized form of profiler_tracing().
>    }
>  
> +  profiler_add_marker("M1", nullptr);

This ", nullptr" seems unnecessary.
Attachment #8878954 - Flags: review?(mstange) → review+
Comment on attachment 8878954 [details] [diff] [review]
(part 4) - Use UniquePtr with profile_add_marker()

Review of attachment 8878954 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/public/GeckoProfiler.h
@@ +105,5 @@
>  #define PROFILER_MARKER(marker_name) do {} while (0)
> +
> +// Like PROFILER_MARKER, but with an additional payload.
> +//
> +// Note: this is deliberately not defined when MOZ_GECKO_PROFILER is defined.

This sentence has the wrong number of negations.
> I can't decide which way to go, so I suppose it doesn't really matter. I'll
> leave it up to you what to do (if anything) about my comment here.

Commenting it out is a much simpler mechanism for preventing screw-ups, so I'll stick with that.
Thank you for the fast reviews.
You need to log in before you can comment on or make changes to this bug.