826673 - GC: Only finish sweeping the current compartment group on reset

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

When a GC reset occurs in the sweep phase, the current behaviour is to finish the entire collection before starting a new one.  This is unnecessary.  Only the compartment group that is currently being swept needs to be swept to completion.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 698760 [details] [diff] [review]
Proposed fix

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

::: js/src/jsgc.cpp
@@ +3097,5 @@
>      rt->gcCurrentCompartmentGroup = rt->gcCompartmentGroups;
>      rt->gcCompartmentGroupIndex = 0;
>  }
>  
> +static void ResetGrayList(JSCompartment* comp);

Could you put a newline after void? Also, the * should go after the space.

@@ +3110,5 @@
>          ComponentFinder<JSCompartment>::mergeCompartmentGroups(rt->gcCurrentCompartmentGroup);
> +
> +    if (rt->gcAbortSweepAfterCurrentGroup) {
> +        JS_ASSERT(!rt->gcIsIncremental);
> +        for (GCCompartmentGroupIter c(rt); !c.done(); c.next()) {

Could you put a comment above this loop that says something like "Iterate over all compartments that haven't been swept yet."?

@@ +3314,5 @@
>      return false;
>  }
>  
> +static void
> +ResetGrayList(JSCompartment* comp)

The * should go after the space.

@@ +3844,5 @@
> +            JS_ASSERT(c->isGCMarking());
> +            c->setNeedsBarrier(false, JSCompartment::UpdateIon);
> +            c->setGCState(JSCompartment::NoGC);
> +            ArrayBufferObject::resetArrayBufferList(c);
> +            ResetGrayList(c);

Was missing the ResetGrayList an existing bug? I wonder why it didn't assert before. It looks like we have an assertion to check that this is empty at the beginning and the end of every GC.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #3)
Comments addressed.

> Was missing the ResetGrayList an existing bug? I wonder why it didn't assert
> before. It looks like we have an assertion to check that this is empty at
> the beginning and the end of every GC.

Yes as far as I can see it was an existing bug - I guess we just never tripped over this (also, we need better tests for gray marking).

Comment 5

[Jon Coppeard (:jonco)]

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

Comment 6

[Jon Coppeard (:jonco)]

Backed out for breaking everything:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc1445ba415

Comment 7

[Jon Coppeard (:jonco)]

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

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab8bfaeaeafc