1651612 - ArrayBuffer sent to worker is leaked until termination / memory minimised

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P2)

Description

[Jon Coppeard (:jonco)]

Split off from bug 1407691 comment 18.

The following test case:

let worker = new Worker("data:text/javascript,onmessage=function(){}");
let buffer = new ArrayBuffer(1024 * 1024 * 512);
worker.postMessage(buffer, [buffer]);

Gives a worker holding 512MB of memory that doesn't clean up until minimize memory is used in about:memory or the worker is terminated.

Comment 1

[Jon Coppeard (:jonco)]

The problem is that not all worker garbage is being collected. In the test case above which uses a large array buffer this leads to significant unnecessary memory use.

The reason for this is that is can take more than one GC cycle for all garbage to be collected. In the test case there's a DOM MessageEvent object with a dead JS wrapper that holds it's data object (the array buffer) live. A single GC will collect the dead wrapper and cause the MessageEvent to be cleaned up, but only at that point does the array buffer get removed from the browser's gray root set. A further GC is necessary to collect it.

This isn't a problem for normal browser operation since GCs happen relatively often, but it's an issue for workers since they GC when they go idle but not necessarily again after that.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1651612 - Run extra garbage collection cycle for idle workers if cycle collection collected anything r?mccr8

This runs an extra GC cycle when a worker goes idle if the previous one removed any JS holder roots. This fixes the test case given (but not the case in the original bug).

Comment 3

[Andrew McCreight [:mccr8]]

Thanks for looking into this. This is interesting. We haven't looked too much at tuning worker GC.

However, I think your approach here isn't quite right. Wrapper cached objects will call DropJSObjects() in Unlink(), but many other JS holders don't call it until their dtors. Although, maybe this is okay because non-wrappercached objects are more likely to go away from the unlinking?

Maybe it would work better to have some logic like nsCycleCollector::ShutdownCollect(), that checks if any objects were collected? Maybe we could surface that value to nsCycleCollector_collect? It would be nice to use the existing mechanism for detecting the productivity of the CC rather than creating a new one.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Wrapper cached objects will call DropJSObjects() in Unlink(), but many other JS holders don't call it until their dtors.

What's the issue here? Is it that the cycle collector will call Unlink() but destructors may be deferred and run later, so we wouldn't see the holders being removed?

Maybe it would work better to have some logic like nsCycleCollector::ShutdownCollect(), that checks if any objects were collected?

That would work, but I think it could run the GC again even if there was no new garbage for it to collect, right? But maybe this patch will miss things that being collectable for other reasons. Anyway, I'll try this.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Jon Coppeard (:jonco) from comment #4)

What's the issue here? Is it that the cycle collector will call Unlink() but destructors may be deferred and run later, so we wouldn't see the holders being removed?

Yes, that is what I was thinking of. I'm not sure if that's an actual issue, however.

Maybe it would work better to have some logic like nsCycleCollector::ShutdownCollect(), that checks if any objects were collected?

That would work, but I think it could run the GC again even if there was no new garbage for it to collect, right? But maybe this patch will miss things that being collectable for other reasons. Anyway, I'll try this.

We do track when GCed objects are "unlinked" in the collector, so we could have some kind of smarter retry cycle. But yeah, what you did in the patch is all I meant. I was mostly after making sure we didn't have a new separate mechanism to see if the CC did anything.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae6b01f1f4b
Run extra garbage collection cycle for idle workers if cycle collection collected anything r=mccr8

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0ae6b01f1f4b