Yet more profiler cleanups

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
Here are a few more patches for the profiler.
Assignee

Comment 1

2 years ago
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.
Attachment #8847351 - Flags: review?(mstange)
Assignee

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
It took me some time to understand this snippet of code, so I wrote a comment
about it.
Attachment #8847353 - Flags: review?(mstange)
Assignee

Comment 3

2 years ago
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().
Attachment #8847356 - Flags: review?(mstange)
Assignee

Comment 4

2 years ago
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.
Attachment #8847360 - Flags: review?(mstange)
Assignee

Comment 5

2 years ago
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.
Attachment #8847374 - Flags: review?(mstange)
Attachment #8847351 - Flags: review?(mstange) → review+
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
Attachment #8847353 - Flags: review?(mstange) → review+
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 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 .
Attachment #8847360 - Flags: review?(mstange) → review+
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.
Attachment #8847374 - Flags: review?(mstange) → review+
Assignee

Comment 10

2 years ago
> ::: 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().
Assignee

Comment 11

2 years ago
> 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.
Oh. Right.
Assignee

Comment 13

2 years ago
> ::: 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.
Assignee

Comment 14

2 years ago
Updated version of part 3.
Attachment #8847844 - Flags: review?(mstange)
Assignee

Updated

2 years ago
Attachment #8847356 - Attachment is obsolete: true
Attachment #8847356 - Flags: review?(mstange)
Attachment #8847844 - Flags: review?(mstange) → review+
Assignee

Comment 17

2 years ago
> 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.
You need to log in before you can comment on or make changes to this bug.