978387 - GGC: fix performance on the benchmark in bug 978077

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

The convolute2d testcase behaves catastrophically on GGC. It is sadfaces.

The test has one large ABO -- an image -- and it creates lots and lots and lots of tiny view objects pointing at it. The ABO ends up in tenured and the view objects are nursery allocated. At GC time the whole-object buffer has the ABO and not much else. So far this is all behaving excellently.

Then things go sideways quickly. The ABO contains a "weak" list of views pointing at it. We mark this strongly during minor gc because we have to. Of course, this means we mark our lots and lots and lots of tiny views. And tenure all of them. And keep them on the linked list: suddenly we have O(n^2) collection behavior. D'oh!

Comment 1

[Niko Matsakis [:nmatsakis]]

Terrifying. One part of this is that I think, if we were optimizing properly, we wouldn't have the views at all -- but of course in some cases we might, and it'd be nice if they didn't make GGC fall over. I really *really* have to read more into the GC code, seems like the details keep coming up.

Comment 2

[Terrence Cole [:terrence]]

Note: "have to" in comment 0 is a bit strong. I think we can actually make use of the existing weak marking infrastructure for this list. We may have to make some adjustments to isAboutToBeFinalized to account for the fact that we only "mark" nursery things during minor GC. In particular (*objp)->isMarked() is going to be false, but we might not even get there. The code in question is:

#ifdef JSGC_GENERATIONAL
    Nursery &nursery = (*thingp)->runtimeFromMainThread()->gcNursery;
    if (nursery.isInside(*thingp))
        return !nursery.getForwardedPointer(thingp);
#endif
    if (!(*thingp)->tenuredZone()->isGCSweeping())
        return false;

    return !(*thingp)->isMarked();

We have an override for nursery things and an optimizations for non-marking zones before we check the mark bits. Since no zones are going to be sweeping during minor GC and getForwardedPointer returns the right thing, this may just work as is.

I hate to add this much complexity to the minor GC at the last moment, but I don't see a better way. If I can't come up with a simpler solution by Monday, I'll try to make this work.

Comment 3

[Terrence Cole [:terrence]]

Attachment: weak_abo_views-v0.diff

This reduces our minor collection times on this benchmark from 0.5s to 100us, allowing the benchmark to finish. We're still quite slow compared to a non-ggc build: 24s compared to 14s, so some further work is necessary. I expect it is probably because of excessive store-buffer traffic, but I need to investigate further.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8384754 [details] [diff] [review]
weak_abo_views-v0.diff

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

Nice!

::: js/src/gc/Nursery.cpp
@@ +780,5 @@
>      collectToFixedPoint(&trc, tenureCounts);
>      TIME_END(collectToFP);
>  
> +    // Update the array buffer object's view lists.
> +    TIME_START(sweepArrayBufferViewList);

Man, these things are such a pain.

@@ +807,5 @@
>      // If we are promoting the nursery, or exhausted the store buffer with
>      // pointers to nursery things, which will force a collection well before
>      // the nursery is full, look for object types that are getting promoted
>      // excessively and try to pretenure them.
> +    TIME_START(pretenure);

I guess you didn't want the time for the comment to be included in the pretenure time? :)

(j/k)

::: js/src/vm/ArrayBufferObject.cpp
@@ +841,2 @@
>      if (trc->runtime->isHeapMinorCollecting()) {
> +        IsObjectAboutToBeFinalized(&GetViewListRef(&buffer));

Ugh. Can't this have another name when its return value is being ignored? It can just call the same thing.

Comment 5

[Terrence Cole [:terrence]]

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

(In reply to Steve Fink [:sfink] from comment #4)
> ::: js/src/gc/Nursery.cpp
> @@ +780,5 @@
> >      collectToFixedPoint(&trc, tenureCounts);
> >      TIME_END(collectToFP);
> >  
> > +    // Update the array buffer object's view lists.
> > +    TIME_START(sweepArrayBufferViewList);
> 
> Man, these things are such a pain.

Yet less painful than both Jon and I having to independently rebase our slightly different versions over each other's patches constantly. :-)
 
> @@ +807,5 @@
> >      // If we are promoting the nursery, or exhausted the store buffer with
> >      // pointers to nursery things, which will force a collection well before
> >      // the nursery is full, look for object types that are getting promoted
> >      // excessively and try to pretenure them.
> > +    TIME_START(pretenure);
> 
> I guess you didn't want the time for the comment to be included in the
> pretenure time? :)
> 
> (j/k)

Now we won't have to manually subtract it out of totalTime. ;-)

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +841,2 @@
> >      if (trc->runtime->isHeapMinorCollecting()) {
> > +        IsObjectAboutToBeFinalized(&GetViewListRef(&buffer));
> 
> Ugh. Can't this have another name when its return value is being ignored? It
> can just call the same thing.

When we started working on GGC, Bill and I were really hoping that everything would either be marked strongly or weakly: the view list is inconveniently neither. Still, an explicit "other way" is better than abusing IsAboutToBeFinalized, so I added an UpdateFooIfRelocated API for this case.

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #5)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b5899e48b7fb
> 
> (In reply to Steve Fink [:sfink] from comment #4)
> > ::: js/src/gc/Nursery.cpp
> > @@ +780,5 @@
> > >      collectToFixedPoint(&trc, tenureCounts);
> > >      TIME_END(collectToFP);
> > >  
> > > +    // Update the array buffer object's view lists.
> > > +    TIME_START(sweepArrayBufferViewList);
> > 
> > Man, these things are such a pain.
> 
> Yet less painful than both Jon and I having to independently rebase our
> slightly different versions over each other's patches constantly. :-)

Actually, I was referring to the whole view list and conditional weakness stuff. Splinter really needs multi-select or something. It can be really hard to figure out what is being commented on.

Comment 8

[Terrence Cole [:terrence]]

No further complaints, so I guess this per issue was fixed.