Clean up JSCompartment tracing

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

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.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Created attachment 8632842 [details] [diff] [review]
Part 0: tidy-compartment-minor-sweep

fitzgen is going to work on this, but I did write this one small patch.
Attachment #8632842 - Flags: review?(terrence)
Created attachment 8633088 [details] [diff] [review]
Part 1: s/mark/trace/ on relevant JSCompartment methods
Attachment #8633088 - Flags: review?(jcoppeard)
Created attachment 8633090 [details] [diff] [review]
Part 1: Consolidate JSCompartment roots tracing within JSCompartment::traceRoots
Attachment #8633090 - Flags: review?(jcoppeard)
Try push for parts 0 and 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46797bbf36b2

Updated

2 years ago
Attachment #8633088 - Flags: review?(jcoppeard) → review+
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.
Attachment #8633090 - Flags: review?(jcoppeard)
Attachment #8632842 - Flags: review?(terrence) → review+
(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.
Created attachment 8633846 [details] [diff] [review]
Part 2: Consolidate JSCompartment roots tracing within JSCompartment::traceRoots
Attachment #8633090 - Attachment is obsolete: true
Attachment #8633846 - Flags: review?(jcoppeard)
Created attachment 8633847 [details] [diff] [review]
Part 3: Consolidate CCW edge fixing after a moving gc into a single method
Attachment #8633847 - Flags: review?(jcoppeard)
Created attachment 8633848 [details] [diff] [review]
Part 4: Consolidate CCW root tracing for per-zone GCs into a single method
Attachment #8633848 - Flags: review?(jcoppeard)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a73687e91e37
Attachment #8632842 - Attachment description: tidy-compartment-minor-sweep → Part 0: tidy-compartment-minor-sweep
Attachment #8633088 - Attachment description: Part 0: s/mark/trace/ on relevant JSCompartment methods → Part 1: s/mark/trace/ on relevant JSCompartment methods
Attachment #8633088 - Attachment filename: Bug-1182104---Part-0-smarktrace-on-relevant-JSComp.patch → Bug-1182104---Part-1-smarktrace-on-relevant-JSComp.patch

Comment 11

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

Updated

2 years ago
Attachment #8633846 - Flags: review?(jcoppeard) → review+
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.
Attachment #8633847 - Flags: review?(jcoppeard) → review+
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.
Attachment #8633848 - Flags: review?(jcoppeard) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/6c8f7d17e334
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Created attachment 8634229 [details] [diff] [review]
Part 4: Consolidate CCW root tracing for per-zone GCs into a single method
Attachment #8633848 - Attachment is obsolete: true
Attachment #8634229 - Flags: review+
Try push w/ xpcshell and mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f23c22012e
Try push looks good.

(This was missing the leave-open keyword, and shouldn't have been resolved.)
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

Comment 19

2 years ago
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
Keywords: checkin-needed
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
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.