Closed
Bug 1362444
Opened 5 years ago
Closed 5 years ago
allow WorkerHolders to avoid increasing the busy count
Categories
(Core :: DOM: Workers, enhancement)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(3 files, 3 obsolete files)
5.12 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.80 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
Attachment #8864987 -
Attachment is obsolete: true
Assignee | ||
Comment 4•5 years ago
|
||
Attachment #8864966 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8864990 -
Attachment is obsolete: true
Assignee | ||
Comment 5•5 years ago
|
||
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)
Assignee | ||
Comment 6•5 years ago
|
||
I think I am close to having a test that demonstrates the problem with CacheStorage.
Assignee | ||
Comment 7•5 years ago
|
||
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)
Assignee | ||
Comment 8•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #8865471 -
Flags: review?(amarchesini) → review+
Updated•5 years ago
|
Attachment #8865469 -
Flags: review?(amarchesini) → review+
Comment 9•5 years ago
|
||
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+
Comment 10•5 years ago
|
||
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
Comment 11•5 years ago
|
||
bugherder |
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: 5 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•