allow WorkerHolders to avoid increasing the busy count

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
Attachment #8864990 - Attachment is obsolete: true
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.
Attachment #8864995 - Flags: review?(amarchesini)
I think I am close to having a test that demonstrates the problem with CacheStorage.
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.
Attachment #8865469 - Flags: review?(amarchesini)
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
Attachment #8865471 - Flags: review?(amarchesini)
Attachment #8865471 - Flags: review?(amarchesini) → review+
Attachment #8865469 - Flags: review?(amarchesini) → review+
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.
Attachment #8864995 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/179023a3c485
https://hg.mozilla.org/mozilla-central/rev/4dfa701c5489
https://hg.mozilla.org/mozilla-central/rev/551b49873e80
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.