843733 - With Ion enabled on Android, shumway demo keeps using up memory and eventually dies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Geoff Brown [:gbrown] (pto, back Jun 10)]

This bug is just like bug 842838, but for the Android/Shumway experience.

On Android, running the shumway (http://mozilla.github.com/shumway) demos in the inspector works for a while but eventually Firefox for Android is killed by the Android low memory killer.

For example, running on a Galaxy Nexus, and executing http://mozilla.github.com/shumway/examples/inspector/inspector.html?rfile=../pac/pac3.swf, PacMan runs for 4 to 5 minutes and then is killed. about:memory dumps at 1 minute intervals show constantly increasing RSS.

If I set javascript.options.ion.content=false and re-run the demo, it seems to run fine (for at least 1 hour).

I will post about:memory dumps.

Comment 1

[Geoff Brown [:gbrown] (pto, back Jun 10)]

Attachment: tar archive of memory dumps taken 1 minute apart while pac3 demo running -- ion enabled

Process killed shortly after last memory dump.

Comment 2

[Geoff Brown [:gbrown] (pto, back Jun 10)]

Attachment: tar archive of memory dumps taken 1 minute apart while pac3 demo running -- ion disabled

Demo continued to run without incident -- ion disabled.

Comment 3

[Nicolas B. Pierron [:nbp]]

Thanks for the profiles.  It at first sight it seems that IonCode might hold references more than it should or more than what JM is doing.

I will try to experiment with benchmarks if I can spot a difference in the shell, first on x64, then on ARM if I don't find any on x64.  The architecture should not matter much for marking pointers contained in the IonCode.

Comment 4

[Nicolas B. Pierron [:nbp]]

I was comparing the *-3 profiles, memory profiles are not extremely useful as a time-based because GC might be triggered by Ion allocations / JM allocations and so we might obtain results which are not matching each others.

I noticed a big difference in *-3 reports, which is the heap-unclassified memory (~50 MB with JM / ~130 MB with Ion). This sounds a weird, and I would be interested if you use DMD[1] to report from where the heap-unclassified is coming from.

[1] https://wiki.mozilla.org/Performance/MemShrink/DMD

I am still investigating on my side, but I cannot reproduce anything similar yet.

Comment 5

[Nicolas B. Pierron [:nbp]]

Marty pointed to my attention that, only on ARM, we are allocating code buffers with a normal malloc instead of using a temporary allocation system that we use for everything else.  And apparently we are not using any free which is likely to cause some heap-unclassified.

Still I would not expect to see 70 extra MB of only caused by this leak unless shumway is doing a lot of compilations.

I will try this on a tegra board and make a test case for it, and hope this would fix this leak, but I have no clue whatever it will help or not for your heap-unclassified report.

** Apparently we have never run anything to report for leaks on ARM since fx18 (with Ion enabled) because we would have detected this leak.  We should definitely do so because this one is clearly a leak as the buffers are never freed.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Use IonAllocPolicy for ARM assembler buffers.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 719423 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 719423 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

https://tbpl.mozilla.org/?tree=Try&rev=150445f6977f

Testing with [1] on Android seems to have improve the lifetime of the browser, even if the browser is still crashing after a while (at least 30 minutes on a galaxy nexus)

[1] ftp://ftp.mozilla.org/pub/firefox/try-builds/[e-mail]-150445f6977f/try-android/fennec-22.0a1.en-US.android-arm.apk

Comment 9

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 719423 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

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

::: js/src/ion/IonCaches.cpp
@@ +714,5 @@
>  bool
>  GetPropertyIC::attachReadSlot(JSContext *cx, IonScript *ion, JSObject *obj, JSObject *holder,
>                                HandleShape shape)
>  {
> +    AutoIonContextAlloc aica(cx);

how does x86/x64 get away without these?

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +14,5 @@
>  namespace ion {
>  typedef Vector<BufferOffset, 512, IonAllocPolicy> LoadOffsets;
>  
> +struct Pool
> +  : public IonAllocPolicy

This doesn't require any constructor to be called?

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 719423 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

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

The AutoIonContextAllocs are created as part of instantiating a MacroAssembler, so they shouldn't be needed.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #10)
> The AutoIonContextAllocs are created as part of instantiating a
> MacroAssembler, so they shouldn't be needed.

I got SEGV because of a NULL temp allocator if I omit them. So I should just give a cx as argument to the MacroAssembler when we are lacking one, instead of adding these.

(In reply to Marty Rosenberg [:mjrosenb] from comment #9)
> ::: js/src/ion/IonCaches.cpp
> @@ +714,5 @@
> >  bool
> >  GetPropertyIC::attachReadSlot(JSContext *cx, IonScript *ion, JSObject *obj, JSObject *holder,
> >                                HandleShape shape)
> >  {
> > +    AutoIonContextAlloc aica(cx);
> 
> how does x86/x64 get away without these?

Some of the data structure inside it are using a SystemAllocPolicy. 

> ::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
> @@ +14,5 @@
> >  namespace ion {
> >  typedef Vector<BufferOffset, 512, IonAllocPolicy> LoadOffsets;
> >  
> > +struct Pool
> > +  : public IonAllocPolicy
> 
> This doesn't require any constructor to be called?

No, it just pick the current one by using the GetIonContext.

Comment 12

[Nicholas Nethercote [inactive]]

> Marty pointed to my attention that, only on ARM, we are allocating code
> buffers with a normal malloc instead of using a temporary allocation system
> that we use for everything else.  And apparently we are not using any free
> which is likely to cause some heap-unclassified.

That's bad;  don't hide memory allocations from the memory reporters.  And "not using free" also sounds like a leak.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: Use IonAllocPolicy for ARM assembler buffers.

As opposed to the previous patch, this patch use the AutoIonContextAlloc provided in the IonMacroAssembler, except that to be able to use it we have to delay the allocation after the construction of the IonMacroAssembler.

This should make the code cleaner from the user point of view, but it introduces 2 extra functions for allocating with the IonAllocPolicy.

Comment 14

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 720236 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

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

looks good to me.

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 720236 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb567ad72fe6

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 720236 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IonMonkey landing (12 sep)

User impact if declined: A leak at each Ion compilation (ARM)

Testing completed (on m-c, etc.): not yet, inbound only.

Risk to taking this patch (and alternatives if risky): low. This patch replace the allocator by a temp allocator which is bound to the stack liveness and it move some initialization code after we initialize the allocator.

String or UUID changes made by this patch: N/A

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 720236 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

Approving for Aurora given where we are in the cycle. For beta though, it doesn't seem like this is a worthwhile fix unless we think this points to a larger issue impacting other sites or we have a mobile shumway push coming up. Are either of those true?

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: Same thing … with operator new.

I saw it, but I forgot to patch it.  I will merge it with the previous one when I backport it.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb567ad72fe6

Comment 20

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 721002 [details] [diff] [review]
Same thing … with operator new.

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

also, a sanity check, valgrind doesn't complain about memory leaks with both patches?

Comment 21

[Nicolas B. Pierron [:nbp]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #20)
> Comment on attachment 721002 [details] [diff] [review]
> Same thing … with operator new.
> 
> Review of attachment 721002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> also, a sanity check, valgrind doesn't complain about memory leaks with both
> patches?

I don't know about valgrind, because my build has instructions which are not supported by valgrind.

On the other hand DMD no longer reports any leaks, and the memory usage is stable even after 3 hours on B2G.

Comment 22

[Till Schneidereit [:till]]

(In reply to Alex Keybl [:akeybl] from comment #17)
> Comment on attachment 720236 [details] [diff] [review]
> Use IonAllocPolicy for ARM assembler buffers.
> 
> Approving for Aurora given where we are in the cycle. For beta though, it
> doesn't seem like this is a worthwhile fix unless we think this points to a
> larger issue impacting other sites or we have a mobile shumway push coming
> up. Are either of those true?

The patch fixes a leak that occurs for all content with scripts that are Ion-compiled. Shumway just has a lot of those and thus is hit particularly bad. See comment 5 for an explanation of the leak.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

I'm going to track this so we can keep tabs on it if there end up being further issues.  Will approve for beta due to the leaks - can someone on this bug file a new bug to get ARM leak testing enabled again?

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 721002 [details] [diff] [review]
Same thing … with operator new.

Apparently there is a persistent unknown problem with this patch: https://tbpl.mozilla.org/?tree=Try&rev=4765b0372896

I will spawn another build on top of m-c instead of inbound and see if I can fix it.

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 721002 [details] [diff] [review]
Same thing … with operator new.

https://tbpl.mozilla.org/?tree=Try&rev=af1aa299b4ba

Checking again on top of master does not reproduce the same error.  I will see if I can reproduce a bug I noticed on b2g with Ion, and push the patch if I cannot reproduce it.

Comment 26

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edcb3f098cce

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/edcb3f098cce

Comment 28

[Nicolas B. Pierron [:nbp]]

(Aurora) https://hg.mozilla.org/releases/mozilla-aurora/rev/799e30e7e6f4 [bad a= flag, set it to lsblakk instead of akeybl]
(Beta) https://hg.mozilla.org/releases/mozilla-beta/rev/a5bef71d4e1d

Comment 29

[Nicolas B. Pierron [:nbp]]

Attachment: Fix compilation warning/error with gcc 4.7

This patch has been verified by mjronseb over IRC.

Comment 30

[Nicolas B. Pierron [:nbp]]

Comment on attachment 721973 [details] [diff] [review]
Fix compilation warning/error with gcc 4.7

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 843733
User impact if declined: gcc 4.7 compilation issues.
Testing completed (on m-c, etc.): in-progress.
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: N/A


https://hg.mozilla.org/integration/mozilla-inbound/rev/e60d44394c02

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e60d44394c02

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 721973 [details] [diff] [review]
Fix compilation warning/error with gcc 4.7

low risk, helps avoid gcc 4.7 compilation warnings due to the previous patch uplifted in this bug.