Submit BHR Profiler Markers to the right threads

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments)

Currently there are a few problems with the BHR profiler markers:

a) They are stored on HangDetails, but not serialized when sent over IPC, so the value on the other side is garbage.
b) They are always registered on the main thread, even if the hang was detected off-main-thread.

These patches add a new API to the profiler to allow submitting a marker for a thread other than the current thread, and change BHR to use that API.
MozReview-Commit-ID: LE6j8Fm38PK
Attachment #8942968 - Flags: review?(mstange)
MozReview-Commit-ID: BivFEJD2KCY
Attachment #8942972 - Flags: review?(mstange)
Attachment #8942968 - Flags: review?(mstange) → review+
Comment on attachment 8942969 [details] [diff] [review]
Part 2: Change ProfilerMarker to use an internal ThreadId property, rather than the ThreadId entry

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

::: tools/profiler/core/ProfileBufferEntry.cpp
@@ +921,5 @@
> +  // Stream all markers whose threadId matches aThreadId. We skip other entries,
> +  // because we process them in StreamSamplesToJSON().
> +  //
> +  // NOTE: The ThreadId of a marker is determined by its GetThreadId() method,
> +  // rather than ThreadId entries, as they can be added outside of samples.

"rather than ThreadId buffer entries, as markers can be added outside of samples."
Attachment #8942969 - Flags: review?(mstange) → review+
Comment on attachment 8942970 [details] [diff] [review]
Part 3: Expose profiler_add_marker_for_thread to add a marker for a specific thread

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

::: tools/profiler/core/platform.cpp
@@ +3313,5 @@
> +      realThread = true;
> +      break;
> +    }
> +  }
> +  MOZ_ASSERT(realThread, "Invalid thread id");

Hmm, is this better?

MOZ_ASSERT(
  std::any_of(liveThreads.begin(), liveThreads.end(),
              [aThreadId](ThreadInfo* info){
    return info->ThreadId() == aThreadId;
  }),
  "Invalid thread id"); 

... probably not.
Attachment #8942970 - Flags: review?(mstange) → review+
Attachment #8942972 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #6)
> Hmm, is this better?
> 
> MOZ_ASSERT(
>   std::any_of(liveThreads.begin(), liveThreads.end(),
>               [aThreadId](ThreadInfo* info){
>     return info->ThreadId() == aThreadId;
>   }),
>   "Invalid thread id"); 
> 
> ... probably not.

Don't really understand / know most of the std algorithms, so I avoid using them TBH. IMO the current code is more clear than that *shrug*.
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22667aec75a
Part 1: Move mThreadId from ThreadInfo to RacyThreadInfo, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9628da12e3f
Part 2: Change ProfilerMarker to use an internal ThreadId property, rather than the ThreadId entry, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee21d20f68dd
Part 3: Expose profiler_add_marker_for_thread to add a marker for a specific thread, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f09976eaa7a
Part 4: Rework BHR's profiler marker to use add_marker_for_thread, r=mstange
You need to log in before you can comment on or make changes to this bug.