Web workers set their maximum nursery size to a value lower than the default maximum nursery size. In some testing (with the JS shell) I found no difference. Note that this is the maximum size the nursery is allowed to grow to, not the current or initial nursery size. There's a good chance setting it specifically to a lower value has no impact at all, and we can remove this code.

I'll do more testing in the browser also.

Note that this basically reverts Bug 1037510. However that was landed before the nursery was good at using less memory / discarding its unused chunks properly and growing lazily.

I'd think carefully about doing this at a time when we are trying to reduce memory usage for fission. This would increase the possible max memory usage by 15MB, per worker, per process.

Do we have telemetry for in-the-wild Worker usage? (I know Paul said he did some testing in the shell but I'm curious how much memory Service Workers use, for example). I'm tentatively marking this as P3 but we can adjust as necessary.

It's important to remember that this is the maximum bound of how much memory may be used.

I've started some raptor, talos and awsy jobs to test it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d4f5f450d434da3cbc3bd0ff3c189a6991046b

I don't see any telemetry for workers, or I didn't recognise it. I agree, it'd be nice to know before jumping in. Alternatively we can monitor the change in memory usage after the patch lands. OTOH if we don't have to increase this then why bother?

I have the feeling that the only reason people set the parameter to something other than the default is that is a parameter passed to JS_NewContext() rather than one of the many parameters settable with JS_SetGCParameter().

Depends on D47870

This patch updates this for workers and worklets.

Depends on D47871

Depends on D47872

(In reply to Paul Bone [:pbone] from comment #4)

I've started some raptor, talos and awsy jobs to test it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d4f5f450d434da3cbc3bd0ff3c189a6991046b

Do those tests use workers?

I have the feeling that the only reason people set the parameter to something other than the default is that is a parameter passed to JS_NewContext() rather than one of the many parameters settable with JS_SetGCParameter().

No, this was deliberate (see bug 1037510 or bug 1034611 comment 1).

Whether it's still applicable is another matter. Have you thought about how you might detect problems this change might cause?

(In reply to Jon Coppeard (:jonco) from comment #8)

(In reply to Paul Bone [:pbone] from comment #4)

I've started some raptor, talos and awsy jobs to test it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d4f5f450d434da3cbc3bd0ff3c189a6991046b

Do those tests use workers?

I hope so. These are all the performance and AWSY tests I know about. If they don't test workers then that's a pretty big omission.

I have the feeling that the only reason people set the parameter to something other than the default is that is a parameter passed to JS_NewContext() rather than one of the many parameters settable with JS_SetGCParameter().

No, this was deliberate (see bug 1037510 or bug 1034611 comment 1).

Whether it's still applicable is another matter. Have you thought about how you might detect problems this change might cause?

In an extreme case we might see OOMs go up, but I doubt it. There are nursery size telemetry probes.. Oh but I'm not sure they work for workers.

Overholt had a good point that this has extra risk when we're specifically trying to drive down memory usage. I've decoupled it from Bug 1530251 (which was why I looked at it, to tidy up code related to that) so now it doesn't have to land. I want to defer to someone who knows about workers specifically whether we have telemetry on them and/or what tests exercise them. Baku what do you think? If you think this is a bad idea then then I don't want to land it and won't mind if you close the bug.

Or hsinyi might know.

asuth for sure knows better than I. :D

Here's the AWSY results:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=89cac0c4d6f56ece28e41f5922396c93aab91ad6&newProject=try&newRevision=99d4f5f450d434da3cbc3bd0ff3c189a6991046b&framework=4

No significant difference.

We don't have nursery telemetry for workers or any other meaningful telemetry beyond when worker creation is deferred due to per-domain worker limits. Only the XPConnect XPCJSRuntime has JS telemetry hooks[1].

The platform makes some use of workers internally (specifically the still-JS based sessionstore) so there should be some coverage at minimum.

Given that this is altering the max, this seems reasonable to make workers behave more consistently with window globals.

Clearing baku's needinfo because baku is trying to get out of the workers game and already r+'d the patch.

1: https://searchfox.org/mozilla-central/rev/b6f088f2f68d2a8ae0b49d6c8fbd7cbd3a65fa5b/js/xpconnect/src/XPCJSRuntime.cpp#3081 hooks up
https://searchfox.org/mozilla-central/rev/b6f088f2f68d2a8ae0b49d6c8fbd7cbd3a65fa5b/js/xpconnect/src/XPCJSRuntime.cpp#2658

Thanks Andrew.

The benefit of this change is a fairly minor cleanup, but the risk is a regression we won't be able to monitor properly. I'm going to hold off on landing the patches for now.

Hi :pbone, can we now take a decision here? Just to not leave this open with an (aging) patch attached. Thank you!

Yeah, I want to land this. We can watch general telemetry to see if it increases memory usage (but I doubt it will) and I would like these things to be consistent, it'll make it easier to make future changes to how the nursery is configured.

Jonco, could you review the last patch?

https://hg.mozilla.org/mozilla-central/rev/7c86675ec7b2
https://hg.mozilla.org/mozilla-central/rev/f0e49f75b1db
https://hg.mozilla.org/mozilla-central/rev/d89e641acc9d