1891664 - nsThreadPool: Give idle threads a grace period before shutting them down to reduce noise from spinning up/down threads

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Jens Stutte [:jstutte]]

Bug 1733964 and investigation on bug 1891227 show that under certain load patterns we can start to spin up and down thread pool threads very frequently. These conditions in question do spin down a thread immediately if we have more than mIdleThreadLimit idle threads, and this condition will immediately spawn a new thread if we are allowed to and get a new event to serve. Together this can easily lead to a continuous up and down of threads if we fire a sequential flow of events with very short pauses between them.

We want to have a grace period for idle threads before they really go away. This can be achieved with a very similar pattern as for mIdleThreadTimeout. In fact, a common misunderstanding about thread pools is that mIdleThreadTimeout would be this grace period, instead it tells after what (usually long) timeout an idle thread is closed even if we are below our mIdleThreadLimit, ensuring thus that a never used thread pool does not occupy resources forever.

Comment 1

[Jens Stutte [:jstutte]]

Attachment: Bug 1891664 - Have a grace timeout before shutting down excess idle threads. old

Have idleThreadGraceTimeout and idleThreadMaximumTimeout instead of just idleThreadTimeout.
Clarify that idleThreadMaximumTimeout is only affecting allowed idle threads.
Make idle threads end only after at minimum idleThreadGraceTimeout even if they are in excess.
Apply the same idleThreadTimeoutRegressive rule also on idleThreadGraceTimeout.

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1891664 - Add a test for idle handling. r?#xpcom-reviewers

Comment 3

[Jens Stutte [:jstutte]]

It seems we had very similar discussions already in bug 1670737.

Comment 4

[Jens Stutte [:jstutte]]

Attachment: Bug 1891664 - Have an MRU list for idle threads in nsThreadPool and separate condvars for each thread. r?#xpcom-reviewers

D207521 makes it so that idle threads are kept alive a bit longer ("grace timeout") before shutting them down. While this reduces the noise of very short living threads significantly, under certain workloads it can lead to a situation where we keep more idle threads longer than we would expect them to live.

When having more than one idle thread, of the threads waiting on the same condvar the system chooses randomly one. This choice is out of our control and on some systems (Linux for sure) it seems that this can lead to excessive thread bouncing of incoming events between all available idle threads, keeping them all alive when we could serve the requests equally fast with less threads. To avoid this, this patch introduces a "most recently used" priority for notifying idle threads. The management of the linked list has constant time, adding thus only minimal overhead wrt to the previous wasIdle flags we had.

As a side effect (and coming from the investigations in bug 1891732) to some extent this can help to improve the "logical thread affinity", at least for some specific workloads where we have bursts of serialized events coming through a TaskQueue with only few other parallel events coming in. This is no real affinity and can easily be fooled by having several burst of serialized events coming in at the same time, though.

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1891664 - Add some statistics about threadpool usage in nightly and early beta. r?#xpcom-reviewers

For now we just throw out a printf_stderr at thread pool shutdown for
debugging. We should add telemetry for that (or an about:threadpools).
Up the patch stack we remove the printf after adjusting the settings.

Depends on D207785

Comment 7

[Jens Stutte [:jstutte]]

Attachment: Bug 1891664 - Remove printf_stderr for thread pool statistics. r?#xpcom-reviewers

Depends on D209363

Comment 9

[Jens Stutte [:jstutte]]

Attachment: Bug 1891664 - Have a grace timeout before shutting down excess idle threads. r?#!xpcom-reviewers

Have idleThreadGraceTimeout and idleThreadMaximumTimeout instead of just idleThreadTimeout.
Clarify that idleThreadMaximumTimeout is only affecting allowed idle threads.
Make idle threads end only after at minimum idleThreadGraceTimeout even if they are in excess.
Remove the idleThreadTimeoutRegressive setting.

Introduce a "most recently used" priority for notifying idle threads to
avoid excessive round-robin through all available idle threads.
The management of the linked list has constant time, adding thus only
minimal overhead wrt to the previous wasIdle flags we had.

As a side effect (and coming from the investigations in bug 1891732) to
some extent this can help to improve the "logical thread affinity",
together with trying to keep events dispatched with NS_DISPATCH_AT_END
on the dispatching thread as much as possible, which should help
TaskQueue a lot with affinity.

Depends on D207545

Comment 10

[Phabricator Automation]

Comment on attachment 9400131 [details]
Bug 1891664 - Add some statistics about threadpool usage in nightly and early beta. r?#xpcom-reviewers

Revision D209485 was moved to bug 1895073. Setting attachment 9400131 [details] to obsolete.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/559d64791056
Add a test for idle handling. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/3ae2900967ce
Have a grace timeout before shutting down excess idle threads. r=xpcom-reviewers,necko-reviewers,dom-storage-reviewers,nika,janv,jesup#!xpcom-reviewers

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/559d64791056
https://hg.mozilla.org/mozilla-central/rev/3ae2900967ce