Closed
Bug 1340928
Opened 7 years ago
Closed 7 years ago
Still more profiler cleanups
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(16 files, 2 obsolete files)
3.91 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
6.03 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
4.21 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.68 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
11.74 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
10.59 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.17 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
6.90 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.39 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
19.89 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
4.22 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
2.93 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
26.23 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
I'm working on a big patch that overhauls the handling of all the global state in platform.cpp so that it's properly and obviously synchronized. It's a big change so I'm pulling out precursor changes where possible, and this bug is for them.
Assignee | ||
Comment 1•7 years ago
|
||
- Don't bother checking gSampler in ProfilerSignalHandler. It is equivalent to checking gIsActive and we do that at the top of the loop in SignalSender(). There is no point repeatedly checking the same condition in the middle of that loop; that just opens up the possibility of partially complete samples where some threads are missing. - Clear gCurrentThreadInfo in SignalSender() instead of in ProfilerSignalHandler(). The effect is much the same, but this change means gCurrentThreadInfo is both set and cleared in SignalSender(), i.e. on a single thread, removing any need for Atomic<>.
Attachment #8838989 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
This change means that all the relevant code is now within platform-linux-android.cpp, which is nice.
Attachment #8838990 -
Flags: review?(mstange)
Assignee | ||
Comment 3•7 years ago
|
||
There is another PlatformStop() call earlier in the function, and gIsActive is always false by the time we reach the removed call, so it's dead code.
Attachment #8838991 -
Flags: review?(mstange)
Assignee | ||
Comment 4•7 years ago
|
||
It's only being used in a boolean fashion, so this patch replaces it with a boolean.
Attachment #8838992 -
Flags: review?(mstange)
Assignee | ||
Updated•7 years ago
|
Blocks: profiler-cleanup
Assignee | ||
Comment 5•7 years ago
|
||
This avoids the need for platform-linux-android.cpp to read gInterval off the main thread in an awkward spot. It also makes platform-linux-android.cpp more like platform-{win32,macos}.cpp.
Attachment #8838993 -
Flags: review?(mstange)
Assignee | ||
Comment 6•7 years ago
|
||
This patch mostly does formatting fixes. It also removes some declarations from platform.h that are no longer necessary now that platform-linux-android.cpp is in the same compilation unit as platform.cpp (due to it being #include-d directly); this required reordering some things.
Attachment #8838994 -
Flags: review?(mstange)
Assignee | ||
Comment 7•7 years ago
|
||
PlatformStart() and PlatformStop() are currently responsible for setting and clearing gIsActive, but it's better if we do it in profiler_{start,stop}(). The patch also does the following. - Adds some missing emacs/vim modelines. - Makes Platform{Start,Stop}() crash if they have failures. I'm not at all confident that ignoring the errors as is currently done will result in sensible behaviour, so brittleness is better.
Attachment #8838995 -
Flags: review?(mstange)
Assignee | ||
Comment 8•7 years ago
|
||
The profiler can currently handle nested calls to profiler_{init,shutdown}() -- only the first call to profiler_init() and the last call to profiler_shutdown() do anything. And sure enough, we have the following. - Outer init/shutdown pairs in XRE_main()/XRE_InitChildProcess() (via GeckoProfilerInitRAII). - Inner init/shutdown pairs in NS_InitXPCOM2()/NS_InitMinimalXPCOM() (both shut down in ShutdownXPCOM()). This is all rather silly, so the patch removes the inner pairs. This will allow gInitCount -- which tracks the nesting depth -- to be removed in a future patch.
Attachment #8838996 -
Flags: review?(mstange)
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8838996 [details] [diff] [review] (part 8) - Remove nested calls to profiler_{init,shutdown}() Turns out I need to add a GeckoProfilerInitRAII to XRE_XPCShellMain() and I haven't yet worked out the right place to put it. Tomorrow.
Attachment #8838996 -
Flags: review?(mstange)
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8838996 [details] [diff] [review] (part 8) - Remove nested calls to profiler_{init,shutdown}() I'm not sure about this patch so I'm going to abandon it for now.
Attachment #8838996 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
This removes the one use of gStartTime outside of platform*.cpp, which lets us restrict its visibility to just that compilation unit.
Attachment #8839281 -
Flags: review?(mstange)
Assignee | ||
Comment 12•7 years ago
|
||
It's used to set gUnwindStackScan, which is never used.
Attachment #8839302 -
Flags: review?(mstange)
Assignee | ||
Comment 13•7 years ago
|
||
Ok, I worked out where to do the init/shutdown for xpcshell.
Attachment #8839315 -
Flags: review?(mstange)
Assignee | ||
Comment 14•7 years ago
|
||
It looks like it used to be called by multiple constructors, but that's no longer the case.
Attachment #8839325 -
Flags: review?(amarchesini)
Comment 15•7 years ago
|
||
Comment on attachment 8839325 [details] [diff] [review] (part 11) - Inline and remove ContentParent::InitializeMembers Review of attachment 8839325 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +1910,1 @@ > remove this empty line here.
Attachment #8839325 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 16•7 years ago
|
||
profiler_get_gatherer() exposes ProfileGatherer to the outside world in a way that makes future changes difficult. This patch: - Removes ProfileGatherer.h from the list of headers exported from the profiler. - Removes nsIProfiler.profileGatherer and nsProfiler::GetProfileGatherer(). - Replaces profiler_get_gatherer() with three new functions that provide minimal but sufficient access to ProfileGatherer: profiler_will_gather_OOP_profile(), profiler_gathered_OOP_profile(), and profiler_OOP_exit_profile(). These functions provide access to the ProfileGatherer in a similar fashion to the pre-existing functions profiler_get_profile_jsobject_async() and profiler_save_profile_to_file_async() This significantly reduces the size of the profiler's API surface.
Attachment #8839369 -
Flags: review?(mstange)
Assignee | ||
Comment 17•7 years ago
|
||
This is the correct version of the patch.
Attachment #8839725 -
Flags: review?(mstange)
Assignee | ||
Comment 18•7 years ago
|
||
The |nsIFile*| one is only called by the |const nsACString&| one, so this patch combines them.
Attachment #8839753 -
Flags: review?(mstange)
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8839755 -
Flags: review?(mstange)
Assignee | ||
Comment 20•7 years ago
|
||
It has a single call site.
Attachment #8839784 -
Flags: review?(mstange)
Updated•7 years ago
|
Attachment #8838989 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8838990 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8838991 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8838992 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8838993 -
Flags: review?(mstange) → review+
Comment 21•7 years ago
|
||
Comment on attachment 8838994 [details] [diff] [review] (part 6) - Clean up profiler code relating to env vars Review of attachment 8838994 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/platform.cpp @@ +1253,5 @@ > > enum class ProfilerVerbosity : int8_t { UNCHECKED, NOTVERBOSE, VERBOSE }; > > // Raced on, potentially > static ProfilerVerbosity profiler_verbosity = ProfilerVerbosity::UNCHECKED; This should probably be renamed to gProfilerVerbosity.
Attachment #8838994 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b86ca9a7e8e3364e64fbab7c777f89305e0592e Bug 1340928 (part 1) - Two small platform-linux-android.cpp tweaks. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a18d2124d73caac76f56fc87410fcaec7336b4 Bug 1340928 (part 2) - Don't use ThreadInfo to pass Linux memory measurements. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc8f0119f8ba08e5c2f9782107531c085e794d7 Bug 1340928 (part 3) - Remove Sampler from ProfileGatherer. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/c043ce740d1bed8c359290b5b7125cc27f238147 Bug 1340928 (part 4) - Remove redundant PlatformStop() call in profiler_stop(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6850af9372989b7dd66318076deb6df02ddf55 Bug 1340928 (part 5) - Pass the interval to PlatformStart(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac1a3617dc977f6d4f36e644f41a1dd5acad6ff Bug 1340928 (part 6) - Clean up profiler code relating to env vars. r=mstange.
Assignee | ||
Comment 23•7 years ago
|
||
> This should probably be renamed to gProfilerVerbosity.
I changed it to gVerbosity, and renamed the type ProfilerVerbosity to Verbosity. In both cases the "Profiler" was obvious :)
Updated•7 years ago
|
Attachment #8838995 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8839281 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8839302 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8839315 -
Flags: review?(mstange) → review+
Comment 24•7 years ago
|
||
Comment on attachment 8839369 [details] [diff] [review] (part 12) - Remove profiler_get_gatherer() Review of attachment 8839369 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I was afraid that it was going to make it harder to match up overlapping asynchronous profile collections, but the old API wasn't any better at handling that case, so this is fine.
Attachment #8839369 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8839315 -
Attachment is obsolete: true
Attachment #8839315 -
Flags: review+
Comment 25•7 years ago
|
||
Comment on attachment 8839725 [details] [diff] [review] (part 10) - Remove nested calls to profiler_{init,shutdown}() Review of attachment 8839725 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCShellImpl.cpp @@ +1655,5 @@ > if (CrashReporter::GetEnabled()) > CrashReporter::UnsetExceptionHandler(); > #endif > > + profiler_shutdown(); // This must precede NS_LogTerm(). Do you know why? Might be worth adding a comment.
Attachment #8839725 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8839753 -
Flags: review?(mstange) → review+
Comment 26•7 years ago
|
||
Comment on attachment 8839755 [details] [diff] [review] (part 14) - Factor out code repeated in both ProfileGatherer::Start() methods Review of attachment 8839755 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/gecko/ProfileGatherer.h @@ +27,5 @@ > private: > ~ProfileGatherer() {}; > void Finish(); > void Reset(); > + void Start2(double aSinceTime); StartImpl? I don't have a strong opinion on it though.
Attachment #8839755 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8839784 -
Flags: review?(mstange) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4aa11ecb4d6873c113abf48f0694c9da8150d1 Bug 1340928 (part 7) - Factor out gIsActive handling in platform-*.cpp. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac2f3a144257a917ac118499f54c6e18862bd21 Bug 1340928 (part 8) - Pass gStartTime to DuplicateLastSample(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b681adb0173c3d41faec2e7ff1cea163b6fdb6 Bug 1340928 (part 9) - Remove MOZ_PROFILER_STACK_SCAN env var. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7e56055a3ec7b7b4bec6a5a1bc9265e0cb4a4d Bug 1340928 (part 10) - Remove nested calls to profiler_{init,shutdown}(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/c6afc9b586b6f81b7f7bce2cff2e0cc1e8623282 Bug 1340928 (part 11) - Inline and remove ContentParent::InitializeMembers. r=baku. https://hg.mozilla.org/integration/mozilla-inbound/rev/34b0a316c4130407a4b7bcaca4be8557945ddb9b Bug 1340928 (part 12) - Remove profiler_get_gatherer(). r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f1cf49bed1d6466244b54d9d9b2483a1fc0627 Bug 1340928 (part 13) - Merge two ProfileGatherer::Start() variants. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d5dcaa2c361b2b5bd66f9ff06292098305c726 Bug 1340928 (part 14) - Factor out code repeated in both ProfileGatherer::Start() methods. r=mstange. https://hg.mozilla.org/integration/mozilla-inbound/rev/494906dbf2d99146cc25c27f8cb51f84c6fe4759 Bug 1340928 (part 15) - Inline and remove ToJSObject(). r=mstange.
Assignee | ||
Comment 28•7 years ago
|
||
This patch does the following. - Uses "entries" consistently for the name of the value that is obtained from MOZ_PROFILER_ENTRIES and is the first argument to profiler_start(). (I.e. not "entry" or "entrySize".) - Removes variables (e.g. PROFILER_HELP) holding env var names and uses the names (e.g. "MOZ_PROFILER_HELP") directly. Some of the names are already used directly and I think the slight repetition isn't harmful. It's unlikely that we'd want to change these names the way we might need to change a numeric value, and they're perfectly descriptive. - Changes the "MOZ_PROFILING_FEATURES" string in the weird Android-only startup code to be "MOZ_PROFILER_FEATURES", for consistency. - Renames gUnwindInterval and gProfileEntries as gEnvVarInterval and gEnvVarEntries to make it clearer that they come from environment variables, but otherwise are parallel to gInterval and gEntries. - Puts entries before intervals in most places, to match the profiler_start() argument order. - Changes profiler_usage() so that (a) it always prints, no matter the verbosity, (b) it exits at its end, and (c) doesn't double-print "Profiler: " at the start of each line.
Attachment #8840407 -
Flags: review?(mstange)
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b86ca9a7e8e https://hg.mozilla.org/mozilla-central/rev/a3a18d2124d7 https://hg.mozilla.org/mozilla-central/rev/cbc8f0119f8b https://hg.mozilla.org/mozilla-central/rev/c043ce740d1b https://hg.mozilla.org/mozilla-central/rev/7e6850af9372 https://hg.mozilla.org/mozilla-central/rev/8ac1a3617dc9 https://hg.mozilla.org/mozilla-central/rev/6e4aa11ecb4d https://hg.mozilla.org/mozilla-central/rev/8ac2f3a14425 https://hg.mozilla.org/mozilla-central/rev/b6b681adb017 https://hg.mozilla.org/mozilla-central/rev/aa7e56055a3e https://hg.mozilla.org/mozilla-central/rev/c6afc9b586b6 https://hg.mozilla.org/mozilla-central/rev/34b0a316c413 https://hg.mozilla.org/mozilla-central/rev/e5f1cf49bed1 https://hg.mozilla.org/mozilla-central/rev/d0d5dcaa2c36 https://hg.mozilla.org/mozilla-central/rev/494906dbf2d9
Updated•7 years ago
|
Attachment #8840407 -
Flags: review?(mstange) → review+
Comment 30•7 years ago
|
||
Pushed by nnethercote@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d41302b8481 (part 16) - Clean up profiler env var handling. r=mstange.
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d41302b8481
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•