Closed
Bug 1430850
Opened 7 years ago
Closed 7 years ago
Submit BHR Profiler Markers to the right threads
Categories
(Core :: Gecko Profiler, enhancement)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
Attachments
(4 files)
5.26 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
5.00 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.27 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.64 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: LE6j8Fm38PK
Attachment #8942968 -
Flags: review?(mstange)
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: J8Ckvmj4En0
Attachment #8942969 -
Flags: review?(mstange)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 2nyp98oBKHk
Attachment #8942970 -
Flags: review?(mstange)
Assignee | ||
Comment 4•7 years ago
|
||
MozReview-Commit-ID: BivFEJD2KCY
Attachment #8942972 -
Flags: review?(mstange)
Updated•7 years ago
|
Attachment #8942968 -
Flags: review?(mstange) → review+
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8942972 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 7•7 years ago
|
||
(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
Comment 9•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•