Closed
Bug 1347007
Opened 6 years ago
Closed 6 years ago
Profiler registration creates too many nsThreads
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
People
(Reporter: mrbkap, Assigned: mrbkap)
Details
(Whiteboard: [e10s-multi:+])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
gchang
:
approval-mozilla-aurora-
|
Details |
I found this while debugging bug 1208957 and while fixing this doesn't fix the intermittent orange, I think we should fix this anyway. Currently [1], when we start an nsThread-based thread, we set the thread's name, register it with the profiler, and then try to register it with the thread manager. The problem is that registering a thread with the profiler creates a ThreadInfo object, which forces creation of a *new* nsThread (with the stack: profiler_register_thread -> RegisterCurrentThread -> ThreadInfo::ThreadInfo -> NS_GetCurrentThread). This creates two competing nsThreads for the same PRThread. By registering the new thread with the thread manager before registering it with the profiler, we can avoid this. [1] http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/xpcom/threads/nsThread.cpp#463,466,470
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8846900 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ecd69f8995c29a85bad59ec734a62f471aed02b contains this change.
![]() |
||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8846900 [details] Bug 1347007 - Register with the profiler after doing so for the ThreadManager. https://reviewboard.mozilla.org/r/119888/#review121794 ::: xpcom/threads/nsThread.cpp:473 (Diff revision 1) > nsThreadManager::get().RegisterCurrentThread(*self); > > mozilla::IOInterposer::RegisterCurrentThread(); > > + if (!initData->name.IsEmpty()) { > + profiler_register_thread(initData->name.BeginReading(), &stackTop); Ugh. So, AIUI, the problem is a mixing of layers. XPCOM threads are layered on top of NSPR threads, and this code is part of the XPCOM implementation, so it's using NSPR primitives (e.g. PR_GetCurrentThread()), but the profiler uses the XPCOM layer which is only half set up and problems ensue. Can you add a comment here, something like "This must come after the call to nsThreadManager::RegisterCurrentThread(), because that call is needed to properly set up this thread as an nsThread, which profiler_register_thread() requires. See bug 1347007."
Attachment #8846900 -
Flags: review?(n.nethercote) → review+
Comment hidden (mozreview-request) |
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c62935c570f Register with the profiler after doing so for the ThreadManager. r=njn
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c62935c570f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Iteration: --- → 55.1 - Mar 20
Whiteboard: [e10s-multi:+]
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8846900 [details] Bug 1347007 - Register with the profiler after doing so for the ThreadManager. Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: Unknown [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Stepping through the new code shows it doing the right thing. I don't know of any functional changes this causes. [String changes made/needed]: n/a
Attachment #8846900 -
Flags: approval-mozilla-aurora?
Updated•6 years ago
|
status-firefox54:
--- → affected
Comment 8•6 years ago
|
||
Hi :mrbkap, From the request information, I can't tell why we need this in Aurora54. There is no specified feature/bug causing the regression. Could you give us more details about the user impact if we don't take this in Aurora?
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 9•6 years ago
|
||
To be honest, I don't know what user impact there is as a result of this bug. It is probably minimal.
Flags: needinfo?(mrbkap)
Comment 10•6 years ago
|
||
Comment on attachment 8846900 [details] Bug 1347007 - Register with the profiler after doing so for the ThreadManager. Given there is no significant user impact, I prefer to let this ride the train on 55 and won't fix in 54. Aurora54-.
Attachment #8846900 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•