804891 - Shrink memory usage by reduce default chunk size for analysis-temporary (LifoAlloc).

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thinker Li [:sinker]]

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]]

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]]

Attachment: about:memory after change default chunk size

Comment 3

[Thinker Li [:sinker]]

Attachment: about:memory after change default chunk size

Comment 4

[Thinker Li [:sinker]]

75% of compartments do not use more than 32K for analysis-temporary.

Comment 5

[Nicholas Nethercote [inactive]]

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]]

We may incrementally discard these structures now, come to think of it.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, we free this stuff on the background thread now. So I think 16K should be fine.

Comment 8

[Nicholas Nethercote [inactive]]

> 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]

Nice! :)

Comment 10

[Nicholas Nethercote [inactive]]

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 [inactive]]

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 [inactive]]

Attachment: patch

Shrink it from 128 KiB to 32 KiB.

Comment 13

[Nicholas Nethercote [inactive]]

> 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 [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4380bab9db04

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4380bab9db04

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

\o/

Comment 17

[Justin Lebar (not reading bugmail)]

Setting flags so we hopefully don't forget to uplift this one.

Comment 18

[Nicholas Nethercote [inactive]]

billm, can you look at the telemetry to see if any bad effects have occurred?  Thanks.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks good to me. I think it's ready for aurora.

Comment 20

[Andrew McCreight [:mccr8]]

This needs either blocking-basecamp or aurora approval, as far as I understand.

Comment 21

[Nicholas Nethercote [inactive]]

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]]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/86b9ecee110f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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