mozglue's AutoProfilerLabel could direct labels to Base Profiler if enabled
Categories
(Core :: Gecko Profiler, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
Details
Attachments
(4 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
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.
|
Assignee | |
Comment 1•6 years ago
|
||
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.
|
|
Assignee | |
Comment 2•6 years ago
|
||
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
|
|
Assignee | |
Comment 3•6 years ago
|
||
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
|
|
Assignee | |
Comment 4•6 years ago
|
||
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
|
|
Assignee | |
Comment 5•6 years ago
|
||
Sample profile: https://perfht.ml/31qqIqu
Notice the "WindowsDllBlocklist::patched_LdrLoadDll..." labels in the "Main Thread (Base Profiler)" call tree.
|
|
Assignee | |
Comment 6•6 years ago
|
||
Markus: Review please? (Or just let me know if I should ask someone else.)
|
||
Comment 7•6 years ago
|
||
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.
|
|
Assignee | |
Comment 8•6 years ago
|
||
(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.
|
||
Comment 10•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/8fed7bc35767
https://hg.mozilla.org/mozilla-central/rev/c9da4cd2c6f5
https://hg.mozilla.org/mozilla-central/rev/cb011f546027
https://hg.mozilla.org/mozilla-central/rev/c52f0346e630
Description
•