The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: sinker, Assigned: nnethercote)

Tracking

unspecified
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
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!
(Reporter)

Comment 2

5 years ago
Created attachment 674518 [details]
about:memory after change default chunk size
Whiteboard: [MemShrink]
(Reporter)

Comment 3

5 years ago
Created attachment 674519 [details]
about:memory after change default chunk size
Attachment #674518 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
75% of compartments do not use more than 32K for analysis-temporary.
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?
Depends on: 764220
Blocks: 764220
No longer depends on: 764220
We may incrementally discard these structures now, come to think of it.
Yes, we free this stuff on the background thread now. So I think 16K should be fine.
Blocks: 801799
> 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?
Nice! :)
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.
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.
Created attachment 674842 [details] [diff] [review]
patch

Shrink it from 128 KiB to 32 KiB.
Attachment #674842 - Flags: review?(wmccloskey)
> 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.
Blocks: 805219
Attachment #674842 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4380bab9db04
https://hg.mozilla.org/mozilla-central/rev/4380bab9db04
Assignee: nobody → nnethercote
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
\o/
Setting flags so we hopefully don't forget to uplift this one.
status-firefox18: --- → affected
Whiteboard: [MemShrink] → [MemShrink][waiting-for-telemetry-before-uplifting-to-aurora]
billm, can you look at the telemetry to see if any bad effects have occurred?  Thanks.
It looks good to me. I think it's ready for aurora.
Whiteboard: [MemShrink][waiting-for-telemetry-before-uplifting-to-aurora] → [MemShrink][aurora-checkin-needed]
This needs either blocking-basecamp or aurora approval, as far as I understand.
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.
Attachment #674842 - Flags: approval-mozilla-aurora?
Whiteboard: [MemShrink][aurora-checkin-needed] → [MemShrink]
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.
Attachment #674842 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/86b9ecee110f
status-firefox18: affected → fixed
status-firefox19: --- → fixed
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
You need to log in before you can comment on or make changes to this bug.