1340928 - Still more profiler cleanups

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Two small platform-linux-android.cpp tweaks

- 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<>.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't use ThreadInfo to pass Linux memory measurements

This change means that all the relevant code is now within
platform-linux-android.cpp, which is nice.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove redundant PlatformStop() call in profiler_stop()

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove Sampler from ProfileGatherer

It's only being used in a boolean fashion, so this patch replaces it with a
boolean.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Pass the interval to PlatformStart()

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.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Clean up profiler code relating to env vars

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.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Factor out gIsActive handling in platform-*.cpp

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.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Remove nested calls to profiler_{init,shutdown}()

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.

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Pass gStartTime to DuplicateLastSample()

This removes the one use of gStartTime outside of platform*.cpp, which lets us
restrict its visibility to just that compilation unit.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Remove MOZ_PROFILER_STACK_SCAN env var

It's used to set gUnwindStackScan, which is never used.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Remove nested calls to profiler_{init,shutdown}()

Ok, I worked out where to do the init/shutdown for xpcshell.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Inline and remove ContentParent::InitializeMembers

It looks like it used to be called by multiple constructors, but that's no
longer the case.

Comment 15

[Andrea Marchesini [:baku]]

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.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Remove profiler_get_gatherer()

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.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Remove nested calls to profiler_{init,shutdown}()

This is the correct version of the patch.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Merge two ProfileGatherer::Start() variants

The |nsIFile*| one is only called by the |const nsACString&| one, so this patch
combines them.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - Factor out code repeated in both ProfileGatherer::Start() methods

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Inline and remove ToJSObject()

It has a single call site.

Comment 21

[Markus Stange [:mstange]]

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.

Comment 22

[Nicholas Nethercote [inactive]]

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.

Comment 23

[Nicholas Nethercote [inactive]]

> 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 :)

Comment 24

[Markus Stange [:mstange]]

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.

Comment 25

[Markus Stange [:mstange]]

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.

Comment 26

[Markus Stange [:mstange]]

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.

Comment 27

[Nicholas Nethercote [inactive]]

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.

Comment 28

[Nicholas Nethercote [inactive]]

Attachment: (part 16) - Clean up profiler env var handling

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.

Comment 29

[Carsten Book [:Tomcat]]

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d41302b8481
(part 16) - Clean up profiler env var handling. r=mstange.

Comment 31

[Nicholas Nethercote [inactive]]

That's enough patches for this bug.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d41302b8481