Closed Bug 1258257 Opened 8 years ago Closed 8 years ago

Reduce mozjemalloc's page cache size

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(1 file)

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.
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: nobody → n.nethercote
Status: NEW → ASSIGNED
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)
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)
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+
> 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 :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8060bad4e3e0f3ba6afd3dabe5b0950ccea6fb57
Bug 1258257 - Reduce mozjemalloc page cache size from 4 MiB to 1 MiB. r=glandium.
https://hg.mozilla.org/mozilla-central/rev/8060bad4e3e0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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?
> [User impact if declined]: This page saves up to 3 MiB of physical memory
> usage per process.

Sorry, that should say "This *patch* saves..."
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)
(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)
(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.
You need to log in before you can comment on or make changes to this bug.