Closed Bug 1251463 Opened 4 years ago Closed 4 years ago

Remove ShrinkGCBuffers

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- affected
firefox50 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Currently the ShrinkGCBuffers timeout is 4 seconds. This gets delayed as long as there is user activity, but at 4 seconds the usage has to be pretty intense for this not to fire. And if the usage is intense, the chunks that were swept are available for reallocation all throughout sweeping and decommit anyway (keeping in mind that BackgroundAllocTask will grab more chunks if we're allocating), so I think it's pretty unlikely this is doing anything these days. Which means that the entire aging and shrinking mechanism is probably not doing much of anything either.

 7 files changed, 22 insertions(+), 197 deletions(-)

Well, it is doing that I guess.
Attachment #8723846 - Flags: review?(jcoppeard)
Attachment #8723846 - Flags: review?(continuation)
Comment on attachment 8723846 [details] [diff] [review]
10.02_expire_chunks_in_decommit-v0.diff

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

r=me for the nsJSEnvironment.{h,cpp} changes
Attachment #8723846 - Flags: review?(continuation) → review+
Comment on attachment 8723846 [details] [diff] [review]
10.02_expire_chunks_in_decommit-v0.diff

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

This is a really nice simplification, but I think we should check that it doesn't affect performance.  Maybe check talos results?  Or land it back it out if there are problems.  I'm trying to think if there's any telemetry we could look at to tell us...

I think maxEmptyChunkCount_ is dead now with this change.

::: js/src/gc/Heap.h
@@ +834,5 @@
>  struct ChunkInfo
>  {
>      void init() {
>          next = prev = nullptr;
> +        //age = 0;

Needs removing.
Attachment #8723846 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #3)
> Comment on attachment 8723846 [details] [diff] [review]
> 10.02_expire_chunks_in_decommit-v0.diff
> 
> Review of attachment 8723846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a really nice simplification, but I think we should check that it
> doesn't affect performance.  Maybe check talos results?  Or land it back it
> out if there are problems.  I'm trying to think if there's any telemetry we
> could look at to tell us...

Checking talos before landing is a great idea.

> I think maxEmptyChunkCount_ is dead now with this change.

I was thinking it was used by the background alloc task, but I guess that only fills up to minEmptyChunkCount. I'd actually like to make it smarter: estimate the allocation rate and allocate between minEmptyChunkCount and maxEmptyChunkCount. 

> ::: js/src/gc/Heap.h
> @@ +834,5 @@
> >  struct ChunkInfo
> >  {
> >      void init() {
> >          next = prev = nullptr;
> > +        //age = 0;
> 
> Needs removing.

D'oh! Missed one.
https://hg.mozilla.org/mozilla-central/rev/f35cc482a71b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.