1182104 - Clean up JSCompartment tracing

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

JSCompartment stuff is traced in like 10 different ways, there is no single entry point (despite there being a method JSCompartment::trace, which only traces the compartment's SavedStacks). And the various ways it is traced are called in multiple different places with a different set of things traced in each. We have optimizations to only trace certain edges if we are marking vs general tracing or if this is a minor GC or not, which doesn't help legibility.

A few examples, probably don't have everything:

  JSCompartment::markRoots
  JSCompartment::trace
  JSCompartment::markCrossCompartmentWrappers

  Within markRuntime:
        /* During a GC, these are treated as weak pointers. */
        if (traceOrMark == TraceRuntime) {
            if (c->watchpointMap)
                c->watchpointMap->markAll(trc);
        }


        /* Mark debug scopes, if present */
        if (c->debugScopes)
            c->debugScopes->mark(trc);

        if (c->lazyArrayBuffers)
            c->lazyArrayBuffers->trace(trc);

        if (c->objectMetadataTable)
            c->objectMetadataTable->trace(trc);

The result is that trying to understand JSCompartment tracing leads to confusion and adding new things that must be traced from JSCompartment is difficult and error prone.

Sweeping JSCompartments is a pretty similar situation.

It would be *awesome* if we could consolidate all this stuff into a single entry point that then had all of the isHeapMinorCollecting checks and tracing vs marking branches within it, so that we contained some of the complexity within JSCompartment and made adding or modifying things traced a little easier.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Part 0: tidy-compartment-minor-sweep

fitzgen is going to work on this, but I did write this one small patch.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: s/mark/trace/ on relevant JSCompartment methods

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Consolidate JSCompartment roots tracing within JSCompartment::traceRoots

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push for parts 0 and 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46797bbf36b2

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8633090 [details] [diff] [review]
Part 1: Consolidate JSCompartment roots tracing within JSCompartment::traceRoots

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

Looks fine with comments addressed.  We need to address the try failures though.

::: js/src/jscompartment.cpp
@@ +526,5 @@
>                    "on-stack object pending metadata");
>      }
>  
> +    if (trc->runtime()->isHeapMinorCollecting()) {
> +        globalWriteBarriered = false;

I think this should go somewhere else since it's not tracing/marking related at all.  My previous patch on this bug moved this to JSCompartment::sweepAfterMinorGC().

@@ +549,5 @@
> +    // Cross compartment wrappers must be marked when they are potentially
> +    // pointing into other zones that are being GC'd separately from this one,
> +    // or when we are fixing up edges during compacting.
> +    if (markingForOtherZonesGC || trc->runtime()->gc.isHeapCompacting())
> +        traceCrossCompartmentWrappers(trc);

I think we want to keep traceCrossCompartmentWrappers() out of traceRoots() because conceptually they are not roots of this compartment, and it complicates the logic here.

::: js/src/jsgc.cpp
@@ -2616,4 @@
>              WeakMapBase::markAll(c, &trc);
> -            if (c->watchpointMap)
> -                c->watchpointMap->markAll(&trc);
> -        }

This seems to have been removed without being replaced anywhere.  Possibly responsible for the CGC test failures.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> Looks fine with comments addressed.  We need to address the try failures
> though.

Yup.

> @@ +549,5 @@
> > +    // Cross compartment wrappers must be marked when they are potentially
> > +    // pointing into other zones that are being GC'd separately from this one,
> > +    // or when we are fixing up edges during compacting.
> > +    if (markingForOtherZonesGC || trc->runtime()->gc.isHeapCompacting())
> > +        traceCrossCompartmentWrappers(trc);
> 
> I think we want to keep traceCrossCompartmentWrappers() out of traceRoots()
> because conceptually they are not roots of this compartment, and it
> complicates the logic here.

Yeah, that makes a lot of sense.

> ::: js/src/jsgc.cpp
> @@ -2616,4 @@
> >              WeakMapBase::markAll(c, &trc);
> > -            if (c->watchpointMap)
> > -                c->watchpointMap->markAll(&trc);
> > -        }
> 
> This seems to have been removed without being replaced anywhere.  Possibly
> responsible for the CGC test failures.

The watchpoint map is marked inside JSCompartment::traceRoots, which is already called by GCRuntime::markRuntime, which is called below. Will dig in some more, I might have messed up the logic for scenarios when the watchpoint map does or doesn't get traced.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Consolidate JSCompartment roots tracing within JSCompartment::traceRoots

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Consolidate CCW edge fixing after a moving gc into a single method

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Consolidate CCW root tracing for per-zone GCs into a single method

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a73687e91e37

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8f7d17e334

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 8633847 [details] [diff] [review]
Part 3: Consolidate CCW edge fixing after a moving gc into a single method

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

Looks good.

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 8633848 [details] [diff] [review]
Part 4: Consolidate CCW root tracing for per-zone GCs into a single method

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

::: js/src/jscompartment.h
@@ +528,5 @@
>       * dangling (full GCs naturally follow pointers across compartments) and
>       * when compacting to update cross-compartment pointers.
>       */
>      void traceOutgoingCrossCompartmentWrappers(JSTracer* trc);
> +    static void traceAllCrossCompartmentWrappers(JSTracer* trc);

I think we need a better name because this doesn't trace *all* CCWs, just ones from zones that are not being collected.  Something like traceIncomingCrossCompartmentEdgesForZoneGC maybe although that's a bit of a mouthful.

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Pulsebot from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8f7d17e334

Try results look good, but it's important to run the browser tests too as zone GC is not well tested by the JS tests on their own.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c8f7d17e334

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Consolidate CCW root tracing for per-zone GCs into a single method

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push w/ xpcshell and mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f23c22012e

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push looks good.

(This was missing the leave-open keyword, and shouldn't have been resolved.)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b7719ca560e
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d8c40ffbbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/552f005dd07c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb0551bfc3a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b7719ca560e
https://hg.mozilla.org/mozilla-central/rev/21d8c40ffbbf
https://hg.mozilla.org/mozilla-central/rev/552f005dd07c
https://hg.mozilla.org/mozilla-central/rev/5bb0551bfc3a