Reduce mozjemalloc's page cache size

RESOLVED FIXED in Firefox 47

Status

()

Core
Memory Allocator
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
mozjemalloc has a page cache of dirty pages that it holds onto for recycling. Its size is controlled by DIRTY_MAX_DEFAULT, which is currently 1024, giving a limit of 4 MiB.

Every process has its own page cache, and as we increase the number of processes it makes sense to reduce the page cache size. Hopefully this won't hurt performance.

jemalloc4 doesn't have the DIRTY_MAX_DEFAULT constant, and I'm not sure if it has a page cache or something similar.
(Assignee)

Comment 1

2 years ago
Created attachment 8732692 [details] [diff] [review]
Reduce mozjemalloc page cache size from 4 MiB to 1 MiB

I confirmed that the "explicit/heap-overhead/page-cache" number dropped
accordingly. (It doesn't always reach the limit, but if you measure a number of
times it'll often gets close.)

I haven't done any performance testing yet. Is Talos the best way to do it?
How should I invoke it? Any tips on interpreting the results? Thank you.
Attachment #8732692 - Flags: review?(mh+mozilla)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Avi, glandium suggested that you would know the best way to measure the performance impact of this change via Talos. Any suggestions? Thank you.
Flags: needinfo?(avihpit)

Comment 3

2 years ago
The most obvious approach would be to do two try pushes for all (desktop?) platforms with all talos tests enabled, where one of the pushes has the old behavior/setup and the other having the new/suggested setup, then compare the two revisions using perfherder and see if anything interesting shows up.
Flags: needinfo?(avihpit)
(Assignee)

Comment 4

2 years ago
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=237a09b0cdd8&newProject=try&newRevision=604efd6c966e&framework=1&showOnlyImportant=0

- "tp5o Main_RSS opt" improved by 2% for e10s and non-e10s, but only on Windows. Not sure why it wouldn't show up on Mac and Linux.

- "tart opt e10s" worsened by 2% for e10s.

- Nothing else significant is showing up.

I'm inclined to land this.
Comment on attachment 8732692 [details] [diff] [review]
Reduce mozjemalloc page cache size from 4 MiB to 1 MiB

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

Fair enough, since it doesn't seem to impact performance. OTOH, considering we only have one arena per process, it's not going to make miracles.

Note that jemalloc4 doesn't work the same way. See opt.lg_dirty_mult in its documentation.
Attachment #8732692 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 6

2 years ago
> it's not going to make miracles.

It saves up to 3 MiB per process. In other bugs I'm sweating to save 50--150 KiB of static data per process, so in comparison this is a big win :)
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8060bad4e3e0f3ba6afd3dabe5b0950ccea6fb57
Bug 1258257 - Reduce mozjemalloc page cache size from 4 MiB to 1 MiB. r=glandium.

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8060bad4e3e0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 9

2 years ago
Comment on attachment 8732692 [details] [diff] [review]
Reduce mozjemalloc page cache size from 4 MiB to 1 MiB

Approval Request Comment

[Feature/regressing bug #]: N/A

[User impact if declined]: This page saves up to 3 MiB of physical memory usage per process. The exact amount varies but is usually in the 2--3 MiB range. So for e10s with one content process it usually saves 4--6 MiB. e10s memory usage is a concern and this is the biggest easy win I know of.

[Describe test coverage new/current, TreeHerder]:

[Risks and why]: The patch is trivial -- it just changes one constant. The only risk is that it might hurt performance, but my Talos run showed no regressions and I haven't heard any complaints (automated or human) since it landed on m-c five days ago.
https://treeherder.mozilla.org/perf.html#/alerts?status=0&framework=1 is clear.

[String/UUID change made/needed]: None.
Attachment #8732692 - Flags: approval-mozilla-beta?
Attachment #8732692 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

2 years ago
> [User impact if declined]: This page saves up to 3 MiB of physical memory
> usage per process.

Sorry, that should say "This *patch* saves..."

Updated

2 years ago
status-firefox47: --- → affected
Hi Jim, I am planning to take this uplift in Aurora 47 given the mem improvements in e10s mode. Do you think a week of bake time in Nightly is enough for this? Any other concerns? Just thought of checking with you before rubberstamping this one.
Flags: needinfo?(jmathies)

Comment 12

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #11)
> Hi Jim, I am planning to take this uplift in Aurora 47 given the mem
> improvements in e10s mode. Do you think a week of bake time in Nightly is
> enough for this? Any other concerns? Just thought of checking with you
> before rubberstamping this one.

This looks safe to uplift.
Flags: needinfo?(jmathies)

Comment 13

2 years ago
(In reply to Jim Mathies [:jimm] from comment #12)
> (In reply to Ritu Kothari (:ritu) from comment #11)
> > Hi Jim, I am planning to take this uplift in Aurora 47 given the mem
> > improvements in e10s mode. Do you think a week of bake time in Nightly is
> > enough for this? Any other concerns? Just thought of checking with you
> > before rubberstamping this one.
> 
> This looks safe to uplift.

to aurora, skip beta.
It sounds like this would help with memory use when e10s is disabled so I am inclined to take it for beta 8.
Comment on attachment 8732692 [details] [diff] [review]
Reduce mozjemalloc page cache size from 4 MiB to 1 MiB

Let's take this for aurora so we can look for the e10s related perf improvements in beta 47 experiments. But let's hold off for 46 since we are very late in beta.
Attachment #8732692 - Flags: approval-mozilla-beta?
Attachment #8732692 - Flags: approval-mozilla-beta-
Attachment #8732692 - Flags: approval-mozilla-aurora?
Attachment #8732692 - Flags: approval-mozilla-aurora+
(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #12)
> > (In reply to Ritu Kothari (:ritu) from comment #11)
> > > Hi Jim, I am planning to take this uplift in Aurora 47 given the mem
> > > improvements in e10s mode. Do you think a week of bake time in Nightly is
> > > enough for this? Any other concerns? Just thought of checking with you
> > > before rubberstamping this one.
> > 
> > This looks safe to uplift.
> 
> to aurora, skip beta.

Thanks Jim! Wontfix for 46 then.
status-firefox46: --- → wontfix

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a425d7a0ddb7
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.