Closed Bug 1559000 Opened 4 years ago Closed 4 years ago

mozglue's AutoProfilerLabel could direct labels to Base Profiler if enabled

Categories

(Core :: Gecko Profiler, enhancement, P3)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

Details

Attachments

(4 files)

Bug 1559000 - mozglue's AutoProfilerLabel doesn't need to know about ProfilingStack - r?mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1559000 - Make AutoProfilerLabel thread-safe - r?mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1559000 - Enable/disable mozglue's AutoProfilerLabel when Gecko Profiler starts/stops - r?mstange
47 bytes, text/x-phabricator-request
Details | Review
Bug 1559000 - Enable/disable mozglue's AutoProfilerLabel when Base Profiler starts/stops - r?mstange
47 bytes, text/x-phabricator-request
Details | Review

AutoProfilerLabel in mozglue allows adding labels (pseudo stack frames) from non-libxul code.
When Gecko Profiler starts, it registers its own label functions so that AutoProfilerLabel can call them.

It should be possible to extend this so that Base Profiler may inject similar label-handling functions when Gecko Profiler is not in use.

Eventually I expect all non-libxul code to always use Base Profiler (which would work with Gecko Profiler), so AutoProfilerLabel should disappear. But in the meantime I think there is value to easily add labels to Base Profiler profiles.
In particular, the WindowsDllBlocklist::patched_LdrLoadDll pre-xpcom labels would be nice to have.

ProfilingStack* happens to be the information that the current Gecko Profiler
entry function wants to forward to the exit function, but AutoProfilerLabel does
not really need to know about that.
Changing it to void*, so that we can later use different entry/exit functions
that use different context types.

Profilers will soon be able to set/reset entry&exit functions at different
times, but simultaneously other code may want to use AutoProfilerLabel, so we
need to make this all thread-safe.

All shared static information is now encapsulated in an RAII class that enforces
proper locking before giving access to this information.

Also added a "generation" count, so that if an AutoProfilerLabel is in-flight
when entry&exit functions are changed, the context given by the old entry
function will not be passed to a mismatched new exit function.

Depends on D34806

Instead of setting entry&exit function when Gecko Profiler is initialized, we
now set them when profiling actually starts, and reset them when profiling
stops. AutoProfilerLabel should not be called outside of profiling sessions
anyway, and edge cases (missing or unneeded labels at the very start or end of a
session) are not an issue.

Depends on D34807

Now that Gecko Profiler only registers its entry&exit functions when running,
and it ensures that Base Profiler is stopped beforehand, Base Profiler can now
register its own entry&exit functions to capture labels before xpcom starts.

Depends on D34808

Sample profile: https://perfht.ml/31qqIqu
Notice the "WindowsDllBlocklist::patched_LdrLoadDll..." labels in the "Main Thread (Base Profiler)" call tree.

Markus: Review please? (Or just let me know if I should ask someone else.)

Flags: needinfo?(mstange)

I've done the reviews now, sorry for the delay! Didn't have time during the work week, then took 2 days of PTO, then didn't find a chance to do the reviews Wed-Fri, and then had a holiday on Monday.

A lot of the complexity here comes from the fact that BaseProfiler and the regular profiler have two different profiling stacks. We should put effort into making it so that there's only one profiling stack (the BaseProfiler's profiling stack) and then remove this complexity again.
In fact, once there's only one profiling stack, we don't need to pass around callback functions any more at all, because mozglue code will be able to use the regular AutoProfilerLabel directly! That sounds very promising.

Flags: needinfo?(mstange)

(In reply to Markus Stange [:mstange] from comment #7)

I've done the reviews now, sorry for the delay! Didn't have time during the work week, then took 2 days of PTO, then didn't find a chance to do the reviews Wed-Fri, and then had a holiday on Monday.

Thank you for the first review. No worries, busy times!

A lot of the complexity here comes from the fact that BaseProfiler and the regular profiler have two different profiling stacks. We should put effort into making it so that there's only one profiling stack (the BaseProfiler's profiling stack) and then remove this complexity again.

I plan on enabling BaseProfiler for Windows next week (after the soft freeze and version merge), after which I'll start working on de-duplication, including of the profiling stacks.

In fact, once there's only one profiling stack, we don't need to pass around callback functions any more at all, because mozglue code will be able to use the regular AutoProfilerLabel directly! That sounds very promising.

That's right, we shouldn't need the dual AutoProfilerLabel anymore after all this!

But I thought this fairly small change was worth it, as it will add some valuable pre-xpcom labels right now.

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fed7bc35767
mozglue's AutoProfilerLabel doesn't need to know about ProfilingStack - r=mstange
https://hg.mozilla.org/integration/autoland/rev/c9da4cd2c6f5
Make AutoProfilerLabel thread-safe - r=mstange
https://hg.mozilla.org/integration/autoland/rev/cb011f546027
Enable/disable mozglue's AutoProfilerLabel when Gecko Profiler starts/stops - r=mstange
https://hg.mozilla.org/integration/autoland/rev/c52f0346e630
Enable/disable mozglue's AutoProfilerLabel when Base Profiler starts/stops - r=mstange
You need to log in before you can comment on or make changes to this bug.