Nicholas Nethercote [inactive] Reporter
	Description • 8 years ago

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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 1 • 8 years ago

Should worker runtimes eat a piece of this pie too?

	Jon Coppeard (:jonco)
	Comment 2 • 8 years ago

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

	Nicholas Nethercote [inactive] Reporter
	Comment 3 • 8 years ago

> The DefaultNurseryBytes setting is 16MiB.

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

	Jon Coppeard (:jonco)
	Comment 4 • 8 years ago

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.

	Terrence Cole [:terrence]
	Comment 5 • 8 years ago

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.

	Terrence Cole [:terrence]
	Comment 6 • 8 years ago

(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.

	Eric Rahm [:erahm]
	Comment 7 • 8 years ago

(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

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 8 • 8 years ago

(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).

	Pulsebot
	Comment 9 • 8 years ago

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/350ea93a27b4
Add telemetry for nursery size r=terrence

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago
bugherder

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

	Jon Coppeard (:jonco)
	Comment 11 • 8 years ago

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

	Pulsebot
	Comment 12 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 8 years ago
bugherder

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

	Chris H-C :chutten
	Comment 14 • 8 years ago

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

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

	Jon Coppeard (:jonco)
	Comment 15 • 8 years ago

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

	Terrence Cole [:terrence]
	Comment 16 • 8 years ago

Let's fix decommit in a separate bug.

	Firefox Bug Husbandry Bot
	Comment 17 • 6 years ago

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