1895846 - Make SharedThreadPool less volatile

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Jens Stutte [:jstutte]]

Currently SharedThreadPool shuts down a pool as soon as consumers do not hold a reference to it. In case of consumers acquiring a new reference frequently without keeping it across uses, this can lead to many thread shutdowns and re-creations. MediaPDecoder seems to be an example of such usage. It seems in WebRTC code we added some AddRef calls here and there to account for this.

It might be easier for consumers, if SharedThreadPool keeps an ever-used pool alive until shutdown. If it is really not used for a while, all its threads will be closed and we would just consume the thread pool's object memory.

Comment 1

[Jens Stutte [:jstutte]]

Attachment: WIP: Bug 1895846 - Streamline SharedThreadPool a bit and audit its uses for thread numbers and timeouts. r?#xpcom-reviewers

SharedThreadPool's interface makes consumers believe that they do not
need to keep a reference to the obtained thread pool alive and that
SharedThreadPool will manage the shutdown behavior.

The latter is kind of true, but the first is not: as soon as the last
reference to the same named pool is dropped, it will be destroyed. For
consumers that repeatedly do something on a pool with the same name but
do not store the reference long enough across those uses, this will
result in frequent re-creations of the same named pool, resulting in
more frequent thread spin-ups than needed.

Given that the nsThreadPool object itself is pretty lightweight and
its threads will time out and be closed eventually if the pool is idle
for long enough, it is probably better to keep it around until
shutdown, benefitting from its idle timeouts for the next usage by the
same consumer if it arrives soon enough. We seem to not have any use
that is causing an infinite number of different thread pool names such
that this could really matter for memory usage.

We also make no use of SharedThreadPool's ability to change the thread
limit on each retreival, thus it get's removed. Together with that we
revisit some thread limits and timeout values.

Depends on D209197

Comment 2

[Jens Stutte [:jstutte]]

Looking at how SharedThreadPool manages its shutdown I came across:

nsThreadPool::PutEvent bails out on shutdown regardless of the source of an event.

if (NS_WARN_IF(mShutdown && !IsOnCurrentThreadInfallible())) might be a better choice, together with avoiding to spawn a new thread below when mShutdown (the one we are running on can just take the event).

which might deserve an own bug.

Comment 4

[Jens Stutte [:jstutte]]

We might also want to consider renaming SharedThreadPool.

Comment 5

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Have SharedThreadPoolHolder to avoid some fiddly AddRefs. r?#xpcom-reviewers

Previously SharedThreadPool combined both the functionality of wrapping a thread pool and
the book keeping for being in use. While this minimizes the number of objects created, it
makes it hard to understand and even harder to add a grace timeout (for example due to the
order of locking during AddRef/Release).

This patch establishes a SharedThreadPoolHolder which is unique to each shared pool and
manages its book keeping, while handing out separate thin SharedThreadPool wrappers on each
SharedThreadPool::Get. The destruction of each SharedThreadPool wrapper will trigger the
holder's book keeping of active clients.

As a side effect we can avoid having a reentrant monitor but just use a StaticMutex for
our book keeping and clean up a bit.

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Have a grace timeout for SharedThreadPool. r?#xpcom-reviewers

Previously we did shut down shared pools as soon as the last reference to it was dropped.
But some consumers do not really care about keeping the reference to the pool longer than
strictly needed to do a piece of work, which can lead to frequent shut down and create
cycles.

Now that we have the SharedThreadPoolHolder and thus decoupled the book keeping of
active consumers from reference counting, we can easily keep the holder and pool alive
for a bit longer before destroying them.

During XPCOM shutdown instead we do not use a delay and try to shutdown all orphaned pools
immediately. We continue to wait for shared pools that are still in use (there might be
a case for shutting down the pool in any case and leak worst case only the wrapper
and an empty pool, but that could have side effects during shutdown).

Comment 7

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Ensure PostTimerEvent will not release anything important while holding the monitor's lock. r?#xpcom-reviewers

Short living event targets (like SharedThreadPool or TaskQueue or even dying threads)
must never be released while we hold TimerThread's monitor locked. PostTimerEvent needs
to keep a strong reference to the target before unlocking and must release that
reference before re-acquiring the lock.

The same goes for nsTimerEvent, which we can just forget on Dispatch as it cares about
leaking in case of a failed dispatch.

Comment 8

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Keep SharedThreadPool instances until shutdown but close idle threads earlier. r?#xpcom-reviewers

Comment 9

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Drive-by: Make sPools guarded in SharedThreadPool. r?#xpcom-reviewers

Comment 10

[Phabricator Automation]

Comment on attachment 9501751 [details]
Bug 1895846 - Ensure PostTimerEvent will not release anything important while holding the monitor's lock. r?#xpcom-reviewers

Revision D257966 was moved to bug 1999508. Setting attachment 9501751 [details] to obsolete.

Comment 11

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Add SharedThreadPool logging. r?#xpcom-reviewers

Comment 12

[Jens Stutte [:jstutte]]

Attachment: Bug 1895846 - Forward Un/RegisterShutdownTask to the underlying pool in SharedThreadPool. r?#xpcom-reviewers

Now that all SharedThreadPools are explicitly shutdown regardless of other objects still referencing them, we can leverage the shutdown tasks also here to shut down associated TaskQueues (and maybe other things registering in the future).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/654244600cf5
https://hg.mozilla.org/integration/autoland/rev/e85d06f7dc60
Add SharedThreadPool logging. r=xpcom-reviewers,nika
https://github.com/mozilla-firefox/firefox/commit/b4b3a6c71af6
https://hg.mozilla.org/integration/autoland/rev/6976ad06cf8d
Keep SharedThreadPool instances until shutdown but close idle threads earlier. r=xpcom-reviewers,media-playback-reviewers,nika,karlt
https://github.com/mozilla-firefox/firefox/commit/0fc599421fc2
https://hg.mozilla.org/integration/autoland/rev/dc866ea7c36b
Forward Un/RegisterShutdownTask to the underlying pool in SharedThreadPool. r=xpcom-reviewers,nika

Comment 14

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/e85d06f7dc60
https://hg.mozilla.org/mozilla-central/rev/6976ad06cf8d
https://hg.mozilla.org/mozilla-central/rev/dc866ea7c36b