1167452 - Incrementalize weak marking phase

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

Weakmap marking should happen incrementally when possible. I marked a dependency on bug 1164294, but I'm not sure if it is really dependent. It seems like the iterative algorithm could be incremental too.

Comment 1

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

We should do this soon. Bug 1314828 shows that we're spending a lot of time in weak marking.

Comment 2

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

Attachment: WIP: Attempts at incremental weak marking

Recording my current state here. In theory, this should mostly work. But the compiler helpfully told me about some unworkable stuff I was trying to do, which must be resolved. I put in a temporary comment describing the situation.

Snapshotting so I can focus on something else for a little while.

Comment 3

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

Attachment: Incrementalize weakmap marking

Version that passes shell tests, fails lots of mochitests

Comment 4

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

Attachment: Incrementalize weakmap marking

Version that passes shell tests, fails lots of mochitests

Comment 5

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

Attachment: Incrementalize weakmap marking

Version that passes shell tests, fails lots of mochitests

Comment 6

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

Attachment: Incrementalize weakmap marking

Version that passes shell tests, fails lots of mochitests

Comment 7

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

Attachment: Incrementalize weakmap marking

Version that passes shell tests, fails lots of mochitests

Comment 8

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

Attachment: Incrementalize weakmap marking

Comment 9

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

Attachment: Incrementalize weakmap marking

Comment 10

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

Attachment: Incrementalize weakmap marking

Comment 11

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

Current status: some browser tests are now green! Still many failures remaining, though. I'm going to start putting parts of this up for review.

Comment 12

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

Attachment: Incrementalize weakmap marking

Notes on the algorithm:

How it works before this patch:
 - during regular marking, when a WM is encountered, scan through its entries
   - anything known to be live (as in, the key or its delegate is marked) is marked through
   - else, do nothing
 - when transitioning to weak marking phase
   - enter weak marking mode (so any marking that happens from now on will take into account
     the keys so far in weakKeys)
   - scan through all *live* WMs
     - mark through anything known to be live
     - otherwise, enter into weakKeys table
 - so at this point, weakKeys contains all live WM's keys that have not yet been marked through
   - WMs not yet known to be live do not have their keys in the table
   - WMs whose keys were already known to be live when they were encountered will not be in the table
   - entries whose values are in the table will either be in weakKeys (if they've already been seen)
     or they will be encountered later in this pass

Just during incremental
 - when starting iGC
   - clear all marks
   - clear weakKeys
   - enable barriers
 - barriers
   - insert K: if map is marked, mark V if K is marked, else add K to weakKeys
     - marking V is an optimization to avoid putting everything in weakKeys
   - delete K: if map is marked, delete K from weakKeys
     - if map is unmarked, K should not be in there
   - update existing K:
     - if map is marked, mark V
       - for the case where added via iGC WM mark and K was already marked. The old V would
         be marked, but we could be swapping in an unmarked one
 - incremental marking
   - when WM is marked, do markEntries (currently markIteratively)
     - mark through live keys, enter other keys into weakKeys
   - when some random key is marked, do nothing
     - will be handled when transitioning to weak marking mode
 - transitioning to weak marking mode
   - at start, all simply-reachable WMs will be marked and have any unknown keys in weakKeys
   - flip the weakMarking mode flag
   - scan through weakKeys
     - mark through marked keys
       - even if key already marked (to handle update)?

Invariants:
 - (1) all values that were at any time associated with a marked weakmap and marked
   key will end up getting marked
   - at the time WM is marked
     - if V's K was not yet present
       - if K mapped to V when K was inserted
         - if K was marked, V gets marked when K is inserted
         - if K was unmarked, <WM,K> was added to weakKeys, and all weakKeys' values are marked
       - if K mapped to something else, later changed to V, V is marked during update
     - if V's K was marked when the WM was marked, its V will be marked
     - if V's K was unmarked when the WM was marked, <WM,K> was added to weakKeys
 - (2) any unmarked key in a marked weakmap will be in weakKeys
   - note that some marked keys might be in there too
   - trivially true outside of GC because no weakmaps are marked
   - maintained during iGC by:
     - when marking a WM, all unmarked keys are added
     - when adding a new key to a marked WM, key is added (or maybe unmarked only)
     - when updating a key's value in a marked WM:
       - K already in weakKeys - trivially maintained
       - K not in weakKeys
         - if K is unmarked, then it was unmarked when WM was marked, so is in weakKeys
         - if K is marked, then it is irrelevant for this invariant. But invariant (1) is
           maintained by marking the value. (This is conservative; the value could later
           be overwritten.)
 - (3) all keys in weakKeys are keys in some marked weakmap
   - the only thing ever added to weakKeys are keys in marked weakmaps
   - maintained during deletion by removing deleted <WM,K> pair from weakKeys
   - maintained during update by virtue of update not changing the key

Time overhead is proportional to total number of keys in live weakmaps. In practice, we try to make it proportional to only the number of live keys that are not simply reachable (ie, that require marking through weakmaps to discover that they are live.) This property is not fully upheld, because of marking order. (If you mark a weakmap before one of its keys, the key will be added to weakKeys and later scanned despite being simply reachable.)

Comment 13

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

Attachment: Incrementalize weakmap marking

Rebased

Comment 14

[Naveed Ihsanullah [:naveed]]

Deferred to post FF57

Comment 15

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

Would it be possible to do something simpler here? It seems like there are two possibilities:

1. We do a little "actual" weak marking work, which causes a big subgraph of the heap to be marked black, and all that black marking takes time. This subgraph does not contain any weak pointers itself.
2. Or, it's possible that we have tons of weak maps and the weak marking itself is slow.

#1 seems more likely to me since I still suspect people don't use weakmaps that much. If this is the case, couldn't we keep the atomic weak marking pass, but still do some incremental weak marking before it? That would allow us to avoid the complexity of barriers while still solving #1.

Comment 16

[Andrew Overholt [:overholt]]

Steve advised other high priority things will prevent this from getting into 60.

Comment 17

[Kannan Vijayan [:djvj]]

Steve, is there a chance you'll get to pick this up this year?

Comment 18

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

(In reply to Kannan Vijayan [:djvj] from comment #17)
> Steve, is there a chance you'll get to pick this up this year?

Yes, I've picked this back up and am working on it now.

Comment 19

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

Attachment: Patch 1 - Simplify Compartment::findOutgoingEdges

Comment 20

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

Attachment: Patch 2 - Move getDelegate from WeakMap<K,V> to WeakMapBase

Comment 21

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

Attachment: Patch 3 - Move getDelegate from WeakMap<K,V> to WeakMapBase

Comment 22

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

Attachment: Patch 4 - Move getDelegate from WeakMap<K,V> to WeakMapBase

Comment 23

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

Attachment: Patch 2 - Move getDelegate from WeakMap<K,V> to WeakMapBase

Comment 24

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

Attachment: Patch 3 - Implement isMarked, IsForwarded, and Forwarded for GCCellPtr

Comment 25

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

Attachment: Patch 4 - Move gcWeakKeys from zone to compartment

The weak keys table is used to look up a just-marked object and find what weakmap entries need to be marked through. For a CCW key, both the wrapped object and the CCW need to be registered in the table. If they are in the same table (because it's per-Zone), then if the non-CCW is removed from the weak keys table then the CCW will be left behind.

This doesn't fundamentally cause any problems other than potentially some overmarking, but it prevents some assertions (eg that gcWeakKeys only contains <weakmap,key> pairs where the key is in the weakmap.) Also, in a future change there will be nursery keys involved, and then we'll end up with nursery objects lying around in gcWeakKeys after the nursery has been evicted, which is ugly and interferes with more assertions.

Comment 26

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

Attachment: Patch 5 - Split out nursery weak keys from tenured weak keys

Comment 27

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

Attachment: Patch 6 - Prevent barriers from firing during tracing, refactor a little

Comment 28

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

Attachment: Patch 7 - Incrementalize weakmap marking

Comment 29

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

Attachment: rollup for fuzzing

Comment 30

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

Comment on attachment 9020404 [details] [diff] [review]
rollup for fuzzing

Would it be possible to get some fuzzing on this patch? It includes a test that should be a decent starting point.

Comment 31

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Now testing patch in comment 30 on m-c rev a2e694f49968 (please specify the m-c rev for future reference).

Comment 32

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I found a large hard-to-reproduce fragile testcase with the following symptoms and sent it to :sfink.

Assertion failure: tag_ == TracerKindTag::WeakMarking, at /home/ubuntu/trees/mozilla-central/js/src/gc/Marking.cpp:2709

(gdb) bt
#0  0x000055555698dc42 in js::GCMarker::leaveWeakMarkingMode (this=0x7ffff5d1d690) at /home/ubuntu/trees/mozilla-central/js/src/gc/Marking.cpp:2708
#1  0x00005555569f9421 in js::GCMarker::abortLinearWeakMarking (this=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/gc/GCMarker.h:295
#2  js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::insertWeakEntry (marker=0x7ffff5d1d690, comp=<optimized out>, key=..., markable=...)
    at /home/ubuntu/trees/mozilla-central/js/src/gc/WeakMap-inl.h:189
#3  0x00005555569f92be in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::addWeakEntry (this=0x7ffff257f4a0, marker=0x7ffff5d1d690, 
    key=...) at /home/ubuntu/trees/mozilla-central/js/src/gc/WeakMap-inl.h:206
#4  0x00005555569df8d1 in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::traceEntries (this=0x7ffff257f4a0, marker=0x7ffff5d1d690)
    at /home/ubuntu/trees/mozilla-central/js/src/gc/WeakMap-inl.h:240
#5  0x00005555569dd793 in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::trace (this=0x7ffff257f4a0, trc=0x7ffff5d1d690)
    at /home/ubuntu/trees/mozilla-central/js/src/gc/WeakMap-inl.h:156
#6  0x0000555556cc180b in js::Class::doTrace (trc=<optimized out>, obj=<optimized out>, this=<optimized out>)
    at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-1167452-c30_diff-1afd28541328-a2e694f49968/objdir-js/dist/include/js/Class.h:899
#7  JSObject::traceChildren (this=0x7ffff29c74c0, trc=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/vm/JSObject.cpp:4314
#8  0x00005555569dc742 in JS::DispatchTraceKindTyped<TraceChildrenFunctor, JSTracer*&, void*&> (f=..., traceKind=<optimized out>, 
    args=<optimized out>, args=<optimized out>)
    at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-1167452-c30_diff-1afd28541328-a2e694f49968/objdir-js/dist/include/js/TraceKind.h:205
#9  0x00005555569d10b7 in js::TraceChildren (trc=0x7ffff5d1d690, thing=0x7ffff29c74c0, kind=<optimized out>)
    at /home/ubuntu/trees/mozilla-central/js/src/gc/Tracer.cpp:143
#10 0x000055555698ddf9 in js::GCMarker::markDelayedChildren (this=0x7ffff5d1d690, arena=<optimized out>)
    at /home/ubuntu/trees/mozilla-central/js/src/gc/Marking.cpp:2735

Comment 33

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

Attachment: Patch 1 - Simplify Compartment::findOutgoingEdges

Posting rebased patches for review. Try is showing no failures, but a bunch of timeouts that are worrisome. Still, it seems close enough to begin reviewing at least some of these patches.

Comment 34

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

Attachment: Patch 2 - Implement isMarked, IsForwarded, and Forwarded for GCCellPtr

Comment 35

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

Attachment: Patch 2 - Move getDelegate from WeakMap<K,V> to WeakMapBase

Comment 36

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

Attachment: Patch 3 - Implement isMarked, IsForwarded, and Forwarded for GCCellPtr

Comment 37

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

Comment on attachment 9021409 [details] [diff] [review]
Patch 2 - Implement isMarked, IsForwarded, and Forwarded for GCCellPtr

(sorry, messed up numbering)

Comment 38

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

Attachment: Patch 4 - Move gcWeakKeys from zone to compartment

The weak keys table is used to look up a just-marked object and find what weakmap entries need to be marked through. For a CCW key, both the wrapped object and the CCW need to be registered in the table. If they are in the same table (because it's per-Zone), then if the non-CCW is removed from the weak keys table then the CCW will be left behind.

This doesn't fundamentally cause any problems other than potentially some overmarking, but it prevents some assertions (eg that gcWeakKeys only contains <weakmap,key> pairs where the key is in the weakmap.) Also, in a future change there will be nursery keys involved, and then we'll end up with nursery objects lying around in gcWeakKeys after the nursery has been evicted, which is ugly and interferes with more assertions.

...except that I've become skeptical that this is really needed. Or rather, that it's the best way. I do need to handle same-Zone wrappers somehow, and this patch simplifies that to just handling same-compartment wrappers. But rather than having separate tables, I probably ought to allow multiple entries in the Zone's weak keys tables (one per wrapper, plus one for their common delegate). I haven't tried it yet, though; this seems to be working although with more looping than I'd like.

Comment 39

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

Attachment: Patch 5 - Split out nursery weak keys from tenured weak keys

Comment 40

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

Attachment: Patch 6 - Prevent barriers from firing during tracing, refactor a little

Comment 41

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

Attachment: Patch 7 - Incrementalize weakmap marking

This patch could use some cleanup, and possibly some splitting into simpler pieces. I'll put it up for review now, mostly so you can see it together with all of the earlier patches. I expect an r-.

Comment 42

[Jon Coppeard (:jonco)]

Comment on attachment 9021408 [details] [diff] [review]
Patch 1 - Simplify Compartment::findOutgoingEdges

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

Nice.

Comment 43

[Jon Coppeard (:jonco)]

Comment on attachment 9021411 [details] [diff] [review]
Patch 3 - Implement isMarked, IsForwarded, and Forwarded for GCCellPtr

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

Looks fine, but I'm wondering why we need to use GCCellPtr internally?  Could we Cell* instead?

Comment 44

[Jon Coppeard (:jonco)]

Comment on attachment 9021412 [details] [diff] [review]
Patch 4 - Move gcWeakKeys from zone to compartment

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

> The weak keys table is used to look up a just-marked object and find what weakmap entries need to be marked through. For a CCW key, both the wrapped object and the CCW need to be registered in the table

This is probably a dumb question, but why?  Wouldn't we end up marking the target when we mark the wrapper anyway?

> we'll end up with nursery objects lying around in gcWeakKeys after the nursery has been evicted,

Wouldn't this be a problem if another object was allocated at that address?

> this patch simplifies that to just handling same-compartment wrappers

I don't think we have same-compartment CCWs.  I think I misunderstood something here.

In general it would be better not to do this if we don't have too.  I'll leave review until I understand what's going on.

Comment 45

[Jon Coppeard (:jonco)]

Comment on attachment 9021413 [details] [diff] [review]
Patch 5 - Split out nursery weak keys from tenured weak keys

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

Wow this is fiddly.  Makes sense though.

::: js/src/gc/Marking.h
@@ +231,5 @@
> +// Remove all elements of a collection matching a predicate. General utility
> +// used by Marking.cpp and Zone.cpp.
> +template <typename Collection, typename ShouldRemove>
> +static void
> +purge(Collection& entries, ShouldRemove shouldRemove)

nit: name should be capitalised, also maybe should be called RemoveIf or something more obvious.

::: js/src/vm/Compartment.cpp
@@ +467,5 @@
>      for (RealmsInCompartmentIter r(this); !r.done(); r.next()) {
>          r->sweepAfterMinorGC();
>      }
> +
> +    for (WeakKeyTable::Range r = gcNurseryWeakKeys().all(); !r.empty(); r.popFront()) {

nit: Maybe split this block out into a separate method.

Comment 46

[Jon Coppeard (:jonco)]

Comment on attachment 9021415 [details] [diff] [review]
Patch 7 - Incrementalize weakmap marking

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

Some questions...  I think I also need you to talk me though how this works but we can do that at the meeting today.

::: js/src/gc/GCMarker.h
@@ +304,5 @@
>          return !!unmarkedArenaStackTop;
>      }
>  
> +    void purgeWeakKey(WeakMapBase* map, JS::Compartment* comp,
> +                      JS::GCCellPtr key, JS::GCCellPtr deadmeat);

deadmeat? :)

::: js/src/gc/Marking.cpp
@@ +2671,5 @@
> +    // instead.)
> +    //
> +    // Scan through gcWeakKeys and mark all values whose keys are marked, to
> +    // get to a consistent state where all values are marked if both their map
> +    // and key are marked.

That consistent state sounds like the end state of weak marking, but that can't be what's happening here.

@@ +2688,5 @@
> +            // we may add additional entries while iterating over the Range.
> +            gc::WeakKeyTable::Range r = comp->gcWeakKeys().all();
> +            while (!r.empty()) {
> +                JS::GCCellPtr key = r.front().key;
> +                if (gc::IsMarked(rt, &key)) {

Shouldn't this be IsMarkedUnbarriered?  How does this work if key is GCCellPtr?

::: js/src/gc/WeakMap-inl.h
@@ +59,5 @@
> +{
> +    if (!zone->isGCMarking()) {
> +        return false;
> +    }
> +    return (zone->gcState() == JS::shadow::Zone::MarkGray) == (markColor() == gc::MarkColor::Gray);

You can use zone->isGCMarkingGray() here.

@@ +88,5 @@
>  // the mark bits on everything it cares about, one of which will be
>  // markedCell. But a subclass might use it to optimize the liveness check.
> +//
> +// markEntry is called when encountering a weakmap key during marking, or when
> +// entering weak marking mode.

Thanks for adding that.  This file is pretty confusing generally.

@@ +100,5 @@
>      // Lookup that can't be constructed from a Cell*. The WeakKeyTable
>      // mechanism is indexed with a GCCellPtr, so that won't work.
>      Ptr p = Base::lookup(static_cast<Lookup>(origKey.asCell()));
> +
> +    // We should only processing <weakmap,key> pairs where the key exists in

nit: You probably meant 'only be processing'.

::: js/src/gc/WeakMap.cpp
@@ +57,5 @@
>  WeakMapBase::markZoneIteratively(JS::Zone* zone, GCMarker* marker)
>  {
>      bool markedAny = false;
>      for (WeakMapBase* m : zone->gcWeakMapList()) {
> +        if (m->marked && m->markZoneIteratively(zone, marker)) {

Is this calling itself recursively here or did you add a non-static markZoneIteratively?

@@ +138,5 @@
> +    // For weakmap keys with delegates in a different zone, add a zone edge to
> +    // ensure that the delegate zone finishes marking before the key zone. And
> +    // then add an edge the other direction too, because scanning the
> +    // gcWeakKeys for a zone containing a delegate might end up marking a value
> +    // in the map/key zone.

I don't really understand this last bit.  How does scannling the list end up marking anything?

Also, it would be best if we could avoid adding the reverse edge if possible to keep sweeping as incremental as possible.

::: js/src/gc/WeakMap.h
@@ +106,5 @@
>      // Any weakmap key types that want to participate in the non-iterative
>      // ephemeron marking must override this method.
>      virtual void markEntry(GCMarker* marker, gc::Cell* markedCell, JS::GCCellPtr l) = 0;
>  
> +    virtual bool traceEntries(GCMarker* marker) = 0;

This should take a JSTracer* if it's just tracing.  It looks like it depends the argument being a GCMarker for the addWeakEntry call though.

@@ +201,5 @@
> +    bool keyIsMarked(T& k) {
> +        return !k->zone()->shouldMarkInZone() || !k->isTenured() || k->asTenured().isMarkedAny();
> +    }
> +
> +    void barrierForInsert(Key k, const Value& v) {

Should this be private?

::: js/src/vm/ProxyObject.cpp
@@ +157,5 @@
> +    // are remapping the wrapper.
> +    JSObject* delegate = js::WeakMapBase::getDelegate(this);
> +    if (delegate) {
> +        delegate->zone()->delegatePreWriteBarrier(this, delegate);
> +    }

It looks like we do removeWrapper() when we're nuking a wrapper, which also does this barrier.  Maybe this isn't required?

@@ +161,5 @@
> +    }
> +    // FIXME: What should happen when there are multiple layers of proxies? eg
> +    // this is a WaiveXray of an Xray of an object. getDelegate() will return
> +    // the innermost object -- but then what happens if you look up the middle
> +    // layer in a WeakMap?

Good question.  If possible please add a test to exercise this.

Comment 47

[Jon Coppeard (:jonco)]

Another thing that would be great is to add a comment to the top of WeakMap-inl.h giving an overview of how all this works.

Comment 48

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: found(), at dist/include/mozilla/HashTable.h:1273
Build version: mozilla-central revision a2e694f49968+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu
Runtime options: --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --disable-oom-functions --ion-eager

Testcase:

gczeal(10, 1)
gczeal(15);
var blacklist = {
  'quit': true,
  'readline': true,
  'nukeAllCCWs': true,
};
function f(y) {}
for (let e of Object.values(newGlobal())) {
  if (e.name in blacklist)
      continue;
  try {
      e();
  } catch (r) {}
}
function loadFile(lfVarx) {}

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x569cf6b6 in mozilla::detail::HashTable<mozilla::HashMapEntry<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value> >, mozilla::HashMap<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value>, js::CrossCompartmentKey::Hasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Ptr::operator-> (this=0xffffb918) at dist/include/mozilla/HashTable.h:1273
#1  0x569e40a4 in js::RemapWrapper (cx=<optimized out>, wobjArg=<optimized out>, newTargetArg=<optimized out>) at js/src/proxy/CrossCompartmentWrapper.cpp:634
#2  0x569e5f4c in js::RecomputeWrappers (cx=0xf6e1b800, sourceFilter=..., targetFilter=...) at js/src/proxy/CrossCompartmentWrapper.cpp:745
#3  0x56726475 in RecomputeWrappers (cx=0xf6e1b800, argc=0, vp=0xffffbdc0) at js/src/shell/js.cpp:6115
#4  0x56894a9a in CallJSNative (cx=0xf6e1b800, native=0x56726380 <RecomputeWrappers(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:468
#5  0x56887fcd in js::InternalCallOrConstruct (cx=<optimized out>, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:560
#6  0x56888570 in InternalCall (cx=cx@entry=0xf6e1b800, args=...) at js/src/vm/Interpreter.cpp:614
#7  0x5688872a in js::Call (cx=0xf6e1b800, fval=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:633
#8  0x569ff8d3 in js::ForwardingProxyHandler::call (this=0x578b9330 <js::CrossCompartmentWrapper::singleton>, cx=0xf6e1b800, proxy=..., args=...) at js/src/proxy/Wrapper.cpp:178
#9  0x569e9762 in js::CrossCompartmentWrapper::call (this=0x578b9330 <js::CrossCompartmentWrapper::singleton>, cx=<optimized out>, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:355
#10 0x569f68d2 in js::Proxy::call (cx=0xf6e1b800, proxy=..., args=...) at js/src/proxy/Proxy.cpp:560
#11 0x568883fd in js::InternalCallOrConstruct (cx=<optimized out>, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:535
#12 0x56888570 in InternalCall (cx=cx@entry=0xf6e1b800, args=...) at js/src/vm/Interpreter.cpp:614
#13 0x5688872a in js::Call (cx=0xf6e1b800, fval=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:633
#14 0x56fd0456 in js::jit::InvokeFunction (cx=<optimized out>, obj=..., constructing=false, ignoresReturnValue=true, argc=0, argv=0xffffc240, rval=...) at js/src/jit/VMFunctions.cpp:112
#15 0x4ab8c59f in ?? ()
#16 0xf687e790 in ?? ()
eax	0x0	0
ebx	0x578d5ff4	1468882932
ecx	0xf7d90864	-136771484
edx	0x0	0
esi	0xffffb8d0	-18224
edi	0xffffb92c	-18132
ebp	0xffffb878	4294948984
esp	0xffffb870	4294948976
eip	0x569cf6b6 <mozilla::detail::HashTable<mozilla::HashMapEntry<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value> >, mozilla::HashMap<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value>, js::CrossCompartmentKey::Hasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Ptr::operator->() const+86>
=> 0x569cf6b6 <mozilla::detail::HashTable<mozilla::HashMapEntry<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value> >, mozilla::HashMap<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value>, js::CrossCompartmentKey::Hasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Ptr::operator->() const+86>:	movl   $0x0,0x0
   0x569cf6c0 <mozilla::detail::HashTable<mozilla::HashMapEntry<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value> >, mozilla::HashMap<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value>, js::CrossCompartmentKey::Hasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Ptr::operator->() const+96>:	ud2



The initial testcase was highly intermittent, now it should be (fairly) stable.

Comment 49

[Jon Coppeard (:jonco)]

Comment on attachment 9021412 [details] [diff] [review]
Patch 4 - Move gcWeakKeys from zone to compartment

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

Cancelling review for now so bugzilla stops hassling me.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1796e4fc907
Simplify Compartment::findOutgoingEdges, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5568f16f0191
Move getDelegate from WeakMap<K,V> to WeakMapBase, r=jonco

Comment 51

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d1796e4fc907
https://hg.mozilla.org/mozilla-central/rev/5568f16f0191

Comment 52

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

Attachment: Bug 1167452 - Set gcLastSweepGroupIndex earlier (debugging feature) r=jonco

Comment 53

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

Attachment: Bug 1167452 - Switch weakmap marking code back from GCCellPtr to plain Cell* r=jonco

Depends on D31951

Comment 54

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

Attachment: Bug 1167452 - Split out nursery weak keys from tenured weak keys r=jonco

Depends on D31952

Comment 55

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

Attachment: Bug 1167452 - Prevent barriers from firing during tracing, rename markIteratively -> markEntries r=jonco

Depends on D31953

Comment 56

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

Attachment: Bug 1167452 - Split mark stack into black and gray. r=jonco

Depends on D31954

Comment 57

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

Attachment: Bug 1167452 - Make a Cell::isMarked(color) utility function. r=jonco

Depends on D31955

Comment 58

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

Attachment: Bug 1167452 - Implement a mark queue to control marking order during testing r=jonco

Depends on D31956

Comment 59

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

Attachment: Bug 1167452 - Barrier weakmap operations and maintain weak keys table during incremental collections. r=jonco

Depends on D31957

Comment 60

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

Attachment: Bug 1167452 - Make weakmap marking incremental r=jonco

Depends on D31958

Comment 61

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

Attachment: Bug 1167452 - Unbarrier lookup for delete r=jonco

Depends on D31959

Comment 62

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

Attachment: Bug 1167452 - Add a currentgc() function that returns lots of info on the internal incremental GC state. r=jonco

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01cf1b9a681c
Set gcLastSweepGroupIndex earlier (debugging feature) r=jonco
https://hg.mozilla.org/integration/autoland/rev/1566746f179c
Switch weakmap marking code back from GCCellPtr to plain Cell* r=jonco
https://hg.mozilla.org/integration/autoland/rev/d1b2e8a06822
Split out nursery weak keys from tenured weak keys r=jonco
https://hg.mozilla.org/integration/autoland/rev/5e813b247fee
Prevent barriers from firing during tracing, rename markIteratively -> markEntries r=jonco
https://hg.mozilla.org/integration/autoland/rev/436fbb0e51e5
Split mark stack into black and gray. r=jonco
https://hg.mozilla.org/integration/autoland/rev/3b174feaa3f2
Make a Cell::isMarked(color) utility function. r=jonco
https://hg.mozilla.org/integration/autoland/rev/196e318992aa
Implement a mark queue to control marking order during testing r=jonco

Comment 64

[Cristian Brindusan [:cbrindusan]]

Backed out 7 changesets (bug 1167452) for spidermonkey bustages on weak-marking-02.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/c452cbbba791fabe660a39d8cf22a200ddb107ff

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&selectedJob=249040643&revision=196e318992aa867869ab3de7cea0ae1365320cc3

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249040643&repo=autoland&lineNumber=22453

Comment 65

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

Oops, I guess I never tested opt builds. Sorry about that.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7c3cf320526
Set gcLastSweepGroupIndex earlier (debugging feature) r=jonco
https://hg.mozilla.org/integration/autoland/rev/2202a5363839
Switch weakmap marking code back from GCCellPtr to plain Cell* r=jonco
https://hg.mozilla.org/integration/autoland/rev/a4f9778eec68
Split out nursery weak keys from tenured weak keys r=jonco
https://hg.mozilla.org/integration/autoland/rev/ab159f8b9041
Prevent barriers from firing during tracing, rename markIteratively -> markEntries r=jonco
https://hg.mozilla.org/integration/autoland/rev/617df479fac1
Split mark stack into black and gray. r=jonco
https://hg.mozilla.org/integration/autoland/rev/83677df08b08
Make a Cell::isMarked(color) utility function. r=jonco
https://hg.mozilla.org/integration/autoland/rev/d5c768b50d69
Implement a mark queue to control marking order during testing r=jonco

Comment 67

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b7c3cf320526
https://hg.mozilla.org/mozilla-central/rev/2202a5363839
https://hg.mozilla.org/mozilla-central/rev/a4f9778eec68
https://hg.mozilla.org/mozilla-central/rev/ab159f8b9041
https://hg.mozilla.org/mozilla-central/rev/617df479fac1
https://hg.mozilla.org/mozilla-central/rev/83677df08b08
https://hg.mozilla.org/mozilla-central/rev/d5c768b50d69

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37f9bd277c34
Barrier weakmap operations and maintain weak keys table during incremental collections. r=jonco
https://hg.mozilla.org/integration/autoland/rev/11d3f2265b35
Make weakmap marking incremental r=jonco
https://hg.mozilla.org/integration/autoland/rev/72fc109fe0f2
Unbarrier lookup for delete r=jonco

Comment 69

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/37f9bd277c34
https://hg.mozilla.org/mozilla-central/rev/11d3f2265b35
https://hg.mozilla.org/mozilla-central/rev/72fc109fe0f2

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad665e4aafbb
Add a currentgc() function that returns lots of info on the internal incremental GC state. r=jonco

Comment 71

[Alexandru Michis [:malexandru]]

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

Comment 72

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Last changeset got backed. More information about the backout in bug 1514421.

Comment 73

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

Thank you sheriffs for the surgical backouts. After all the carnage, there are 3 regressions that I'll need to deal with to re-land:

bug 1514421 - this is the big one, it's a correctness problem that shows up as an uncommon (but not rare) intermittent. I fixed one incarnation of it shortly before landing. Apparently there's another.

bug 1556706 - smallish memory regression. This is still in the tree. It's from https://phabricator.services.mozilla.com/D31955 and I hope to fix or at least minimize it by playing with numbers.

bug 1557701 - performance regression. I was kind of hoping this would have the opposite effect, so I'll have to look at what's going on.

Comment 74

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

Oops, make that 4 regressions. I didn't notice this last one. It would be nice if the tree graph treated regressions as if they were either blocking or depending.

bug 1556329 - correctness problem. But it's caught in a fuzz bug, so life is good.

Comment 75

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad75b878827
Backed out changeset 617df479fac1

Comment 76

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

Attachment: Bug 1167452 - When verifying weakmap marking, consider whether key's zone is collecting

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f27409c81df2
When verifying weakmap marking, consider whether key's zone is collecting r=jonco

Comment 78

[Dorel Luca [:dluca]]

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

Comment 79

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

Attachment: Bug 1167452 - Add a pref to control incremental weakmap marking

Comment 80

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

Attachment: Bug 1167452 - Add a pref to control incremental weakmap marking

Comment 81

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

Attachment: Bug 1167452 - Add a pref to control incremental weakmap marking

Comment 82

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

Attachment: Bug 1167452 - Add a pref to control incremental weakmap marking

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/175201340cdf
Barrier weakmap operations and maintain weak keys table during incremental collections. r=jonco
https://hg.mozilla.org/integration/autoland/rev/c5b504e35a81
Make weakmap marking incremental r=jonco
https://hg.mozilla.org/integration/autoland/rev/1f8ee4357261
Unbarrier lookup for delete r=jonco
https://hg.mozilla.org/integration/autoland/rev/d284cce161c3
Add a pref to control incremental weakmap marking r=jonco

Comment 84

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

Attachment: Bug 1167452 - minor warning fix

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8bd2751e166
minor warning fix

Comment 86

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/175201340cdf
https://hg.mozilla.org/mozilla-central/rev/c5b504e35a81
https://hg.mozilla.org/mozilla-central/rev/1f8ee4357261
https://hg.mozilla.org/mozilla-central/rev/d284cce161c3
https://hg.mozilla.org/mozilla-central/rev/a8bd2751e166

Comment 87

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

Resolving bug, actual activation happening in bug 1633176.

Comment 88

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

Attachment: Backed out incremental weakmap marking (bug 1167452 and bug 1633176) to postpone until after Fx77

Comment 89

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91ec9a272383
Backed out incremental weakmap marking (bug 1167452 and bug 1633176) to postpone until after Fx77

Comment 90

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/91ec9a272383

Comment 91

[Florian Scholz (Open Web Docs)]

This looks like a great achievement! I've added a note to the developer release notes at https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/78#JavaScript. Let me know if it captures this well.

Comment 92

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

(In reply to Florian Scholz [:fscholz] (MDN) from comment #91)

This looks like a great achievement! I've added a note to the developer release notes at https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/78#JavaScript. Let me know if it captures this well.

It sounds just right to me. Thank you!