Closed Bug 1347348 Opened 3 years ago Closed 3 years ago

Yet more profiler cleanups

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(5 files, 1 obsolete file)

Here are a few more patches for the profiler.
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: nobody → n.nethercote
Status: NEW → ASSIGNED
It took me some time to understand this snippet of code, so I wrote a comment
about it.
Attachment #8847353 - Flags: review?(mstange)
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)
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)
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+
> ::: 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().
> 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.
> ::: 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.
Updated version of part 3.
Attachment #8847844 - Flags: review?(mstange)
Attachment #8847356 - Attachment is obsolete: true
Attachment #8847356 - Flags: review?(mstange)
Attachment #8847844 - Flags: review?(mstange) → review+
> 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.