Closed Bug 1347007 Opened 6 years ago Closed 6 years ago

Profiler registration creates too many nsThreads

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Iteration:
55.1 - Mar 20
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

(Whiteboard: [e10s-multi:+])

Attachments

(1 file)

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
Attachment #8846900 - Flags: review?(n.nethercote)
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+
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
https://hg.mozilla.org/mozilla-central/rev/4c62935c570f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Iteration: --- → 55.1 - Mar 20
Whiteboard: [e10s-multi:+]
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?
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)
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 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-
You need to log in before you can comment on or make changes to this bug.