Closed Bug 1133759 Opened 5 years ago Closed 5 years ago

Always shrink GC buffers at the end of a compacting GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch shrink-buffers-on-shrinking-gc (obsolete) — Splinter Review
At the moment we have the situation where ShrinkGCBuffers() is called by the browser some time after a full (non-zone) GC has finished.  This means that arenas freed up by a shrinking zone GCs may not be decommitted until after the next full GC.

I don't think it ever makes sense not shrink as much as possible after a shrinking GC, so in this case I think we should do it automatically.
Attachment #8565452 - Flags: review?(terrence)
Blocks: CompactingGC
Comment on attachment 8565452 [details] [diff] [review]
shrink-buffers-on-shrinking-gc

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

I thought we already did this at the end of endSweepPhase, but it looks like it's conditional on !sweepOnBackgroundThread. The comment there doesn't really make sense to me either.

In any case, I think we definitely want this.
Attachment #8565452 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/45a72ac5b382
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8565452 [details] [diff] [review]
shrink-buffers-on-shrinking-gc

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

::: js/src/jsgc.cpp
@@ +5464,5 @@
>  #endif
>  
> +    // Ensure execess chunks are returns to the system and free arenas
> +    // decommitted.
> +    shrinkBuffers();

I should have done this in finishCollection() rather than compactPhase() in case we decided not to compact.  Also, I can't spell.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
And here's the fix.
Attachment #8567986 - Flags: review?(terrence)
Comment on attachment 8567986 [details] [diff] [review]
bug1133759-shrink-at-end

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

::: js/src/jsgc.cpp
@@ +5509,5 @@
>          MOZ_ASSERT(!zone->isCollecting());
>          MOZ_ASSERT(!zone->wasGCStarted());
>      }
>  
> +    // Ensure execess chunks are returned to the system and free arenas

s/execess/excess/
Attachment #8567986 - Flags: review?(terrence) → review+
Backed out again for octane-splay regression:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dee3876119fc
Updated to only apply to shrinking builds as originally intended.
Attachment #8565452 - Attachment is obsolete: true
Attachment #8569261 - Flags: review?(terrence)
Comment on attachment 8569261 [details] [diff] [review]
bug1133759-shrink-at-end-v2

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

Sorry I didn't catch that the first time!
Attachment #8569261 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/4a0181e789cc
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla39
You need to log in before you can comment on or make changes to this bug.