Closed Bug 1430850 Opened 7 years ago Closed 7 years ago

Submit BHR Profiler Markers to the right threads

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(4 files)

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.

Attachment

General

Created:
Updated:
Size: