Reduce the default GGC nursery size

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: terrence)

Tracking

Trunk
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 attachments, 2 obsolete attachments)

Spun off from bug 956501.  It sounds like the default GGC nursery size is 16MB.  That should be reduced for b2g.
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.
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.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8357378 - Flags: review?(jcoppeard)
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!
Attachment #8357378 - Flags: review?(jcoppeard) → review+
terrence, can you explain the effect of this patch?
Flags: needinfo?(terrence)
Whiteboard: [MemShrink] → [MemShrink:P1]
BTW, https://bugzilla.mozilla.org/show_bug.cgi?id=961883#c11 explains how the memory reporters should be updated for this change.
(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_.
Attachment #8363851 - Flags: review?(jcoppeard)
Flags: needinfo?(terrence)
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 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.
Attachment #8363851 - Flags: review?(jcoppeard) → review+
(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?
Attachment #8357378 - Attachment is obsolete: true
Attachment #8363851 - Attachment is obsolete: true
Attachment #8390681 - Flags: review+
Attachment #8390681 - Flags: feedback?(n.nethercote)
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."
Attachment #8390681 - Flags: feedback?(n.nethercote) → feedback+
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.
https://hg.mozilla.org/mozilla-central/rev/6311a62c5901
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8393787 - Flags: review?(n.nethercote)
Attachment #8393787 - Flags: review?(n.nethercote) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/61e72c13e60b
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.