Closed Bug 1236789 Opened 4 years ago Closed 4 years ago

TaskQueue creates unnecessary thread(s)

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 + wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: roc, Assigned: jwwang)

References

Details

Attachments

(2 files)

At the end of TaskQueue::Runner::Run() it redispatches itself to its thread pool if it has more tasks to run. Imagine there's a thread pool with one thread and one TaskQueue and we reach that point; the thread pool sees that there are no idle threads, so it spins up another thread. Now the TaskQueue is effectively consuming two threads though it really only needs one since we only allow one task to run at a time.

I think this is really a thread pool API issue. When a thread pool dispatches an event, and that event's last act is to dispatch another event to the same thread pool, it's wasteful to create a new thread since the current thread is about to become idle.

So I'm going to attach a patch that adds a new nsIEventTarget::Dispatch flag to indicate that the dispatcher is an event dispatched to the same event target that is just about to return. nsThreadPool can optimize based on that.

I verified that at least when running under rr, this significantly reduces the number of MediaPlayback threads created to play a simple Ogg video --- to 2, which I think is what we expect. Outside of rr I'd expect this to save just one thread in the media playback thread pool (when the limit has not been reached), but that's probably still worth having with such a simple patch.
Comment on attachment 8703944 [details]
MozReview Request: Bug 1236789. Avoid creating an unnecessary thread pool thread for tail-dispatch in TaskQueue. r=bholley

https://reviewboard.mozilla.org/r/29533/#review26417

r=me with those changes. Nice find!

::: xpcom/threads/SharedThreadPool.h:62
(Diff revision 1)
> +  // Call this when dispatching from an event on the same
> +  // threadpool that is about to complete. We should not create a new thread
> +  // in that case since a thread is about to become idle.
> +  nsresult TailDispatch(nsIRunnable *event) { return Dispatch(event, NS_DISPATCH_TAIL); }

Unfortunately, we already have something similar-but-different called TailDispatch - See all the stuff in AbstractThread.h and TaskDispatcher.h.

So maybe remove the TailDispatch helper, and call the flag NS_DISPATCH_POSTRUN or something?
Attachment #8703944 - Flags: review?(bobbyholley)
I'll make it NS_DISPATCH_AT_END.
FWIW under rr with randomized thread priorities the behavior is worse because we trigger an interesting pathology:
1) Run first paragraph of comment #0
2) The extra thread is spun up, but after its nsThreadStartupEvent notifies the original thread, it yields before that event exits, so it's still not marked as idle
3) The original thread runs the TaskQueue runnable again, i.e. goto 1), consuming an arbitrary number of threads up to the thread pool limit
This probably doesn't happen much in real life, but it could happen depending on kernel scheduling.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> I'll make it NS_DISPATCH_AT_END.

sgtm. r+ with that, I'm just still bad at MozReview.
Also, another win for rr! :-)
Component: Audio/Video → Audio/Video: Playback
This should be a xpcom bug since the changed files are under xpcom/.
Component: Audio/Video: Playback → XPCOM
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ec7338bddf9f1147e19c1d4d1b90a0cdb8da9a
Bug 1236789. Avoid creating an unnecessary thread pool thread for tail-dispatch in TaskQueue. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9dc51c14938a3df662e80506cf80210e3422c0
Back out changeset b5ec7338bddf (bug 1236789) on a CLOSED TREE for causing assertions on most (but not all) debug test runs.
Also because of

130 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_audioTabIcon.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
Assignee: roc → nobody
I will try to look at this bug.
Flags: needinfo?(roc)
Attachment #8726630 - Flags: review?(bobbyholley)
Comment on attachment 8726630 [details]
MozReview Request: Bug 1236789. Part 2 - fix assertions when |aFlags == NS_DISPATCH_TAIL|. r=bholley

https://reviewboard.mozilla.org/r/38123/#review36517
Attachment #8726630 - Flags: review?(bobbyholley) → review+
Thanks!
https://hg.mozilla.org/mozilla-central/rev/024ce6807e88
https://hg.mozilla.org/mozilla-central/rev/a2575708048d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee: nobody → jwwang
Comment on attachment 8703944 [details]
MozReview Request: Bug 1236789. Avoid creating an unnecessary thread pool thread for tail-dispatch in TaskQueue. r=bholley

Approval Request Comment
[Feature/regressing bug #]: bug 1259196 top crash in video playback
[User impact if declined]: Crashes due to out of memory; I believe this fixes this top crash:
https://crash-stats.mozilla.com/report/list?signature=OOM%20%7C%20unknown%20%7C%20mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20moz_xmalloc%20%7C%20mozilla%3A%3AWMFMediaDataDecoder%3A%3AProcessOutput
[Describe test coverage new/current, TreeHerder]: We have lots of media mochitests
[Risks and why]: This changes how our threading works, so that's concerning, but given that this change has been in aurora and nightly for a while and is looking fine there, so I think we can consider the risk mitigated.
[String/UUID change made/needed]: None.
Attachment #8703944 - Flags: approval-mozilla-beta?
Patch part2 also needs to be uplifted which fix some assertion from part 1.
Comment on attachment 8703944 [details]
MozReview Request: Bug 1236789. Avoid creating an unnecessary thread pool thread for tail-dispatch in TaskQueue. r=bholley

Fix for topcrash, please uplift for beta 6.
Attachment #8703944 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Sounds like both patches should land.
Comment on attachment 8703944 [details]
MozReview Request: Bug 1236789. Avoid creating an unnecessary thread pool thread for tail-dispatch in TaskQueue. r=bholley

Un-approving. This doesn't look like a topcrash in beta 46. Since we aren't shipping e10s in 46 I don't think we need to take on this fix or any risk it would entail this late in the 46 beta cycle.
Attachment #8703944 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.