957723 - Reduce the default GGC nursery size

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

Spun off from bug 956501.  It sounds like the default GGC nursery size is 16MB.  That should be reduced for b2g.

Comment 1

[Nicolas B. Pierron [:nbp]]

I think that before addressing this issue, we should ensure that AWFY is constantly running, and make sure we can run the static analysis in the first place.

Comment 2

[Terrence Cole [:terrence]]

Attachment: unmap_nursery_before_use-v0.diff

I'm not sure why we'd want to wait; this is actually pretty orthogonal. I've had this patch sitting in my queue forever, just hadn't gotten around to creating a bug for it.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8357378 [details] [diff] [review]
unmap_nursery_before_use-v0.diff

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

This is simpler than I expected - looks good!

Comment 4

[Nicholas Nethercote [inactive]]

terrence, can you explain the effect of this patch?

Comment 5

[Nicholas Nethercote [inactive]]

BTW, https://bugzilla.mozilla.org/show_bug.cgi?id=961883#c11 explains how the memory reporters should be updated for this change.

Comment 6

[Terrence Cole [:terrence]]

Attachment: unmap_nursery_before_use-v1.diff

(In reply to Nicholas Nethercote [:njn] from comment #4)
> terrence, can you explain the effect of this patch?

When the currently running program has a small working set size -- e.g. most objects die quickly -- we want to sweep and reuse the nursery as quickly as possible so that the processor's cache is being well utilized and not clogged with dead objects. On the other hand, if the working set size is large, we want to delay collection as long as possible so that we don't end up tenuring lots of almost-dead objects. We do the above by growing and shrinking the nursery's size at runtime as we observe the working set size change. This is implemented by adjusting a pointer to the virtual end of the nursery within the full 16MiB allocation.

The attached patch adds calls to MarkPagesUnused on the unused portion of of the nursery's allocation when we adjust the working set size.

Jon, I'm asking for re-review since I changed the way this works a bit. The simple way ends up making a syscall quite a bit more frequently than we need. I think a better way is to only decommit when we actually shrink and just eat a bit more complexity in the management of currentStart_.

Comment 7

[Nicholas Nethercote [inactive]]

Ok... do you have any measurements? It's not clear how that policy works in practice, and its effect on memory consumption.

BTW, you haven't update the memory reporter for the nursery.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8363851 [details] [diff] [review]
unmap_nursery_before_use-v1.diff

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

So with we only decommit when we shrink the nursery and on initialization.  Looks good to me.

Comment 9

[Terrence Cole [:terrence]]

Attachment: decommit_nursery-v2.diff

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Ok... do you have any measurements? It's not clear how that policy works in
> practice, and its effect on memory consumption.
> 
> BTW, you haven't update the memory reporter for the nursery.

You hadn't landed your rewrite yet. Do the memory reporter bits here look okay to you?

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8390681 [details] [diff] [review]
decommit_nursery-v2.diff

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

I've nit-picked the descriptions, but otherwise looks fine. Thanks.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2347,1 @@
>          "The GC nursery.");

Update the description to indicate that this is the committed part?

@@ +2390,5 @@
>  
> +    REPORT_GC_BYTES(rtPath2 + NS_LITERAL_CSTRING("runtime/gc/nursery-decommitted"),
> +        rtStats.runtime.gc.nurseryDecommitted,
> +        "Reserved address space for the GC's nursery that does not use any "
> +        "physical memory or swap space.");

Make this description more like the one above it, e.g. "Parts of the GC nursery that are decommitted, i.e. that up address space but no physical memory or swap space."

Comment 11

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7130b21a68

Comment 12

[Terrence Cole [:terrence]]

And backed out in: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4fc0dad4bd

I forgot to update currentStart_ on enable, so we ended up allocating behind currentStart_. This only trips assertions if currentStart_ is /equal/ to the current position, making isEmpty() true when it shouldn't be. I'm guessing I didn't see this assertion when I tested before because it is highly sensitive to allocation order.

Comment 13

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6311a62c5901

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6311a62c5901

Comment 15

[Terrence Cole [:terrence]]

Attachment: fix_decommited_nursery_reporting-v0.diff

Using the _GC_ macros adds some assertions which we aren't really prepared for since the nursery is attached to the runtime. I think the best option is just to use the normal macros for now, as you already did for the commited section. I should have just copied what you did exactly instead of trying to get clever.

Comment 16

[Nicholas Nethercote [inactive]]

Bugzilla appears to have swallowed my comment, which was: I'm surprised you don't need the _GC_ versions here, but if it fixes assertion failures then I guess it's the right thing to do.

Comment 17

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e72c13e60b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61e72c13e60b