Andrew McCreight [:mccr8] Assignee
	Description • 10 years ago

Continued from bug 1034910.  This is just a variant of my patch in bug 1034611.  This probably isn't going to be enough for B2G, but it should at least help with desktop GGC stability.  I'm going to reduce the nursery size to 4MB, from 16MB.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 1 • 10 years ago

I suggest this be pref-controlled so that we can tweak for different devices.

	Andrew McCreight [:mccr8] Assignee
	Comment 2 • 10 years ago

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #1)
> I suggest this be pref-controlled so that we can tweak for different devices.

Yeah, definitely.  It may be a followup bug depending on whether I can quickly figure out how to get a mainthread pref over to the worker.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 10 years ago

try run: https://tbpl.mozilla.org/?tree=Try&rev=f136c8fe434e

This first patch is just a refactoring. It shouldn't alter the behavior. I split out the actual change in worker nursery size to a separate patch to make it easier to back out if something goes wrong.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 5 • 10 years ago

Comment on attachment 8454653 [details] [diff] [review]
part 1 - Hoist JS runtime creation out of CycleCollectedJSRuntime.

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

I would strongly prefer that we just added another parameter to the ctor.  Why do you want to do it this way?

	Andrew McCreight [:mccr8] Assignee
	Comment 6 • 10 years ago

Pulling JS_NewRuntime() out of the ctor will make it easier deal with it failing in a fallible way.  But I guess I could always bail early from the ctor in the case of failure, and then add some kind of method to check if the ctor succeeded.  So I can just add another parameter if you prefer it.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 7 • 10 years ago

Ah, so the goal is to sneak bug 1034611 in here too?

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 10 years ago

I was just moving some of my planned refactoring for that in here, not actually making it fallible yet.  But if you don't think that's the way to go for that, I can do this differently.

	Andrew McCreight [:mccr8] Assignee
	Comment 9 • 10 years ago

I'm perfectly happy to kick the can down the road on refactoring for fallibility. This just adds a new argument for the nursery size. The next patch will reduce the size.

	Andrew McCreight [:mccr8] Assignee
	Comment 11 • 10 years ago

Kyle: review ping.  I could also get Olli to review it if you are busy.  I assume you are okay with this approach because I basically just did what you suggested.

	Andrew McCreight [:mccr8] Assignee
	Comment 12 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40e39eea4621
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd748cd23efc

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/40e39eea4621
https://hg.mozilla.org/mozilla-central/rev/cd748cd23efc

	Andrew McCreight [:mccr8] Assignee
	Comment 14 • 10 years ago

It looks like this improved various TP5 memory stats by 8% to 18%, which I guess makes sense if there's a worker anywhere in there.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 15 • 10 years ago

There's the OS.File worker if nothing else.

Can you nominate this for approvals?

	Andrew McCreight [:mccr8] Assignee
	Comment 16 • 10 years ago

Yeah, I was going to wait a day or two in case there are any horrible regressions.

I'll needinfo myself so I don't forget.

	Nicholas Nethercote [inactive]
	Comment 17 • 10 years ago

> There's the OS.File worker if nothing else.

And SessionWorker.js. And PageThumbsWorker.js comes and goes.

	Andrew McCreight [:mccr8] Assignee
	Comment 18 • 10 years ago

Comment on attachment 8455475 [details] [diff] [review]
part 1 - Add nursery size as a parameter of CycleCollectedJSRuntime.

(this request is for part 1 and 2)

There are no crashes of at least one signature of bug 991845 yesterday, so that's something.

Approval Request Comment
[Feature/regressing bug #]: generational GC
[User impact if declined]: OOM crashes when creating workers, like in bug 991845 (top crash)
[Describe test coverage new/current, TBPL]: workers are heavily tested
[Risks and why]: This could hurt worker performance somewhat, but that's probably not a huge deal.  Part 2 is the only part that changes behavior, and it would be easy to back out.
[String/UUID change made/needed]: none

	Sylvestre Ledru [:Sylvestre]
	Comment 19 • 10 years ago

Comment on attachment 8455477 [details] [diff] [review]
part 2 - Reduce GGC nursery size to 1MB on workers.

cf comment #18

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/e646f8f8e027
https://hg.mozilla.org/releases/mozilla-aurora/rev/3cb4eb9b4d3a

https://hg.mozilla.org/releases/mozilla-beta/rev/84bf8e0aaecb
https://hg.mozilla.org/releases/mozilla-beta/rev/e61371be0f38

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/84bf8e0aaecb
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e61371be0f38