868700 - Structured clones of typed arrays cause leaks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gregor Wagner [:gwagner]]

Attachment: about:memory

We have a regression on current nightly. It woks fine on FF 21.
STR:
Just go to the URL and start the worker videos. Firefox will grow and grow and grow and grow....

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Alice, can you do a bisection here?

Comment 2

[Alice0775 White]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Alice, can you do a bisection here?

Unable to reproduce...

>STR:
>Just go to the URL and start the worker videos.

How do I start the worker videos?

Comment 3

[Michael Bebenita [:mbx]]

Clicking on the Canvas should do it, just wait a few moments for the video to download entirely. Once it's cached, it should be a lot faster.

Comment 4

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ab0974ead030
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130411 Firefox/23.0 ID:20130411161820
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/058c640a3799
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130411 Firefox/23.0 ID:20130411164419
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab0974ead030&tochange=058c640a3799

Suspected: 
 058c640a3799	Steve Fink — Bug 789593 - Clone typed arrays by cloning their buffers and only saving construction parameters. r=jorendorff, bent Also enters a compartment where needed, and bumps both the structured clone and IndexedDB schema versions

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks Alice.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

lsblakk, looks like your tracking flags got wiped out here.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This belongs in the JS engine.

Comment 8

[Steve Fink [:sfink] [:s:]]

The STR works fine for me, so I see it. Bug 861925 will probably fix this, but it's a pretty big change and may be too big to backport. I will look into a more targeted fix.

Comment 9

[Steve Fink [:sfink] [:s:]]

Bug 861925 will *not* fix this.

I don't think this is a leak. I believe what is happening is that the main thread is sending over chunks of compressed data to the workers by grabbing views of portions of one large buffer and postMessage'ing them.

Previously (before bug 789593), this would result in the data within that view getting copied into a structured clone buffer and a new ArrayBuffer+TypedArray pair constructed with that data. Now, the *entire* underlying ArrayBuffer is copied into the clone buffer, then the worker constructs a new ArrayBuffer out of that and makes a view of the appropriate portion.

This is what the spec says to do, and results in 3 copies of the full ArrayBuffer in memory at once temporarily. Worse, if the main thread sends multiple views of the same buffer over to the worker, it will make one full additional copy for each one. Unfortunately there doesn't seem to be any spec-compatible way to get the previous behavior without a manual copy of the portion of interest.

With Transferable ArrayBuffers, you could send the whole ArrayBuffer over with zero copying at all -- but only one worker could have it at a time, so you couldn't use multiple workers to process separate parts of the encoded data buffer.

Comment 10

[Steve Fink [:sfink] [:s:]]

For the life of me, I can't figure out the clone buffer management. Even in the nonleaking version, I see many more structured clone buffers written to than are ever read by the worker. It seems like the main thread sends over more messages than the worker can handle, and the worker never accesses the data within those messages, so it is never read out. Which is fine; perhaps it's intentionally dropping overdue frames or something, but not even MessageEventRunnable::WorkerRun seems to be getting called (in a quick sample run, I had 3178 MessageEventRunnable instances created and destroyed, but only 878 of them were run.) I am baffled as to why that *doesn't* leak, since with the current code it looks like that would leak a pointer to the data in MessageEventRunnable.mData. Or maybe it does, but it's not enough data to notice? (needinfo? bent)

At any rate, I locally patched it to use RAII guards for both the MessageEventRunnable and the MessageEvent, so that this would definitely get cleaned up, but the overall behavior is the same (namely: massive memory usage with the new structured clone typed array code, nothing noticeable with the old.) In fact, since the structured clone code is backwards-compatible, I can toggle which way it writes out typed arrays with an environment variable and see the difference. So the primary leak this bug is about definitely seems to be due to the whole-buffer cloning.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The worker message loop isn't very complicated... If the worker can't keep up with the number of messages sent by the main thread then it won't get to run all of them. The queue will keep building up.

When the worker is shut down (either when closing the browser or calling 'terminate') then those MessageEventRunnables will be deleted. I think it's definitely a bug that we don't clear the structured clone data in the destructor there.

If the worker isn't being shut down then those MessageEventRunnables will eventually run given enough time and memory.

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Manage structured clone memory with JSAutoStructuredCloneBuffer to prevent leaking unused buffers

With this patch, plus updating Broadway to copy Uint8Array views before cloning, the leak is gone.

Comment 13

[Steve Fink [:sfink] [:s:]]

That is to say, this patch fixes a leak, but the Broadway demo memory usage is still out of control even after this patch is applied, because it's constantly cloning 50MB buffers and only using 350KB of them. That's enough to run my machine out of memory, so there may be a GC scheduling issue here.

Comment 14

[Steve Fink [:sfink] [:s:]]

I submitted a pull request of https://github.com/hotsphink/Broadway/commit/1c500d6700c89207c57d347941cc389710b0aa36 to improve its memory handling.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 761162 [details] [diff] [review]
Manage structured clone memory with JSAutoStructuredCloneBuffer to prevent leaking unused buffers

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

r=me with this addressed:

::: dom/workers/Events.cpp
@@ +444,5 @@
>  
>    virtual ~MessageEvent()
>    {
>      MOZ_COUNT_DTOR(mozilla::dom::workers::MessageEvent);
> +    JS_ASSERT(!mBuffer.data());

You're no longer forcing the free in the finalize hook so I don't think you can assert this, right?

Comment 16

[Steve Fink [:sfink] [:s:]]

Filed bug 881914 on the (potential) GC scheduling problem.

Comment 17

[Steve Fink [:sfink] [:s:]]

(In reply to ben turner [:bent] from comment #15)
> Comment on attachment 761162 [details] [diff] [review]
> Manage structured clone memory with JSAutoStructuredCloneBuffer to prevent
> leaking unused buffers
> 
> Review of attachment 761162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with this addressed:
> 
> ::: dom/workers/Events.cpp
> @@ +444,5 @@
> >  
> >    virtual ~MessageEvent()
> >    {
> >      MOZ_COUNT_DTOR(mozilla::dom::workers::MessageEvent);
> > +    JS_ASSERT(!mBuffer.data());
> 
> You're no longer forcing the free in the finalize hook so I don't think you
> can assert this, right?

I do believe you are correct. Thanks.

Comment 18

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2f1bb6823625

Comment 19

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2f1bb6823625

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f1bb6823625

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Uplift nomination?

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 761162 [details] [diff] [review]
Manage structured clone memory with JSAutoStructuredCloneBuffer to prevent leaking unused buffers

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 789593
User impact if declined: potentially massive memory leaks in pages using a combination of features (Workers + postMessaged typed arrays + high message frequency)
Testing completed (on m-c, etc.): on m-c and uplifted to aurora
Risk to taking this patch (and alternatives if risky): discussed via email
String or IDL/UUID changes made by this patch: none

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 761162 [details] [diff] [review]
Manage structured clone memory with JSAutoStructuredCloneBuffer to prevent leaking unused buffers

Approving for beta uplift since, as discussed in email, bug 887420 has also been uplifted.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Fwiw, this is already marked fixed on FF24 so ignore the aurora approval.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/55e0f601dc37

Comment 26

[Bogdan Maris, Desktop Test Engineering]

The link provided in comment 0 has expired (404 error). Is there another TC or steps in order to reproduce the issue.

Comment 27

[Bogdan Maris, Desktop Test Engineering]

Ups, the ending of my above comment was a question.