Closed Bug 1431755 Opened 7 years ago Closed 7 years ago

Profile thread responsiveness on nsIThreads other than main

Categories

(Core :: Gecko Profiler, enhancement)

Product:
Core
Component:
Gecko Profiler
Version:
59 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(3 files)

Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1431755 - Part 3: Remove a redundant dispatch that was causing us to record the duration of _two_ dispatches instead of one.
59 bytes, text/x-review-board-request
mstange
: review+
Details
It would be extremely useful to be able to see thread responsiveness on all threads in Gecko profiler. For now, just getting the nsIThreads would be a large improvement.
Attachment #8943984 - Flags: review?(nfroyd)
Attachment #8943985 - Flags: review?(mstange)
Attachment #8943986 - Flags: review?(mstange)
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220030 There are some documentation fixes to make here, but there's also a broader question about what we're trying to accomplish, see below. ::: xpcom/threads/nsThreadManager.h:43 (Diff revision 1) > > // Returns the current thread. Returns null if OOM or if ThreadManager isn't > // initialized. > nsThread* GetCurrentThread(); > > + bool IsNsThread() const; This method needs documentation. `HasNSThread` sounds like a better name to me, WDYT? To be clear, returning `false` from this method is only ever clear for some point in time, right? We could call this, get `false` back, act as though there was no `nsThread`, and then have the `nsThread` created for us. My impression is that the question this method really wants to provide an answer to is, "is this thread, now or going to be at any time in the future, running an `nsThread` event loop?" Is that a more correct (if lengthier) summay? Assuming that's the case, can we do better than what this patch proposes, or is this just a limitation that we should stick in the documentation? ::: xpcom/threads/nsThreadUtils.h:371 (Diff revision 1) > // Fast access to the current thread. Do not release the returned pointer! If > // you want to use this pointer from some other thread, then you will need to > // AddRef it. Otherwise, you should only consider this pointer valid from code > // running on the current thread. Perhaps we should also change this documentation to indicate that an `nsThread` is created if one does not already exist? ::: xpcom/threads/nsThreadUtils.h:377 (Diff revision 1) > // you want to use this pointer from some other thread, then you will need to > // AddRef it. Otherwise, you should only consider this pointer valid from code > // running on the current thread. > extern nsIThread* NS_GetCurrentThread(); > > +extern nsIThread* NS_GetCurrentThreadNoCreate(); This function needs documentation. The name is kind of a mouthful too..`NS_QueryCurrentThread`, maybe? I'm unsure if there's something shorter than communicates the `NoCreate` aspect of it.
Attachment #8943984 - Flags: review?(nfroyd)
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220030 > This method needs documentation. `HasNSThread` sounds like a better name to me, WDYT? > > To be clear, returning `false` from this method is only ever clear for some point in time, right? We could call this, get `false` back, act as though there was no `nsThread`, and then have the `nsThread` created for us. > > My impression is that the question this method really wants to provide an answer to is, "is this thread, now or going to be at any time in the future, running an `nsThread` event loop?" Is that a more correct (if lengthier) summay? Assuming that's the case, can we do better than what this patch proposes, or is this just a limitation that we should stick in the documentation? For instance, I'm pretty sure we create `nsThread`s on threads not created by us, even if those threads aren't running a Gecko event loop. We could set a TLS variable when we entered `nsThread`'s run loop to enable us to be more precise here. That scheme obviously doesn't answer the question about event loops running in the future, but maybe it would be an improvement on what this patch provides?
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220030 > For instance, I'm pretty sure we create `nsThread`s on threads not created by us, even if those threads aren't running a Gecko event loop. We could set a TLS variable when we entered `nsThread`'s run loop to enable us to be more precise here. That scheme obviously doesn't answer the question about event loops running in the future, but maybe it would be an improvement on what this patch provides? It seems to me that answering "is this thread, now or going to be at any time in the future, running an nsThread event loop?" is impossible, right? My intent was to answer the question "does this thread run an nsThread event loop right now?", to avoid the profiler tacking superfluous event queues to threads that won't ever service them. From what I can tell, threads that are destined to have an nsThread event loop (other than main) already have it by the time the profiler calls locked_register_thread. I'm fine with renaming. > Perhaps we should also change this documentation to indicate that an `nsThread` is created if one does not already exist? That would be sensible. I wish the name of the function was more self-documenting, but that would require touching _lots_ of code, not to mention the amount of "Wait, where did NS_GetCurrentThread go?" > This function needs documentation. The name is kind of a mouthful too..`NS_QueryCurrentThread`, maybe? I'm unsure if there's something shorter than communicates the `NoCreate` aspect of it. That was the most terse way I could think of, given that NS_GetCurrentThread was taken by code that ought to be named something like NS_GetOrCreateCurrentThread.
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220030 > It seems to me that answering "is this thread, now or going to be at any time in the future, running an nsThread event loop?" is impossible, right? My intent was to answer the question "does this thread run an nsThread event loop right now?", to avoid the profiler tacking superfluous event queues to threads that won't ever service them. From what I can tell, threads that are destined to have an nsThread event loop (other than main) already have it by the time the profiler calls locked_register_thread. > > I'm fine with renaming. Is there any utility to creating an nsThread for a thread that will never service its event queue? It seems to me that doing this is undesireable, because any such nsThread doesn't actually satisfy the API of nsIEventTarget. I would argue that such nsThreads are a mistake to create, and are _probably_ created due to the misleading nature of NS_GetCurrentThread.
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220256 ::: xpcom/threads/nsThreadUtils.h:371 (Diff revision 2) > } > > //----------------------------------------------------------------------------- > > #ifdef MOZILLA_INTERNAL_API > -// Fast access to the current thread. Do not release the returned pointer! If > +// Fast access to the current thread. Will create an nsIThread of one does not Nit: "...if one does not exist already!"
Attachment #8943984 - Flags: review?(nfroyd) → review+
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 Do we need this function? All nsThreads share the same thread func, and are registered with the same call to PROFILER_REGISTER_THREAD(): https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/xpcom/threads/nsThread.cpp#405 (at least once bug 1431184 lands) Could we pass the nsIThread to the profiler in that registration call and store a reference on the ThreadInfo?
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 Let me try.
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 I've looked into this, and there are a _lot_ of entry points that need to be modified, mostly AUTO_PROFILER_REGISTER_THREAD.
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 But do any of the other callers have an nsThread event loop on their thread?
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 Some of them appear to. Each one would have to be inspected.
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 Also, I would argue that we really ought to have this function, as someone who works on code with lots of non-nsThread threads.
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 I like Markus's suggestion, but am sympathetic to the idea that we do need this function. But is it really that common to write code that runs on either a bare OS thread or an nsThread? Or is it usually that the codepaths are so similar that it's not worth abstracting out the differences, and one would rather stick in an `if (nsCOMPtr<nsIThread> t = NS_GetCurrentThreadIfExists())`?
Comment on attachment 8943984 [details] Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. https://reviewboard.mozilla.org/r/214308/#review220266 I cannot speak for the project as a whole, but I've seen it happen enough in just the profiler code.
Comment on attachment 8943985 [details] Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. https://reviewboard.mozilla.org/r/214310/#review221474 Looks good except for the fact that this makes all the events unlabeled again (regressing bug 1378975). Should we use the labeled dispatch functions + timer creation functions for the main thread? I don't know how valuable the labeling is, maybe request review from froydnj on this part, too. ::: tools/profiler/core/ThreadInfo.h:233 (Diff revision 2) > // Returns nullptr if this is not the main thread or if this thread is not > // being profiled. > ThreadResponsiveness* GetThreadResponsiveness() > { > ThreadResponsiveness* responsiveness = mResponsiveness.ptrOr(nullptr); > - MOZ_ASSERT(!!responsiveness == (mIsMainThread && mIsBeingProfiled)); > + MOZ_ASSERT(!responsiveness || mIsBeingProfiled); This seems more relaxed than "!!responsiveness == mIsBeingProfiled". Does it need to be this relaxed?
Attachment #8943985 - Flags: review?(mstange) → review+
Comment on attachment 8943986 [details] Bug 1431755 - Part 3: Remove a redundant dispatch that was causing us to record the duration of _two_ dispatches instead of one. https://reviewboard.mozilla.org/r/214312/#review221478
Attachment #8943986 - Flags: review?(mstange) → review+
Comment on attachment 8943985 [details] Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. https://reviewboard.mozilla.org/r/214310/#review221474 > This seems more relaxed than "!!responsiveness == mIsBeingProfiled". Does it need to be this relaxed? Yes, because we cannot profile responsiveness on non-nsIThreads. Really all we can assert here is that we aren't profiling responsiveness on threads that aren't being profiled.
(In reply to Markus Stange [:mstange] from comment #20) > Comment on attachment 8943985 [details] > Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on > nsIThreads. > > https://reviewboard.mozilla.org/r/214310/#review221474 > > Looks good except for the fact that this makes all the events unlabeled > again (regressing bug 1378975). Should we use the labeled dispatch functions > + timer creation functions for the main thread? I don't know how valuable > the labeling is, maybe request review from froydnj on this part, too. ^ Thoughts? We can only label runnables for main thread, right? Does it make sense to go to that trouble?
Flags: needinfo?(nfroyd)
(In reply to Byron Campen [:bwc] from comment #23) > (In reply to Markus Stange [:mstange] from comment #20) > > Comment on attachment 8943985 [details] > > Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on > > nsIThreads. > > > > https://reviewboard.mozilla.org/r/214310/#review221474 > > > > Looks good except for the fact that this makes all the events unlabeled > > again (regressing bug 1378975). Should we use the labeled dispatch functions > > + timer creation functions for the main thread? I don't know how valuable > > the labeling is, maybe request review from froydnj on this part, too. > > ^ Thoughts? We can only label runnables for main thread, right? Does it make > sense to go to that trouble? Yes, we can only label runnables for the main thread, and yes, it does make sense to go to that trouble.
Flags: needinfo?(nfroyd)
Comment on attachment 8943985 [details] Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. https://reviewboard.mozilla.org/r/214310/#review221474 Ok, I have put the labeling back for main thread.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s c8d70f3528e63fd94d432cd9babadd9211f23fab -d 3a155114f168: rebasing 444784:c8d70f3528e6 "Bug 1431755 - Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. r=froydnj" merging xpcom/threads/nsThreadManager.cpp merging xpcom/threads/nsThreadManager.h merging xpcom/threads/nsThreadUtils.h rebasing 444785:4dd102858cca "Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. r=mstange" merging tools/profiler/core/ThreadInfo.cpp merging tools/profiler/core/ThreadInfo.h merging tools/profiler/core/platform.cpp merging tools/profiler/gecko/ThreadResponsiveness.cpp warning: conflicts while merging tools/profiler/core/ThreadInfo.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging tools/profiler/gecko/ThreadResponsiveness.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1878f143ccda Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/8116e9cfc3f0 Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. r=mstange https://hg.mozilla.org/integration/autoland/rev/c1befb56cf4d Part 3: Remove a redundant dispatch that was causing us to record the duration of _two_ dispatches instead of one. r=mstange
That... is actually a useful test failure. And the test that caught this bug was only added a few days ago. Nice.
> Assertion failure: cancelable (Only nsICancelableRunnable may be dispatched to a worker!), at /builds/worker/workspace/build/src/dom/workers/WorkerThread.cpp:242
Huh. Alrighty then, easy enough to fix I suppose.
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bb20165a692a Part 1: Add a variant of NS_GetCurrentThread that does not auto-create an nsIThread. r=froydnj https://hg.mozilla.org/integration/autoland/rev/b7ac4d81eb11 Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. r=mstange https://hg.mozilla.org/integration/autoland/rev/be80ea498686 Part 3: Remove a redundant dispatch that was causing us to record the duration of _two_ dispatches instead of one. r=mstange
Comment on attachment 8943985 [details] Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. https://reviewboard.mozilla.org/r/214310/#review223460 ::: tools/profiler/gecko/ThreadResponsiveness.cpp:101 (Diff revision 7) > - // Main thread only > + // Should always fire on the thread being profiled > NS_IMETHOD Notify(nsITimer* aTimer) final override > { > - SystemGroup::Dispatch(TaskCategory::Other, > + mThread->Dispatch(this, nsIThread::NS_DISPATCH_NORMAL); > - do_AddRef(this)); > return NS_OK; > } Should this also be changed to use SystemGroup::Dispatch if called on the main thread?
Comment on attachment 8943985 [details] Bug 1431755 - Part 2: Teach GeckoProfiler to profile responsiveness on nsIThreads. https://reviewboard.mozilla.org/r/214310/#review223460 > Should this also be changed to use SystemGroup::Dispatch if called on the main thread? Oh, oops, this line gets removed in part 3. Never mind.
Depends on: 1447858
Depends on: 1439624
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: