Last Comment Bug 804891 - Shrink memory usage by reduce default chunk size for analysis-temporary (LifoAlloc).
: Shrink memory usage by reduce default chunk size for analysis-temporary (Lifo...
Status: RESOLVED FIXED
[MemShrink]
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: nnethercote
:
Mentors:
Depends on:
Blocks: 764220 801799 802446 805219
  Show dependency treegraph
 
Reported: 2012-10-23 20:18 PDT by Thinker Li [:sinker]
Modified: 2012-11-14 02:15 PST (History)
12 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
about:memory after change default chunk size (253.96 KB, text/plain)
2012-10-23 20:23 PDT, Thinker Li [:sinker]
no flags Details
about:memory after change default chunk size (176.79 KB, text/plain)
2012-10-23 20:28 PDT, Thinker Li [:sinker]
no flags Details
patch (904 bytes, patch)
2012-10-24 14:47 PDT, Nicholas Nethercote [:njn]
wmccloskey: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Thinker Li [:sinker] 2012-10-23 20:18:40 PDT
The default value of LIFO_ALLOC_PRIMARY_CHUNK_SIZE is too big (128K) that almost all compartments do not use it up.  It wastes a lot of memory (~10%).  By cutting down LIFO_ALLOC_PRIMARY_CHUNK_SIZE for compartments, it can save a lot of memory.  By setting LIFO_ALLOC_PRIMARY_CHUNK_SIZE to 32K, the memory size for analysis-temporary comes down to 1.8MB from original ~5MB.
Comment 1 Andrew McCreight [:mccr8] 2012-10-23 20:22:59 PDT
The tradeoff here is that smaller chunks cause longer GC pauses, but maybe that is only a real problem on OSX.  Pretty amazing that it is using up that much memory though!
Comment 2 Thinker Li [:sinker] 2012-10-23 20:23:34 PDT
Created attachment 674518 [details]
about:memory after change default chunk size
Comment 3 Thinker Li [:sinker] 2012-10-23 20:28:33 PDT
Created attachment 674519 [details]
about:memory after change default chunk size
Comment 4 Thinker Li [:sinker] 2012-10-23 20:36:13 PDT
75% of compartments do not use more than 32K for analysis-temporary.
Comment 5 Nicholas Nethercote [:njn] 2012-10-23 21:34:31 PDT
Good catch, Thinker!

The size was set to 128 KiB in bug 699668 after lots of discussion.  However, that was before compartment-per-global.  (Indeed, see the last sentence in bug 699668 comment 26.)  32 KiB seems a good starting point for re-evaluation.

billm, are you able to do some experiments like the ones you did in bug 699668 comment 25 to determine if 32 KiB is reasonable, and whether 16 KiB would be ok?
Comment 6 Andrew McCreight [:mccr8] 2012-10-23 21:42:30 PDT
We may incrementally discard these structures now, come to think of it.
Comment 7 Bill McCloskey (:billm) 2012-10-23 22:09:45 PDT
Yes, we free this stuff on the background thread now. So I think 16K should be fine.
Comment 8 Nicholas Nethercote [:njn] 2012-10-23 22:40:51 PDT
> Yes, we free this stuff on the background thread now. So I think 16K should
> be fine.

Great!  Do you want to do experiments before landing a change?
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-23 23:00:38 PDT
Nice! :)
Comment 10 Nicholas Nethercote [:njn] 2012-10-24 14:44:27 PDT
I talked to billm.  The original problem with all this was that free() is really slow on Mac.  But because of the background freeing he thinks it shouldn't be a problem anymore.

Nonetheless, he'd like to land a patch for this on mozilla-central, and then wait a week to see what the telemetry data looks like.  Assuming it doesn't cause problems, we can then backport it to Aurora.
Comment 11 Nicholas Nethercote [:njn] 2012-10-24 14:45:44 PDT
I compared 128, 32 and 16 KiB arenas on Mac64 desktop.  My process:

1. start browser, open about:memory, get measurement
2. hit "minimize memory usage", get measurement
3. open techcrunch.com in a second tab, hit "update" in about:memory, get
   measurement
4. hit "minimize memory usage", get measurement


Results:

128 KiB arenas                              range
--------------                              -----
1.  8.5     6.6     19.6    19.6    19.6    6.6--19.6
2.  0.4     0.4      0.4     0.4     0.4    0.4
3.  3.4     3.4     10.4    15.2     2.2    2.2--15.2
4.  2.0     1.9      1.8     1.8     2.3    1.8-- 2.3

32 KiB arenas                               range
-------------                               -----
1.  4.8     4.0     11.4    11.5    11.5    4.0--11.5
2.  0.2     0.2      0.2     0.2     0.2    0.2
3.  1.2     1.4      4.8     4.4     4.5    1.2--4.8
4.  0.6     0.6      0.6     0.7     0.5    0.5--0.7

16 KiB arenas                               range
-------------                               -----
1.  4.3     3.8     10.7    10.7    10.7    3.8--10.7
2.  0.2     0.2      0.2     0.2     0.2    0.2
3.  3.7     0.9      4.7     1.8     2.4    0.9--4.7
4.  0.4     0.4      0.4     0.5     0.4    0.4-- 0.5


The improvement between 32 and 16 is small enough that I suggest we change it to 32, to minimize the chance of GC time regressions.
Comment 12 Nicholas Nethercote [:njn] 2012-10-24 14:47:44 PDT
Created attachment 674842 [details] [diff] [review]
patch

Shrink it from 128 KiB to 32 KiB.
Comment 13 Nicholas Nethercote [:njn] 2012-10-24 14:48:49 PDT
> 128 KiB arenas                              range
> --------------                              -----
> 1.  8.5     6.6     19.6    19.6    19.6    6.6--19.6
> 2.  0.4     0.4      0.4     0.4     0.4    0.4
> 3.  3.4     3.4     10.4    15.2     2.2    2.2--15.2
> 4.  2.0     1.9      1.8     1.8     2.3    1.8-- 2.3

I failed to mention that measurements 1 and 3 are volatile, which is why I did all the measurements five times.
Comment 14 Nicholas Nethercote [:njn] 2012-10-25 17:58:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4380bab9db04
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-26 04:35:45 PDT
https://hg.mozilla.org/mozilla-central/rev/4380bab9db04
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 11:18:48 PDT
\o/
Comment 17 Justin Lebar (not reading bugmail) 2012-10-26 11:20:00 PDT
Setting flags so we hopefully don't forget to uplift this one.
Comment 18 Nicholas Nethercote [:njn] 2012-10-31 22:55:28 PDT
billm, can you look at the telemetry to see if any bad effects have occurred?  Thanks.
Comment 19 Bill McCloskey (:billm) 2012-11-01 08:29:52 PDT
It looks good to me. I think it's ready for aurora.
Comment 20 Andrew McCreight [:mccr8] 2012-11-01 16:45:48 PDT
This needs either blocking-basecamp or aurora approval, as far as I understand.
Comment 21 Nicholas Nethercote [:njn] 2012-11-01 16:57:11 PDT
Comment on attachment 674842 [details] [diff] [review]
patch

[Approval Request Comment]

Bug caused by (feature/regressing bug #): N/A.

User impact if declined: higher memory usage, esp. on B2G.

Testing completed (on m-c, etc.):  has been on m-c for several days without problem;  billm has checked telemetry data to confirm that an unlikely-but-possible GC time regression didn't occur.

Risk to taking this patch (and alternatives if risky): negligible.  The patch changes a single constant that controls the size of chunks of memory used for type inference analysis.

String or UUID changes made by this patch:  none.
Comment 22 Alex Keybl [:akeybl] 2012-11-02 15:37:28 PDT
Comment on attachment 674842 [details] [diff] [review]
patch

[Triage Comment]
Thanks for looking into whether or not this caused a new performance regression for desktop. Given that analysis, and the risk evaluation here, approving for Aurora 18.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-11-03 10:16:49 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/86b9ecee110f
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-11-03 16:09:22 PDT
This was backed out while investigating Android timeouts and re-landed after it was deemed not at fault.
https://hg.mozilla.org/releases/mozilla-aurora/rev/50cee6bb0eea

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