GC: Sweep compartments in groups

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(13 attachments, 30 obsolete attachments)

22.80 KB, patch
billm
: review+
Details | Diff | Splinter Review
8.70 KB, patch
billm
: review+
Details | Diff | Splinter Review
13.89 KB, patch
billm
: review+
Details | Diff | Splinter Review
9.70 KB, patch
billm
: review+
Details | Diff | Splinter Review
33.31 KB, patch
billm
: review+
Details | Diff | Splinter Review
23.37 KB, patch
billm
: review+
Details | Diff | Splinter Review
4.59 KB, patch
billm
: review+
Details | Diff | Splinter Review
3.61 KB, patch
billm
: review+
Details | Diff | Splinter Review
8.38 KB, patch
billm
: review+
Details | Diff | Splinter Review
15.66 KB, patch
jonco
: review+
Details | Diff | Splinter Review
67.58 KB, patch
billm
: review+
Details | Diff | Splinter Review
33.75 KB, patch
billm
: review+
Details | Diff | Splinter Review
16.61 KB, patch
billm
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
The sweeping phase of GC involves a bunch of processing that has to happen for all collecting compartments as they transition from being marked to being swept, before returning to the caller.  This work cannot be performed incrementally and therefore can lead to noticeable pauses when many compartments are being collected.

One way round this is to not transition all compartments from marking to sweeping in one go.  There are however dependencies between compartments that mean there are constraints on the order compartments can be swept in, and that some compartments must be swept at the same time.

Specifically, if there pointer from an unmarked object in compartment A to unmarked object in compartment B then compartment B must not be swept before A.  If the these pointers are considered as edges in a graph, then the components that must be swept at the same time are the strongly connected components of the graph.
(Assignee)

Updated

6 years ago
QA Contact: jcoppeard
(Assignee)

Comment 1

6 years ago
Created attachment 660163 [details] [diff] [review]
Part 1 - find strongly connected components
(Assignee)

Comment 2

6 years ago
Created attachment 660165 [details] [diff] [review]
Part 2 - add Is*AboutToBeFinalized functions
(Assignee)

Comment 3

6 years ago
Created attachment 660167 [details] [diff] [review]
Part 3 - split up XPC finalization callback
(Assignee)

Comment 4

6 years ago
Created attachment 660168 [details] [diff] [review]
Part 4 - sweep compartments in groups
(Assignee)

Comment 5

6 years ago
Created attachment 660440 [details] [diff] [review]
Part 1 - find strongly connected components
Attachment #660163 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 660441 [details] [diff] [review]
Part 2 - add Is*AboutToBeFinalized functions
(Assignee)

Comment 7

6 years ago
Created attachment 660442 [details] [diff] [review]
Part 3 - split up XPC finalization callback
Attachment #660165 - Attachment is obsolete: true
Attachment #660167 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 660443 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #660168 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 661246 [details] [diff] [review]
Part 1 - find strongly connected components
Attachment #660440 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 661247 [details] [diff] [review]
Part 2 - add Is*AboutToBeFinalized functions
Attachment #660441 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 661248 [details] [diff] [review]
Part 3 - split up XPC finalization callback
Attachment #660442 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Created attachment 661249 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #660443 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 664077 [details] [diff] [review]
Part 1 - find strongly connected components
Attachment #661246 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 664078 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #661249 - Attachment is obsolete: true
Duplicate of this bug: 780960
(Assignee)

Comment 16

6 years ago
Created attachment 664544 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #664078 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
Created attachment 664545 [details] [diff] [review]
Part 5 - attempt to fix gray marking issues
Assignee: general → jcoppeard
QA Contact: jcoppeard
(Assignee)

Comment 18

6 years ago
Created attachment 668024 [details] [diff] [review]
Part 1 - find strongly connected components
Attachment #664077 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Created attachment 668025 [details] [diff] [review]
Part 2 - add Is*AboutToBeFinalized functions
Attachment #661247 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 668026 [details] [diff] [review]
Part 3 - split up XPC finalization callback
Attachment #661248 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 668027 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #664544 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Created attachment 668029 [details] [diff] [review]
Part 5 - attempt to fix gray marking issues
Attachment #664545 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 798678
(Assignee)

Comment 23

6 years ago
Created attachment 670823 [details] [diff] [review]
Part 1 - find strongly connected components
Attachment #668024 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
Created attachment 670824 [details] [diff] [review]
Part 2 - add Is*AboutToBeFinalized functions
Attachment #668025 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
Created attachment 670825 [details] [diff] [review]
Part 3 - split up XPC finalization callback
Attachment #668026 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Created attachment 670827 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #668027 - Attachment is obsolete: true
(Assignee)

Comment 27

6 years ago
Created attachment 670829 [details] [diff] [review]
Part 5 - attempt to fix gray marking issues
Attachment #668029 - Attachment is obsolete: true
(Assignee)

Comment 28

6 years ago
Created attachment 683176 [details] [diff] [review]
Part 1 - find strongly connected components
Attachment #670823 - Attachment is obsolete: true
Attachment #683176 - Flags: review?(wmccloskey)
(Assignee)

Comment 29

6 years ago
Created attachment 683177 [details] [diff] [review]
Part 2 - add Is*AboutToBeFinalized functions
Attachment #670824 - Attachment is obsolete: true
Attachment #683177 - Flags: review?(wmccloskey)
(Assignee)

Comment 30

6 years ago
Created attachment 683178 [details] [diff] [review]
Part 3 - split up XPC finalization callback
Attachment #670825 - Attachment is obsolete: true
Attachment #683178 - Flags: review?(wmccloskey)
(Assignee)

Comment 31

6 years ago
Created attachment 683179 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #683179 - Flags: review?(wmccloskey)
(Assignee)

Updated

6 years ago
Attachment #683179 - Attachment description: art 4 - sweep compartments in groups → Part 4 - sweep compartments in groups
(Assignee)

Updated

6 years ago
Attachment #670827 - Attachment is obsolete: true
(Assignee)

Comment 32

6 years ago
Created attachment 683180 [details] [diff] [review]
Part 5 - Fix gray marking issues
Attachment #670829 - Attachment is obsolete: true
Attachment #683180 - Flags: review?(wmccloskey)
(Assignee)

Comment 33

6 years ago
Created attachment 683181 [details] [diff] [review]
Part 6 - sweep debuggers and their debugees in the same group
Attachment #683181 - Flags: review?(wmccloskey)
(Assignee)

Comment 34

6 years ago
Created attachment 683183 [details] [diff] [review]
Part 7 - Update GC stats
Attachment #683183 - Flags: review?(wmccloskey)
(Assignee)

Comment 35

6 years ago
Created attachment 683185 [details] [diff] [review]
Part 8 - Track live arraybufs per compartment
Attachment #683185 - Flags: review?(wmccloskey)
(Assignee)

Comment 36

6 years ago
Created attachment 683187 [details] [diff] [review]
Part 9 - Refactor DebugScopes to be tracked per compartment
Attachment #683187 - Flags: review?(wmccloskey)
Whiteboard: [Snappy]
(Assignee)

Comment 37

6 years ago
Created attachment 683189 [details] [diff] [review]
Part 10 - Track WeakMaps per compartment
Attachment #683189 - Flags: review?(wmccloskey)
(Assignee)

Comment 38

6 years ago
Created attachment 683191 [details] [diff] [review]
Part 11 - Improve finding of Debugger edges
Attachment #683191 - Flags: review?(wmccloskey)
(Assignee)

Comment 39

6 years ago
Created attachment 683192 [details] [diff] [review]
Part 12 - Mark Watchpoints per compartment
Attachment #683192 - Flags: review?(wmccloskey)
(Assignee)

Comment 40

6 years ago
Created attachment 683193 [details] [diff] [review]
Part 13 - Handle nuked wrappers
Attachment #683193 - Flags: review?(wmccloskey)
Comment on attachment 683176 [details] [diff] [review]
Part 1 - find strongly connected components

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

::: js/src/gc/FindSCCs.cpp
@@ +11,5 @@
> +namespace js {
> +namespace gc {
> +
> +ComponentFinder::ComponentFinder(uintptr_t sl) :
> +    clock(1),

The colon should go on this line.

@@ +62,5 @@
> +            w->gcNextGraphNode = firstComponent;
> +            firstComponent = w;
> +        }
> +
> +        return;

Not needed.

@@ +112,5 @@
> +            w->gcDiscoveryTime = Finished;
> +
> +            /*
> +             * Prepend the component to the beginning of the output list to
> +             * reverse the list and acheive the desired order.

achieve

::: js/src/gc/FindSCCs.h
@@ +19,5 @@
> +    unsigned       gcDiscoveryTime;
> +    unsigned       gcLowLink;
> +
> +    GraphNodeBase() :
> +        gcNextGraphNode(NULL),

Colon should go on the same line as the first initializer.

@@ +69,5 @@
> + *     void findGraphEdges(ComponentFinder& finder)
> + *     {
> + *         for edge in my_outgoing_edges:
> + *             if is_relevant(edge):
> + *                 finder.addEdgeCallback(v, edge.destination)

This now takes just one parameter.

@@ +102,5 @@
> +    static GraphNodeBase *removeFirstGroup(GraphNodeBase *resultsList);
> +    static void removeAllRemaining(GraphNodeBase *resultsList);
> +
> +  public:
> +    // call from implementation of GraphNodeBase::findEdges()

Please use this style to make it consistent with the other comments in the file:
  /* Call from ... findEdges(). */

@@ +113,5 @@
> +    /* Constant used to indicate an processed vertex that is no longer on the stack. */
> +    static const unsigned Finished = (unsigned)-1;
> +
> +    /* Amount of headroom to leave when checking for stack exhaustion. */
> +    static const unsigned stackHeadroom = 1024;

I think this is no longer needed.

@@ +115,5 @@
> +
> +    /* Amount of headroom to leave when checking for stack exhaustion. */
> +    static const unsigned stackHeadroom = 1024;
> +
> +  private:

This is redundant.

@@ +119,5 @@
> +  private:
> +    void processNode(GraphNodeBase *v);
> +    void checkStackFull();
> +
> +  private:

This is unused.

@@ +121,5 @@
> +    void checkStackFull();
> +
> +  private:
> +
> +  private:

This stuff must be super-duper private :-).

@@ +122,5 @@
> +
> +  private:
> +
> +  private:
> +    unsigned   clock;

Please match the indentation of the fields below.

::: js/src/jsapi-tests/testFindSCCs.cpp
@@ +31,5 @@
> +
> +void
> +TestNode::findGraphEdges(ComponentFinder& finder)
> +{
> +    for (unsigned i = 0; i < MaxVertices; ++i)

Braces for this loop, since it encompasses multiple lines.
Attachment #683176 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 42

6 years ago
Created attachment 683502 [details] [diff] [review]
Part 1 - find strongly connected components, comments addressed
Attachment #683176 - Attachment is obsolete: true
Attachment #683502 - Flags: review+
(Assignee)

Comment 43

6 years ago
Created attachment 683671 [details] [diff] [review]
Part 4 - sweep compartments in groups
Attachment #683179 - Attachment is obsolete: true
Attachment #683179 - Flags: review?(wmccloskey)
Attachment #683671 - Flags: review?(wmccloskey)
(Assignee)

Comment 44

6 years ago
Created attachment 683672 [details] [diff] [review]
Part 5 - Fix gray marking issues
Attachment #683180 - Attachment is obsolete: true
Attachment #683180 - Flags: review?(wmccloskey)
Attachment #683672 - Flags: review?(wmccloskey)
(Assignee)

Comment 45

6 years ago
Created attachment 683674 [details] [diff] [review]
Part 7 - Update GC stats
Attachment #683183 - Attachment is obsolete: true
Attachment #683183 - Flags: review?(wmccloskey)
Attachment #683674 - Flags: review?(wmccloskey)
Attachment #683177 - Flags: review?(wmccloskey) → review+
Comment on attachment 683178 [details] [diff] [review]
Part 3 - split up XPC finalization callback

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

I think we should be able to remove all this mThreadRunningGC business, but I guess that can be a separate bug.
Attachment #683178 - Flags: review?(wmccloskey) → review+
Comment on attachment 683502 [details] [diff] [review]
Part 1 - find strongly connected components, comments addressed

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

::: js/src/gc/FindSCCs.h
@@ +24,5 @@
> +        gcDiscoveryTime(0),
> +        gcLowLink(0) {}
> +
> +    virtual ~GraphNodeBase() {}
> +    virtual void findGraphEdges(ComponentFinder& finder) = 0;

One more thing: I realized it would be clearer if this were called findOutgoingEdges.

@@ +103,5 @@
> +    static void removeAllRemaining(GraphNodeBase *resultsList);
> +
> +  public:
> +    /* Call from implementation of GraphNodeBase::findEdges(). */
> +    void addEdgeCallback(GraphNodeBase *w);

...and if this were called addEdgeTo.
Comment on attachment 683181 [details] [diff] [review]
Part 6 - sweep debuggers and their debugees in the same group

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

::: js/src/jsweakmap.h
@@ +204,5 @@
>              }
>          }
>      }
> +
> +protected:

This should be indented two spaces.

::: js/src/vm/Debugger.cpp
@@ +1555,5 @@
>      while (!debuggers->empty())
>          debuggers->back()->removeDebuggeeGlobal(fop, global, compartmentEnum, NULL);
>  }
>  
> +void Debugger::findCompartmentEdges(JSCompartment *v, js::gc::ComponentFinder &finder)

void should be on its own line. Also, we usually write /* static */ before void so it's clear it's a static method. Also, please call the compartment |comp| instead of |v|.

Also, I think this should have a comment explaining that it's adding edges in the reverse direction to ensure the debugger and debuggee are always in the same compartment group.

::: js/src/vm/Debugger.h
@@ +72,5 @@
> +    bool init(uint32_t len = 16) {
> +        return Base::init(len) && compartmentCounts.init();
> +    }
> +
> +    AddPtr lookupForAdd(const Lookup &l) const          { return Base::lookupForAdd(l); }

Usually we'd write this as:
    AddPtr lookupForAdd(const Lookup &l) const {
        return Base::lookupForAdd(l);
    }

Sometimes we do use these sorts of aligned one-liners, but only when there are several of them in a row.

There are a few other instances of this below, too.

@@ +77,5 @@
> +
> +    template<typename KeyInput, typename ValueInput>
> +    bool relookupOrAdd(AddPtr &p, const KeyInput &k, const ValueInput &v) {
> +        if (!valueCompartment)
> +            valueCompartment = v->compartment();

It seems cleaner to me to pass this into the constructor or init(). You can get it via dbg->object->compartment().

@@ +93,5 @@
> +    void remove(const Lookup &l) {
> +        Base::remove(l);
> +        decCompartmentCount(l->compartment());
> +        if (Base::count() == 0)
> +            valueCompartment = NULL;

I don't think this should ever be needed. As far as I know, we never change the compartment a Debugger object is in.

@@ +109,5 @@
> +            JS_ASSERT(key == r.front().key);
> +        }
> +    }
> +
> +    void findCompartmentEdges(JSCompartment *v, js::gc::ComponentFinder &finder) {

I think it would be cleaner if you had a function called |hasKeyInCompartment(comp)| that would return a bool. Then you could call that from Debugger::findCompartmentEdges. I think it's cleaner this way because the logic about debugger edges would be more centrally-located in Debugger.cpp. The way it is now, it's hard to follow without knowing the circumstances in which Debugger::findCompartmentEdges calls this function.
Attachment #683181 - Flags: review?(wmccloskey)
Comment on attachment 683674 [details] [diff] [review]
Part 7 - Update GC stats

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

::: js/src/gc/Statistics.cpp
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +s * vim: set ts=8 sw=4 et tw=78:

There's an extra 's' at the beginning of the line.
Attachment #683674 - Flags: review?(wmccloskey) → review+
Comment on attachment 683187 [details] [diff] [review]
Part 9 - Refactor DebugScopes to be tracked per compartment

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

::: js/src/jscompartment.cpp
@@ +676,5 @@
>          }
>      }
>  
> +    if (debugScopes)
> +        debugScopes->sweep(rt);

It would be nice if this were timed as part of SWEEP_TABLES.

::: js/src/vm/ScopeObject.cpp
@@ +1527,5 @@
>  
> +DebugScopes::DebugScopes(JSContext *c)
> + : proxiedScopes(c),
> +   missingScopes(c),
> +   liveScopes(c)

This won't work. When you pass a JSContext to a hashtable constructor, it hangs onto it so that it can automatically report OOM errors for you. However, the JSContext we pass in here may die before the compartment dies, in which case we'll have a dead reference.

However, I don't think there's any reason why you can't continue to pass in a JSRuntime here. You can get a JSRuntime via cx->runtime or via compartment->rt.
Attachment #683187 - Flags: review?(wmccloskey) → review+
Hmm, it looks like I was wrong about the context thing in comment 50. We explicitly instantiate hashtables with a RuntimeAllocPolicy, which just pulls the runtime off the context. So your change can stay, although I'm still unsure why you can't keep the old behavior. If you do decide to pass in a JSContext, it should be called |cx| instead of |c|.
Comment on attachment 683189 [details] [diff] [review]
Part 10 - Track WeakMaps per compartment

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

I like how you handled the saving and restoring of the weakmaps. That's much cleaner than what I was thinking.

::: js/src/jscompartment.cpp
@@ +100,1 @@
>          js_delete(debugScopes);

Indentation is messed up here.

@@ +674,5 @@
>          }
>      }
>  
> +    /* Finalize unreachable (key,value) pairs in all weak maps. */
> +    WeakMapBase::sweepCompartment(this);

Would be nice if this could be timed under SWEEP_TABLES too.

::: js/src/jsgc.cpp
@@ +3496,5 @@
>      }
>      rt->gcFoundBlackGrayEdges = false;
>  }
>  
> +bool MarkWeakMapsIteratively(JSRuntime *rt)

bool should go on a separate line.

@@ +3501,5 @@
> +{
> +    bool markedAny = false;
> +    GCMarker *gcmarker = &rt->gcMarker;
> +    for (GCCompartmentGroupIter c(rt); !c.done(); c.next()) {
> +        if (WeakMapBase::markCompartmentIteratively(c, gcmarker))

You can just use |= here.

@@ +3586,1 @@
>          return;

Indentation.

::: js/src/jsweakmap.cpp
@@ +63,1 @@
>          m->traceMappings(tracer);

Indentation broken (and the next line).
Attachment #683189 - Flags: review?(wmccloskey) → review+
Comment on attachment 683191 [details] [diff] [review]
Part 11 - Improve finding of Debugger edges

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

Just saw this :-). Thanks.
Attachment #683191 - Flags: review?(wmccloskey) → review+
Attachment #683192 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 54

6 years ago
(In reply to Bill McCloskey (:billm) from comment #51)

Thanks for all the reviews.

The reason I changed the DebugScopes constructor is that proxiedScopes is a WeakMap and I'm getting rid of the WeakMap constructor that takes a JSRuntime in the next patch. I think changed the others just to make it look cleaner.
Attachment #683185 - Flags: review?(wmccloskey) → review+
Comment on attachment 683671 [details] [diff] [review]
Part 4 - sweep compartments in groups

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

::: js/src/jscompartment.h
@@ +210,2 @@
>              return needsBarrier();
>          }

Indentation.

@@ +253,1 @@
>          return gcState == Mark;

Indentation.

@@ +378,5 @@
>      void sweep(js::FreeOp *fop, bool releaseTypes);
>      void sweepCrossCompartmentWrappers();
>      void purge();
>  
> +    void findGraphEdges(js::gc::ComponentFinder& finder);

I guess it's not required, but this should be declared virtual.

::: js/src/jsgc.cpp
@@ +3629,3 @@
>  #endif
>  
> +static void DropStringWrappers(JSRuntime *rt)

static and void should be on a separate line.

@@ +3672,1 @@
>          if (e.front().key.kind == CrossCompartmentKey::StringWrapper)

Can we now assert that this doesn't happen?

@@ +3675,5 @@
>          Cell *other = e.front().key.wrapped;
> +        if (!other->isMarked(BLACK) || other->isMarked(GRAY)) {
> +            JSCompartment *w = other->compartment();
> +            if (w->isGCMarking())
> +            finder.addEdgeCallback(w);

Indentation.

@@ +3744,5 @@
> +            Cell *dst = e.front().key.wrapped;
> +            Cell *src = ToMarkable(e.front().value);
> +            JS_ASSERT(src->compartment() == c);
> +            if (IsCellMarked(&src) && !src->isMarked(GRAY) && dst->isMarked(GRAY)) {
> +                //JS_ASSERT(!dst->compartment()->isCollecting());

Can this happen in more circumstances than it could before? Why did the assert need to be removed?

@@ +3767,1 @@
>                  c->setGCState(JSCompartment::Sweep);

Indentation, here and the rest of this function.

@@ +3781,5 @@
> +            rt->gcFinalizeCallback(&fop, JSFINALIZE_GROUP_START, !rt->gcIsFull /* unused */);
> +    }
> +
> +    if (sweepingAtoms) {
> +        gcstats::AutoPhase ap2(rt->gcStats, gcstats::PHASE_SWEEP_ATOMS);

This can be called ap.

@@ +3804,5 @@
>  
> +#ifdef JS_METHODJIT
> +        /* We need to expand inline frames before stack scanning. */
> +        for (CompartmentsIter c(rt); !c.done(); c.next())
> +            mjit::ExpandInlineFrames(c);

This code here is no longer necessary.

@@ +3907,5 @@
>  {
>      gcstats::AutoPhase ap(rt->gcStats, gcstats::PHASE_SWEEP);
>      FreeOp fop(rt, rt->gcSweepOnBackgroundThread);
>  
> +    for (;;) {

Indentation is messed up in this function.

@@ +4385,5 @@
>          /* fall through */
>        }
>  
>        case SWEEP: {
> +

Can this blank line be removed? Also, this indentation is messed up below.

::: js/src/jsinfer.cpp
@@ +6000,5 @@
>          return;
>      }
>  
> +    TypeObject *tmp = this;
> +    if (IsTypeObjectAboutToBeFinalized(&tmp)) {

I think this can stay as it is because we're guaranteed to be sweeping this compartment.

@@ +6117,5 @@
>              bool remove = false;
> +            TypeObject *typeObject = NULL;
> +            if (key.type.isTypeObject()) {
> +                typeObject = key.type.typeObject();
> +                if (IsTypeObjectAboutToBeFinalized(&typeObject))

Indentation.

@@ +6125,5 @@
>                  remove = true;
>  
>              if (remove)
>                  e.removeFront();
> +            else if (typeObject && typeObject != key.type.typeObject()) {

Need braces around the if part too.

@@ +6159,1 @@
>                      remove = true;

Indentation.

::: js/src/jspropertytree.cpp
@@ +203,5 @@
>      if (!inDictionary()) {
>          /*
> +         * We detach the child from the parent if the parent is reachable.
> +         *
> +         * Note that due to incremental sweeping, the parent pointer may point

I think you want to add "to" to the end of the line.

::: js/src/jstypedarray.cpp
@@ +534,5 @@
>              MarkObjectUnbarriered(trc, views, "arraybuffer.singleview");
> +    }
> +    // Multiple views: do not mark, remove unreachable views in sweepAll.
> +
> +    // Additionally, append buffer to list.

I don't think this needs to be changed. Please revert to the original.

@@ +581,5 @@
> +            if (lastBufferViews)
> +                SetBufferLink(lastBufferViews, buffer);
> +            else
> +                rt->liveArrayBuffers = buffer;
> +            lastBufferViews = *views;

I don't understand these changes. Maybe they were needed before we had a per-compartment list of buffers? If so, please revert to the original.

@@ +591,5 @@
> +    // Terminate the buffer list.
> +    if (lastBufferViews)
> +        SetBufferLink(lastBufferViews, NULL);
> +    else
> +        rt->liveArrayBuffers = NULL;

Same here.

::: js/src/jsweakmap.h
@@ +179,5 @@
>      void sweep(JSTracer *trc) {
>          /* Remove all entries whose keys remain unmarked. */
>          for (Enum e(*this); !e.empty(); e.popFront()) {
>              Key k(e.front().key);
> +            Value v(e.front().value);

Is this needed?
Attachment #683671 - Flags: review?(wmccloskey) → review+
Jon, I'd hoped to get through all of the reviews today, but I haven't had time. I should have some time to finish them tomorrow. That will also give me a chance to review the RenewWrapper fix.
(Assignee)

Comment 57

6 years ago
(In reply to Bill McCloskey (:billm) from comment #55)
> @@ +3744,5 @@
> > +            Cell *dst = e.front().key.wrapped;
> > +            Cell *src = ToMarkable(e.front().value);
> > +            JS_ASSERT(src->compartment() == c);
> > +            if (IsCellMarked(&src) && !src->isMarked(GRAY) && dst->isMarked(GRAY)) {
> > +                //JS_ASSERT(!dst->compartment()->isCollecting());
> 
> Can this happen in more circumstances than it could before? Why did the
> assert need to be removed?

Gray marking doesn't work at this point - it's fixed in the next patch, and the assertion reinstated.


> @@ +581,5 @@
> > +            if (lastBufferViews)
> > +                SetBufferLink(lastBufferViews, buffer);
> > +            else
> > +                rt->liveArrayBuffers = buffer;
> > +            lastBufferViews = *views;
> 
> I don't understand these changes. Maybe they were needed before we had a
> per-compartment list of buffers? If so, please revert to the original.

You're right, this was necessary when we had a single list of buffers.  I'll remove it.

Other comments addressed.

Jon
Comment on attachment 683672 [details] [diff] [review]
Part 5 - Fix gray marking issues

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

It seems like there's a lot of extra whitespace at the end of lines in this patch (especially empty lines). You can do
  (setq show-trailing-whitespace t)
in your .emacs and it will show it in red so it's easy to see.

::: js/src/gc/Marking.cpp
@@ +537,5 @@
>      }
>  }
>  
> +static bool
> +ShouldMarkCrossCompartment(JSTracer *trc, RawObject src, Cell *cell)

Could you rename |cell| to |dst|?

@@ +545,3 @@
>  
> +    JSCompartment *c = cell->compartment();
> +    uint32_t color = ((GCMarker *)trc)->getMarkColor();

We try to use static_cast for these sorts of casts. Although, come to think of it, it might be nice to add a function in jsgc.h that turns a JSTracer into a GCMarker while asserting IS_GC_MARKING_TRACER. Could you do that?

::: js/src/jscompartment.h
@@ +250,5 @@
>      }
>  
>      bool isGCMarking() {
>          if (rt->isHeapCollecting())
> +        return gcState == Mark || gcState == MarkGray;

Indentation.

::: js/src/jsgc.cpp
@@ +1533,5 @@
>              if (JS_UNLIKELY(comp->wasGCStarted())) {
>                  if (comp->needsBarrier()) {
>                      aheader->allocatedDuringIncremental = true;
>                      comp->rt->gcMarker.delayMarkingArena(aheader);
> +                } else if (comp->isGCSweeping() || comp->isGCFinished()) {

I don't understand why we would want to do this if isGCFinished() is true. We're already done sweeping that compartment, so there's no point in marking it any more.

@@ +1566,5 @@
>      if (JS_UNLIKELY(comp->wasGCStarted())) {
>          if (comp->needsBarrier()) {
>              aheader->allocatedDuringIncremental = true;
>              comp->rt->gcMarker.delayMarkingArena(aheader);
> +        } else if (comp->isGCSweeping() || comp->isGCFinished()) {

Same here.

@@ +1961,5 @@
>  
>      JS_ASSERT(!unmarkedArenaStackTop);
>      JS_ASSERT(markLaterArenas == 0);
>  
> +    grayRoots.clearAndFree();

Why can't we assert that grayRoots is empty any more? (The reason for the clearAndFree() is that the vector can use memory even if it's empty; clearAndFree() will free the memory.)

@@ +2134,1 @@
>          JS_SET_TRACING_LOCATION(this, (void *)&elem->thing);

Please fix the indentation here.

Also, we may eventually want a per-compartment list of gray roots. But this seems fine to me for now. I don't think we typically have very many.

@@ +3372,5 @@
>       * compartments that are not being collected, we are not allowed to collect
>       * atoms. Otherwise, the non-collected compartments could contain pointers
>       * to atoms that we would miss.
>       */
> +    JSCompartment* atomsComp = rt->atomsCompartment;

The * should go after the space.

@@ +3631,5 @@
> +                 * If the cycle collector isn't allowed to collect an object
> +                 * after a non-incremental GC has run, then it isn't allowed to
> +                 * collected it after an incremental GC.
> +                 */
> +                JS_ASSERT_IF(!bitmap->isMarked(cell, GRAY), !incBitmap.isMarked(cell, GRAY));

Excellent! It's nice to know that this is passing now.

@@ +3741,5 @@
>  
> +/*
> + * The following code makes use of a per-compartment singly-linked list of
> + * incoming cross compartment pointers whose referents are due to be marked
> + * gray.

Can you give some more background here about why we need this? Something about how we must mark gray after black, so we can't cross into a different compartment group while marking gray, since we may still need to mark some of its objects black.

@@ +3759,5 @@
> + * MarkIncomingCrossCompartmentPointers.
> + */
> +
> +static unsigned
> +GcGrayLinkSlot(RawObject o)

Please call it GCGrayLinkSlot (or maybe just GrayLinkSlot).

@@ +3761,5 @@
> +
> +static unsigned
> +GcGrayLinkSlot(RawObject o)
> +{
> +    return IsCrossCompartmentWrapper(o) ? JSSLOT_PROXY_EXTRA + 1 : Debugger::gcGrayLinkSlot();

Could you assert that it's either a cross-compartment wrapper or a debug wrapper thing (you might need to add a test for this somewhere)?

@@ +3767,5 @@
> +
> +static Cell *
> +CrossCompartmentPointerReferent(RawObject o)
> +{
> +    return (Cell*)(IsCrossCompartmentWrapper(o) ? GetProxyPrivate(o).toGCThing() : o->getPrivate());

Same assertion here too.

@@ +3777,5 @@
> +    unsigned slot = GcGrayLinkSlot(prev);
> +    RawObject next = prev->getReservedSlot(slot).toObjectOrNull();
> +
> +    if (unlink)
> +        prev->setSlot(slot, UndefinedValue());

Instead of the unlink param, you can use getSlotRef, which will return a reference to the slot. Then the caller can set it to undefined if it chooses. That seems cleaner to me.

@@ +3783,5 @@
> +    return next;
> +}
> +
> +void
> +js::DelayCrossCompartmentGrayMarking(RawObject src, Cell *cell)

You can change the name of |cell| to |dst|?

@@ +3850,5 @@
> +
> +        /*
> +         * Mark any incoming black pointers from previously swept compartments
> +         * whose referents are not marked. This can occur when gray cells become
> +         * black by the action of unmarkGray.

UnmarkGray, not unmarkGray.

::: js/src/jsgc.h
@@ +546,5 @@
>  
> +extern void
> +DelayCrossCompartmentGrayMarking(RawObject src, gc::Cell *cell);
> +
> +

Only one blank line please.

::: js/src/jsproxy.cpp
@@ +2853,1 @@
>      MarkSlot(trc, &obj->getReservedSlotRef(JSSLOT_PROXY_EXTRA + 1), "extra1");

Indent.

::: js/src/jsweakmap.h
@@ +129,5 @@
>      bool markValue(JSTracer *trc, Value *x) {
>          if (gc::IsMarked(x))
>              return false;
>          gc::Mark(trc, x, "WeakMap entry");
> +        /* If compartment is not currently being marked, Mark() may have no effect. */

If the compartment isn't being marked, won't the IsMarked check above return true?

::: js/src/vm/Debugger.cpp
@@ +409,5 @@
>  
> +JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGENV_GC_GRAY_LINK) ==
> +                 unsigned(JSSLOT_DEBUGOBJECT_GC_GRAY_LINK));
> +JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGENV_GC_GRAY_LINK) ==
> +                 unsigned(JSSLOT_DEBUGSCRIPT_GC_GRAY_LINK));

It probably makes sense to put this assertion directly above the definition of gcGrayLinkSlot.
Attachment #683672 - Flags: review?(wmccloskey) → review+
Comment on attachment 683193 [details] [diff] [review]
Part 13 - Handle nuked wrappers

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

Looks great. Good luck!

::: js/src/jsgc.cpp
@@ +3808,5 @@
> +    return IsCrossCompartmentWrapper(o) ||
> +           IsDeadProxyObject(o) ||
> +           c == &DebuggerObject_class ||
> +           c == &DebuggerEnv_class ||
> +           c == &DebuggerScript_class;

Could you add code to Debugger.{cpp,h} for a function named IsDebugWrapper that would check the last three cases?

@@ +4100,3 @@
>          JS_ASSERT(!c->gcIncomingGrayPointers);
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();

The & should go after the space.

@@ +4101,5 @@
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();
> +            if (wrapper.isObject()) {
> +                RawObject o = &wrapper.toObject();
> +                if (IsCrossCompartmentWrapper(o))

Could we make this more general and check that GCGrayListSlot is undefined regardless of the kind of wrapper?

@@ +4279,3 @@
>          JS_ASSERT(!c->gcIncomingGrayPointers);
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();

Ampersand placement.

@@ +4280,5 @@
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();
> +            if (wrapper.isObject()) {
> +                RawObject o = &wrapper.toObject();
> +                if (IsCrossCompartmentWrapper(o))

Same here as above.
Attachment #683193 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 60

6 years ago
(In reply to Bill McCloskey (:billm) from comment #58)

> Why can't we assert that grayRoots is empty any more? (The reason for the
> clearAndFree() is that the vector can use memory even if it's empty;
> clearAndFree() will free the memory.)

The original code cleared grayRoots after marking its contents, but now we only remove the gray roots for compartments we actually mark.  I assume it's possible for grayRoots to contain roots in compartments that we aren't currently collecting.

> @@ +3777,5 @@
> > +    unsigned slot = GcGrayLinkSlot(prev);
> > +    RawObject next = prev->getReservedSlot(slot).toObjectOrNull();
> > +
> > +    if (unlink)
> > +        prev->setSlot(slot, UndefinedValue());
> 
> Instead of the unlink param, you can use getSlotRef, which will return a
> reference to the slot. Then the caller can set it to undefined if it
> chooses. That seems cleaner to me.

I tried this but it turns out that you need to pass the object and slot number to HeapSlot::set anyway, so it doesn't really end up any cleaner.

Other comments addressed.

Cheers,

Jon
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED

Updated

6 years ago
Depends on: 816046

Updated

6 years ago
Depends on: 817342
Depends on: 817002
Depends on: 817419
Depends on: 820186
Depends on: 826649
(Assignee)

Updated

4 years ago
Depends on: 1001355
You need to log in before you can comment on or make changes to this bug.