Yet more profiler cleanups

RESOLVED FIXED in Firefox 55

Status

()

Core
Gecko Profiler
RESOLVED FIXED
2 months ago
28 days ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

2 months ago
Here are a few more patches for the profiler.
(Assignee)

Comment 1

2 months ago
Created attachment 8847351 [details] [diff] [review]
(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.
Attachment #8847351 - Flags: review?(mstange)
(Assignee)

Updated

2 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 months ago
Created attachment 8847353 [details] [diff] [review]
(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.
Attachment #8847353 - Flags: review?(mstange)
(Assignee)

Comment 3

2 months ago
Created attachment 8847356 [details] [diff] [review]
(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().
Attachment #8847356 - Flags: review?(mstange)
(Assignee)

Comment 4

2 months ago
Created attachment 8847360 [details] [diff] [review]
(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.
Attachment #8847360 - Flags: review?(mstange)
(Assignee)

Comment 5

2 months ago
Created attachment 8847374 [details] [diff] [review]
(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.
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 months 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 months 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 months 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 months ago
Created attachment 8847844 [details] [diff] [review]
(part 3) - Rejig thread registration in the profiler

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

Updated

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

Comment 15

2 months ago
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

2 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 17

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