1983309 - Reduce locking contention in TaskQueue

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently, when running a number of tasks dispatched to a TaskQueue, we acquire the TaskQueue's QueueMonitor 3 times per-task. This is an excessive number of locking calls, and is likely partially responsible for worsening the locking contention issue from bug 1981051.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1983309 - Reduce lock contention in TaskQueue. r=#xpcom-reviewers

This patch aims to reduce lock contention when dispatching to TaskQueue, by reducing the frequency with which the runner thread needs to acquire the TaskQueue monitor.
Prior to this change, if there is a backlog of queued runnables, TaskQueue::Runner would acquire mQueueMonitor 3 times per event. After this change, mQueueMonitor will only be acquired when the set of tasks read the last time the monitor was held have all been processed.
TaskQueue would need to change to use an unbounded lock-free MPSC queue in order to further reduce contention when dispatching. Doing so is a much larger project, and would require more substantial changes to the TaskQueue type to make state management (especially around shutdown) avoid locking.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/deb16dbc30b9
https://hg.mozilla.org/integration/autoland/rev/ae00850eabbf
Reduce lock contention in TaskQueue, r=xpcom-reviewers,jstutte

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9e34621d5341
https://hg.mozilla.org/integration/autoland/rev/cfab6ef2c2e6
Revert "Bug 1983309 - Reduce lock contention in TaskQueue, r=xpcom-reviewers,jstutte" for causing multiple failures

Comment 4

[Cristina Horotan [:chorotan]]

Backed out for causing multiple failures

Backout link

Push with failures

Failure log mda-bc
Failure log bc
Failure push mda
Failure push gtest

Comment 5

[BugBot [:suhaib / :marco]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 6

[Jens Stutte [:jstutte]]

I think there is just higher priority work than fixing the backout, right now.

Comment 7

[Jens Stutte [:jstutte]]

Attachment: Bug 1983309 - Stop relying on TaskQueue::IsEmpty() from inside an AudioSinkWrapper init runnable. r?#media-playback-reviewers

MaybeAsyncCreateAudioSink used mAsyncInitTaskQueue->IsEmpty() inside
the dispatched runnable as a "let the newest call do the init"
coalesce check. With the previous commit's bulk-steal Runner,
IsEmpty() returns !mIsRunning rather than mTasks.IsEmpty(), so it
stays false for the duration of any in-flight runnable and the
coalesce check always reports "newer queued" — the AudioSink would
never actually initialize.

Replace the TaskQueue-state query with an explicit monotonic dispatch
sequence counter on the wrapper. Each MaybeAsyncCreateAudioSink call
increments mAsyncDispatchSeq and the dispatched runnable captures the
value; if the live counter has advanced past that snapshot when the
runnable runs, a newer call has superseded it and we short-circuit.
Only the newest dispatch does the init, preserving the existing
"later wins" semantic.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3d4820e909f8
https://hg.mozilla.org/integration/autoland/rev/8d217b003092
Stop relying on TaskQueue::IsEmpty() from inside an AudioSinkWrapper init runnable. r=media-playback-reviewers,nika,karlt
https://github.com/mozilla-firefox/firefox/commit/325ec7d64ad2
https://hg.mozilla.org/integration/autoland/rev/aec432fc144f
Reduce lock contention in TaskQueue. r=xpcom-reviewers,jstutte

Comment 9

[amarc]

https://hg.mozilla.org/mozilla-central/rev/8d217b003092
https://hg.mozilla.org/mozilla-central/rev/aec432fc144f