1259347 - Choose nursery size based on number of processes

Status: RESOLVED DUPLICATE of bug 1291292

(Core :: JavaScript: GC, defect)

Description

[Nicholas Nethercote [inactive]]

Currently the nursery size can grow to up to 8 MiB (I think?)  With multiple content processes this is a lot; it accounts for a sizeable fraction of the per-content-process overhead.

We should consider making the nursery size a function of the number of content processes. Maybe we should aim for all the nurseries to add up to roughly 8 MiB?

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Should worker runtimes eat a piece of this pie too?

Comment 2

[Jon Coppeard (:jonco)]

The DefaultNurseryBytes setting is 16MiB.  We should investigate how our performance changes as we reduce this.

Comment 3

[Nicholas Nethercote [inactive]]

> The DefaultNurseryBytes setting is 16MiB.

Thank you for the correction. Even more reason to adjust it :)

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1259347-nursery-size-telemetry

The nursery reserves 16MiB of address space but the amount that's actually used is adjusted dynamically depending on the workload.  We should add telemetry to see how much is used in practice.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8775985 [details] [diff] [review]
bug1259347-nursery-size-telemetry

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

I think we've removed most of the constraints that required the nursery allocation to be contiguous, so it should be fairly easy now to switch from pre-allocating the entire extent to allocating new chunks on the fly. This would allow us to keep an L3 sized nursery stealing all of the address space up front.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> Currently the nursery size can grow to up to 8 MiB (I think?)  With multiple
> content processes this is a lot; it accounts for a sizeable fraction of the
> per-content-process overhead.

We aggressively decommit the portion of the nursery that we are not using, so I'm skeptical that this is that big a deal: http://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#792

> We should consider making the nursery size a function of the number of
> content processes. Maybe we should aim for all the nurseries to add up to
> roughly 8 MiB?

This is a terrible idea. The nursery needs to be larger than the largest temporarily entrained working set size to be effective. Anything lower and you're going to spill temporary allocations into long-term storage and end up with fragmentation eating more space than you lost to the nursery, not to mention running slower as well. Any actually committed allocation in the nursery is probably helping more than it's hurting.

No, splitting the nursery's size among our processes is just going to bring the performance cliff closer for whatever actually needs the nursery -- games, facebook, google docs, etc -- without buying us any actual wins.

That said, there is still the address space issue. We've had the dynamic nursery size on our todo list forever and we can certainly prioritize that.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Terrence Cole [:terrence] from comment #6)
> We aggressively decommit the portion of the nursery that we are not using,
> so I'm skeptical that this is that big a deal:
> http://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#792

So the problem is that doesn't do what most people (including myself) assume it does, mainly it *doesn't* decommit on Windows. We end up doing a VirtualAlloc(MEM_RESET), which according to msdn [1] explicitly is not decommitted:

> "However, the memory block will be used again later, so it should not be decommitted."

IIUC basically we're saying, don't back this in the pagefile we don't care about the contents anymore, but at some point we're going to probably use it again.

> > We should consider making the nursery size a function of the number of
> > content processes. Maybe we should aim for all the nurseries to add up to
> > roughly 8 MiB?
> 
> This is a terrible idea. The nursery needs to be larger than the largest
> temporarily entrained working set size to be effective. Anything lower and
> you're going to spill temporary allocations into long-term storage and end
> up with fragmentation eating more space than you lost to the nursery, not to
> mention running slower as well. Any actually committed allocation in the
> nursery is probably helping more than it's hurting.
> 
> No, splitting the nursery's size among our processes is just going to bring
> the performance cliff closer for whatever actually needs the nursery --
> games, facebook, google docs, etc -- without buying us any actual wins.

I do think there's merit in potentially reducing the nursery size from 16MiB when we switch to multiple content processes. But lets say we end up going process-per-tab and I have 30 tabs open, then, yes, naively 16/30 MiB per process seems like a bad idea. On the other hand 16*30 MiB is also a bad thing.

> That said, there is still the address space issue. We've had the dynamic
> nursery size on our todo list forever and we can certainly prioritize that.

That sounds great, do we have a bug for it?

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Eric Rahm [:erahm] from comment #7)
> So the problem is that doesn't do what most people (including myself) assume
> it does, mainly it *doesn't* decommit on Windows. We end up doing a
> VirtualAlloc(MEM_RESET), which according to msdn [1] explicitly is not
> decommitted:

Good point. IIRC actually decommitting was too slow for our page-sized arenas (though that was before decommitting became its own GC phase), but we should consider actually decommitting the parts of the nursery we aren't using (since this is done in ChunkSized increments).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/350ea93a27b4
Add telemetry for nursery size r=terrence

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/350ea93a27b4

Comment 11

[Jon Coppeard (:jonco)]

Attachment: bug1259347-fix-telemetry

Annoyingly I made the telemetry report the maximum nursery size rather than the current active size as intended.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b5857bcc12
Fix nursery telemetry to report committed size rather than max size r=terrence

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88b5857bcc12

Comment 14

[Chris H-C :chutten]

Hey, look, it works! https://mzl.la/2b9FvOD

(( This showed up on telemetry alerts when the distribution changed wildly. Glad that it's explainable! ))

Comment 15

[Jon Coppeard (:jonco)]

I'm not specifically working on this (and I think it's unclear whether we actually want to do this) so unassigning myself.

Comment 16

[Terrence Cole [:terrence]]

Let's fix decommit in a separate bug.

Comment 17

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.