1362444 - allow WorkerHolders to avoid increasing the busy count

Status: RESOLVED FIXED

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently a WorkerHolder does two things:

1. Adds the holder object to a list.
2. Increases the worker busy count.

The worker holder list prevents thread shutdown as long as its non-empty.

The busy count, on the other hand, also prevents worker shutdown from *starting* due to cycle collection.  This means if a WorkerHolder is alive on a dedicated worker and the window just de-ref's the Worker object, then the worker thread will not begin shutting down.

I need this for bug 1293277 because the ClientManager is created for every worker thread at startup and has a WorkerHolder.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow idle worker shutdown to begin for an opt-in WorkerHolder mode. r=baku

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Allow idle worker shutdown while Cache/CacheStorage DOM objects exist, but block it during Cache operation. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Allow idle worker shutdown while Cache/CacheStorage DOM objects exist, but block it during Cache operation. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow idle worker shutdown to begin for an opt-in WorkerHolder mode. r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8864995 [details] [diff] [review]
P1 Allow idle worker shutdown to begin for an opt-in WorkerHolder mode. r=baku

Andrea, over in bug 1293277 I have a WorkerHolder that gets created for every worker thread and its not accessible via cycle collection.  I mainly need to know when worker thread shutdown starts so I can clean up actors and such.  But I don't want the WorkerHolder to prevent the natural shutdown of idle workers.

To that end I'd like to create an optional, non-default WorkerHolder mode that does not increase the busy count.

I thought I could work around this by using WorkerPrivate::ModifyBusyCountFromWorker(), but thats not quite right given that the busy count is only adjusted for the first WorkerHolder placed in the list.  I really need something like this patch to expose the primitive I need.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I think I am close to having a test that demonstrates the problem with CacheStorage.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Allow idle worker shutdown while Cache/CacheStorage DOM objects exist, but block it during Cache operation. r=baku

This patch makes the CacheStorage and Cache actors use a WorkerHolder with AllowIdleShutdownStart.  Cache operations like match, put, etc use a WorkerHolder with PreventIdleShutdownStart.  This will allow a worker thread that has touched self.caches to idle shutdown once its completed actively doing operations.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a mochitest verifying accessing self.caches does not block idle Worker shutdown. r=baku

This test verifies that a dedicated Worker that has touched self.caches can be shutdown and GC'd once the following conditions are met:

1) The window's DOM Worker object is no longer referenced.
2) The worker thread is idle.

Without the P1 and P2 patches the Worker object is never GC'd because the WorkerPrivate's busy count is stuck at 1.  This makes the test fail.

With the P1 and P2 patches, though, the busy count drops back to 0 on idle and the worker is GC'd.  This allows the test to pass.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20dac74de50b918dfabcb419e13daa931cfc8621

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8864995 [details] [diff] [review]
P1 Allow idle worker shutdown to begin for an opt-in WorkerHolder mode. r=baku

Review of attachment 8864995 [details] [diff] [review]:
-----------------------------------------------------------------

This is very interesting. We should check where else we can use this feature.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/179023a3c485
P1 Allow idle worker shutdown to begin for an opt-in WorkerHolder mode. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dfa701c5489
P2 Allow idle worker shutdown while Cache/CacheStorage DOM objects exist, but block it during Cache operation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/551b49873e80
P3 Add a mochitest verifying accessing self.caches does not block idle Worker shutdown. r=baku

Comment 11

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/179023a3c485
https://hg.mozilla.org/mozilla-central/rev/4dfa701c5489
https://hg.mozilla.org/mozilla-central/rev/551b49873e80