1347348 - Yet more profiler cleanups

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

Here are a few more patches for the profiler.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't export ProfilerBacktrace.h

ProfilerBacktrace.h doesn't need to be visible outside the profiler because
the ProfilerBacktrace type is only used in pointers outside the profiler and
the existing forward declaration in GeckoProfiler.h suffices for that.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add a comment about the profiler's "startTime" field

It took me some time to understand this snippet of code, so I wrote a comment
about it.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rejig thread registration in the profiler

Specifically:

- Renames is_main_thread() as IsMainThread(), and RegisterCurrentThread() as
  locked_register_thread(). This actually increases consistency within the
  file :)

- Removes the aIsMainThread parameter from locked_register_thread(). It can be
  computed from aName.

- Moves the PseudoStack initialization within locked_register_thread(), so it's
  done in a single place. Also calls init() at that place; previously the
  non-main threads didn't have init() called on their tlsPseudoStack! According
  to ThreadLocal.h this shouldn't have worked...

- Adds some !NS_IsMainThread() assertions to
  profiler_{register,unregister}_thread().

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Fix comments in GeckoProfiler.h

Specifically:

- Improve the documentation on all the profiler_*() functions, esp. about what
  they do when the profiler is inactive.

- Fix numerous spelling mistakes.

- Wrap all comment lines at 80 chars.

- Change /**/ comments to // comments throughout.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove some unnecessary profiler_is_active() calls

profiler_get_profiler() will return nullptr is the profiler is not active, so
there's no need to call profiler_is_active() just beforehand.

This patch removes two such calls.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8847353 [details] [diff] [review]
(part 2) - Add a comment about the profiler's "startTime" field

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

Looks like TimeStamp could use a method called MillisecondsSince1970(), similar to https://developer.apple.com/reference/foundation/nsdate/1407504-timeintervalsince1970?language=objc

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8847356 [details] [diff] [review]
(part 3) - Rejig thread registration in the profiler

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

::: tools/profiler/core/platform.cpp
@@ -2681,5 @@
>  
> -  MOZ_ASSERT(tlsPseudoStack.get() == nullptr);
> -  NotNull<PseudoStack*> stack = WrapNotNull(new PseudoStack());
> -  tlsPseudoStack.set(stack);
> -  bool isMainThread = is_main_thread_name(aName);

Could this ever have returned true? We assert that the profiler has been initialized (gPS) and that tlsPseudoStack has not been initialized (i.e. this thread has not been registered yet), but profiler_init registers the main thread.

In other words, I'd prefer to remove is_main_thread_name() completely and keep the bool argument, if that makes sense.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8847360 [details] [diff] [review]
(part 4) - Fix comments in GeckoProfiler.h

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

Thanks for all the clarifications about what happens when the profiler is inactive.

::: tools/profiler/public/GeckoProfiler.h
@@ +18,5 @@
> +//
> +// The profiler collects samples in a platform-independent way by using a
> +// pseudo stack abstraction of the real program stack by using 'sample stack
> +// frames'. When a sample is collected all active sample stack frames and the
> +// program counter are recorded.

This comment should probably be adjusted to mention that we don't only walk the platform-independent pseudostack but also the native stack, and native stack walking has platform-specific implementations.

@@ +47,5 @@
> +//
> +// 'r' - Responsiveness tag following an 's' tag. Gives an indication on how
> +//       well the application is responding to the event loop. Lower is better.
> +//
> +// 't' - Time elapsed since recording started.

This comment needs to be removed in its entirety. There's nothing in here that matches today's state.

The current profile format is documented at http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/tools/profiler/core/ProfileBufferEntry.h#321 .

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8847374 [details] [diff] [review]
(part 5) - Remove some unnecessary profiler_is_active() calls

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

In the commit message, s/profiler_get_profiler/profiler_get_profile/

r=me assuming you address the RecvGatherProfile comment.

::: dom/ipc/ContentChild.cpp
@@ +2531,4 @@
>    UniquePtr<char[]> profile = profiler_get_profile();
>    if (profile) {
> +    nsCString profileCString = nsCString(profile.get(), strlen(profile.get()));
> +    Unused << SendProfile(profileCString);

We should call SendProfile even if the profiler isn't running (for whatever reason) because otherwise the parent process will be stuck waiting for a reply forever.

Comment 10

[Nicholas Nethercote [inactive]]

> ::: tools/profiler/core/platform.cpp
> @@ -2681,5 @@
> >  
> > -  MOZ_ASSERT(tlsPseudoStack.get() == nullptr);
> > -  NotNull<PseudoStack*> stack = WrapNotNull(new PseudoStack());
> > -  tlsPseudoStack.set(stack);
> > -  bool isMainThread = is_main_thread_name(aName);
> 
> Could this ever have returned true?

No.

> In other words, I'd prefer to remove is_main_thread_name() completely and
> keep the bool argument, if that makes sense.

Ok. We'll need to pass in both aName and aIsMainThread, but that's probably worth it to get rid of is_main_thread_name().

Comment 11

[Nicholas Nethercote [inactive]]

> Ok. We'll need to pass in both aName and aIsMainThread, but that's probably
> worth it to get rid of is_main_thread_name().

Oh, I can just use NS_IsMainThread(). Even better.

Comment 12

[Markus Stange [:mstange]]

Oh. Right.

Comment 13

[Nicholas Nethercote [inactive]]

> ::: dom/ipc/ContentChild.cpp
> @@ +2531,4 @@
> >    UniquePtr<char[]> profile = profiler_get_profile();
> >    if (profile) {
> > +    nsCString profileCString = nsCString(profile.get(), strlen(profile.get()));
> > +    Unused << SendProfile(profileCString);
> 
> We should call SendProfile even if the profiler isn't running (for whatever
> reason) because otherwise the parent process will be stuck waiting for a
> reply forever.

On second thought, I'm not confident that sending an empty message is equivalent to sending no message, so I'll be cautious and revert the changes to this file.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rejig thread registration in the profiler

Updated version of part 3.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cf57731fcfad7698e84ab3e90c8fb8a755694c
Bug 1347348 (part 1) - Don't export ProfilerBacktrace.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/576d31f1a1931f2ee8852d2092bdf217fd134276
Bug 1347348 (part 2) - Add a comment about the profiler's "startTime" field. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1129398d7c96aee75afac1909f5cbdcf43e46c9b
Bug 1347348 (part 3) - Rejig thread registration in the profiler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/69598ad0499e20a0a733766545b75987595e8ecd
Bug 1347348 (part 4) - Fix comments in GeckoProfiler.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9639986a232d4cb72eca9aa32c36cb69d83e14f
Bug 1347348 (part 5) - Remove an unnecessary profiler_is_active() calls. r=mstange.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8cf57731fcf
https://hg.mozilla.org/mozilla-central/rev/576d31f1a193
https://hg.mozilla.org/mozilla-central/rev/1129398d7c96
https://hg.mozilla.org/mozilla-central/rev/69598ad0499e
https://hg.mozilla.org/mozilla-central/rev/c9639986a232

Comment 17

[Nicholas Nethercote [inactive]]

> Looks like TimeStamp could use a method called MillisecondsSince1970(),
> similar to
> https://developer.apple.com/reference/foundation/nsdate/1407504-
> timeintervalsince1970?language=objc

I filed bug 1352897 for this.