1430850 - Submit BHR Profiler Markers to the right threads

Status: RESOLVED FIXED

Description

[:Nika Layzell (Away until Aug. 26)]

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.

Comment 1

[:Nika Layzell (Away until Aug. 26)]

Attachment: Part 1: Move mThreadId from ThreadInfo to RacyThreadInfo

MozReview-Commit-ID: LE6j8Fm38PK

Comment 2

[:Nika Layzell (Away until Aug. 26)]

Attachment: Part 2: Change ProfilerMarker to use an internal ThreadId property, rather than the ThreadId entry

MozReview-Commit-ID: J8Ckvmj4En0

Comment 3

[:Nika Layzell (Away until Aug. 26)]

Attachment: Part 3: Expose profiler_add_marker_for_thread to add a marker for a specific thread

MozReview-Commit-ID: 2nyp98oBKHk

Comment 4

[:Nika Layzell (Away until Aug. 26)]

Attachment: Part 4: Rework BHR's profiler marker to use add_marker_for_thread

MozReview-Commit-ID: BivFEJD2KCY

Comment 5

[Markus Stange [:mstange]]

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."

Comment 6

[Markus Stange [:mstange]]

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.

Comment 7

[:Nika Layzell (Away until Aug. 26)]

(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*.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b22667aec75a
https://hg.mozilla.org/mozilla-central/rev/f9628da12e3f
https://hg.mozilla.org/mozilla-central/rev/ee21d20f68dd
https://hg.mozilla.org/mozilla-central/rev/8f09976eaa7a