Last Comment Bug 843733 - With Ion enabled on Android, shumway demo keeps using up memory and eventually dies
: With Ion enabled on Android, shumway demo keeps using up memory and eventuall...
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Android
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
: 842838 (view as bug list)
Depends on:
Blocks: 849103
  Show dependency treegraph
 
Reported: 2013-02-21 11:38 PST by Geoff Brown [:gbrown]
Modified: 2013-04-04 16:32 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
fixed
unaffected
unaffected
unaffected
unaffected


Attachments
tar archive of memory dumps taken 1 minute apart while pac3 demo running -- ion enabled (140.00 KB, application/x-tar)
2013-02-21 20:29 PST, Geoff Brown [:gbrown]
no flags Details
tar archive of memory dumps taken 1 minute apart while pac3 demo running -- ion disabled (190.00 KB, application/x-tar)
2013-02-21 20:30 PST, Geoff Brown [:gbrown]
no flags Details
Use IonAllocPolicy for ARM assembler buffers. (20.76 KB, patch)
2013-02-28 03:36 PST, Nicolas B. Pierron [:nbp]
marty.rosenberg: review+
Details | Diff | Splinter Review
Use IonAllocPolicy for ARM assembler buffers. (23.70 KB, patch)
2013-03-01 23:05 PST, Nicolas B. Pierron [:nbp]
marty.rosenberg: review+
dvander: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release-
Details | Diff | Splinter Review
Same thing … with operator new. (1.54 KB, patch)
2013-03-04 17:26 PST, Nicolas B. Pierron [:nbp]
marty.rosenberg: review+
Details | Diff | Splinter Review
Fix compilation warning/error with gcc 4.7 (1.55 KB, patch)
2013-03-06 18:29 PST, Nicolas B. Pierron [:nbp]
nicolas.b.pierron: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Geoff Brown [:gbrown] 2013-02-21 11:38:37 PST
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] 2013-02-21 20:29:17 PST
Created attachment 716933 [details]
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] 2013-02-21 20:30:46 PST
Created attachment 716935 [details]
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] 2013-02-26 22:32:21 PST
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] 2013-02-27 17:38:55 PST
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] 2013-02-27 19:02:20 PST
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] 2013-02-28 03:36:10 PST
Created attachment 719423 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.
Comment 7 Nicolas B. Pierron [:nbp] 2013-02-28 11:19:31 PST
Comment on attachment 719423 [details] [diff] [review]
Use IonAllocPolicy for ARM assembler buffers.

https://tbpl.mozilla.org/?tree=Try&pusher=npierron@mozilla.com
Comment 8 Nicolas B. Pierron [:nbp] 2013-02-28 13:49:22 PST
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/npierron@mozilla.com-150445f6977f/try-android/fennec-22.0a1.en-US.android-arm.apk
Comment 9 Marty Rosenberg [:mjrosenb] 2013-02-28 17:11:22 PST
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] 2013-02-28 17:19:25 PST
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] 2013-02-28 18:26:12 PST
(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 [:njn] 2013-03-01 01:29:30 PST
> 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] 2013-03-01 23:05:47 PST
Created attachment 720236 [details] [diff] [review]
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] 2013-03-04 12:17:49 PST
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] 2013-03-04 12:56:31 PST
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] 2013-03-04 15:27:08 PST
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] 2013-03-04 15:37:11 PST
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] 2013-03-04 17:26:44 PST
Created attachment 721002 [details] [diff] [review]
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] 2013-03-04 18:07:22 PST
https://hg.mozilla.org/mozilla-central/rev/fb567ad72fe6
Comment 20 Marty Rosenberg [:mjrosenb] 2013-03-04 22:02:47 PST
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] 2013-03-04 23:14:25 PST
(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] (ECOOP July 18 - July 22, pto July 23 - July 31) 2013-03-05 05:12:52 PST
(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 2013-03-05 09:15:58 PST
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] 2013-03-05 11:07:02 PST
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] 2013-03-05 14:10:32 PST
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] 2013-03-05 15:37:57 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/edcb3f098cce
Comment 27 Ed Morley [:emorley] 2013-03-06 08:34:29 PST
https://hg.mozilla.org/mozilla-central/rev/edcb3f098cce
Comment 28 Nicolas B. Pierron [:nbp] 2013-03-06 10:55:37 PST
(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] 2013-03-06 18:29:25 PST
Created attachment 721973 [details] [diff] [review]
Fix compilation warning/error with gcc 4.7

This patch has been verified by mjronseb over IRC.
Comment 30 Nicolas B. Pierron [:nbp] 2013-03-06 23:09:15 PST
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] 2013-03-07 09:50:00 PST
https://hg.mozilla.org/mozilla-central/rev/e60d44394c02
Comment 32 bhavana bajaj [:bajaj] 2013-03-08 11:43:24 PST
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.
Comment 33 Nicholas Nethercote [:njn] 2013-04-04 16:32:46 PDT
*** Bug 842838 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.