Closed Bug 1362444 Opened 7 years ago Closed 7 years ago

allow WorkerHolders to avoid increasing the busy count


(Core :: DOM: Workers, enhancement)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: bkelly, Assigned: bkelly)




(3 files, 3 obsolete files)

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.
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
P1 Allow idle worker shutdown to begin for an opt-in WorkerHolder mode. r=baku
P2 Allow idle worker shutdown while Cache/CacheStorage DOM objects exist, but block it during Cache operation. r=baku
P3 Add a mochitest verifying accessing self.caches does not block idle Worker shutdown. r=baku
You need to log in before you can comment on or make changes to this bug.