Incrementalize weak marking phase

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
4 years ago
7 hours ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks 2 bugs, {leave-open, perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p2])

Attachments

(18 attachments, 22 obsolete attachments)

72.50 KB, patch
decoder
: feedback-
Details | Diff | Splinter Review
2.62 KB, patch
jonco
: review+
Details | Diff | Splinter Review
4.51 KB, patch
jonco
: review+
Details | Diff | Splinter Review
6.53 KB, patch
jonco
: review+
Details | Diff | Splinter Review
21.83 KB, patch
Details | Diff | Splinter Review
10.13 KB, patch
jonco
: review+
Details | Diff | Splinter Review
8.90 KB, patch
jonco
: review+
Details | Diff | Splinter Review
40.80 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Depends on: 1215752
Assignee

Updated

4 years ago
Depends on: 1216744
We should do this soon. Bug 1314828 shows that we're spending a lot of time in weak marking.
Assignee

Comment 2

2 years ago
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.
Assignee

Updated

2 years ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Whiteboard: [qf:p1]
Assignee

Comment 3

2 years ago
Version that passes shell tests, fails lots of mochitests
Assignee

Comment 4

2 years ago
Version that passes shell tests, fails lots of mochitests
Attachment #8870587 - Flags: review?(sphink)
Assignee

Updated

2 years ago
Attachment #8870585 - Attachment is obsolete: true
Assignee

Comment 5

2 years ago
Version that passes shell tests, fails lots of mochitests
Attachment #8870589 - Flags: review?(sphink)
Assignee

Updated

2 years ago
Attachment #8870587 - Attachment is obsolete: true
Attachment #8870587 - Flags: review?(sphink)
Assignee

Comment 6

2 years ago
Version that passes shell tests, fails lots of mochitests
Attachment #8870591 - Flags: review?(sphink)
Assignee

Updated

2 years ago
Attachment #8870589 - Attachment is obsolete: true
Attachment #8870589 - Flags: review?(sphink)
Assignee

Comment 7

2 years ago
Version that passes shell tests, fails lots of mochitests
Attachment #8870592 - Flags: review?(sphink)
Assignee

Updated

2 years ago
Attachment #8870591 - Attachment is obsolete: true
Attachment #8870591 - Flags: review?(sphink)
Assignee

Comment 8

2 years ago
Assignee

Comment 9

2 years ago
Assignee

Comment 10

2 years ago
Assignee

Updated

2 years ago
Attachment #8846775 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8874006 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8870592 - Attachment is obsolete: true
Attachment #8870592 - Flags: review?(sphink)
Assignee

Updated

2 years ago
Attachment #8874018 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8874027 - Attachment is obsolete: true
Assignee

Comment 11

2 years ago
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.
Keywords: leave-open
Assignee

Comment 12

2 years ago
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.)
Assignee

Comment 13

2 years ago
Rebased
Assignee

Updated

2 years ago
Attachment #8887114 - Attachment is obsolete: true
Deferred to post FF57
Whiteboard: [qf:p1] → [qf:p2]
Keywords: perf
Whiteboard: [qf:p2] → [perf:p2]
No longer blocks: 1323083

Updated

2 years ago
Whiteboard: [perf:p2] → [qf:p2]
Whiteboard: [qf:p2] → [qf:p1]
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.

Updated

2 years ago
Whiteboard: [qf:p1] → [qf:i60][qf:p1]

Updated

2 years ago
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
Blocks: 1323083
Steve advised other high priority things will prevent this from getting into 60.
Flags: needinfo?(jgong)

Updated

a year ago
Flags: needinfo?(jgong)
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
Steve, is there a chance you'll get to pick this up this year?
Flags: needinfo?(sfink)
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1]

Updated

a year ago
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]

Updated

10 months ago
Whiteboard: [qf:p1:f64] → [qf:p2]
Blocks: 1488435
Assignee

Comment 18

8 months ago
(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.
Flags: needinfo?(sfink)
Assignee

Updated

7 months ago
Attachment #9020394 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020395 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020396 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020397 - Attachment is obsolete: true
Assignee

Comment 25

7 months ago
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.
Assignee

Updated

7 months ago
Attachment #9020398 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020399 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020401 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020401 - Attachment is obsolete: false
Assignee

Updated

7 months ago
Attachment #9020399 - Attachment is obsolete: false
Assignee

Updated

7 months ago
Attachment #9020398 - Attachment is obsolete: false
Assignee

Updated

7 months ago
Attachment #9020397 - Attachment is obsolete: false
Assignee

Comment 29

7 months ago
Attachment #8887121 - Attachment is obsolete: true
Assignee

Comment 30

7 months ago
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.
Attachment #9020404 - Flags: feedback?(nth10sd)
Attachment #9020404 - Flags: feedback?(choller)
Now testing patch in comment 30 on m-c rev a2e694f49968 (please specify the m-c rev for future reference).
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
Attachment #9020404 - Flags: feedback?(nth10sd) → feedback-
Assignee

Comment 33

7 months ago
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.
Attachment #9021408 - Flags: review?(jcoppeard)
Assignee

Updated

7 months ago
Attachment #9020393 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9021408 - Attachment description: Simplify Compartment::findOutgoingEdges → Patch 1 - Simplify Compartment::findOutgoingEdges
Assignee

Updated

7 months ago
Attachment #9020397 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020398 - Attachment is obsolete: true
Assignee

Comment 37

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

(sorry, messed up numbering)
Attachment #9021409 - Attachment is obsolete: true
Attachment #9021409 - Flags: review?(jcoppeard)
Assignee

Comment 38

7 months ago
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.
Attachment #9021412 - Flags: review?(jcoppeard)
Assignee

Updated

7 months ago
Attachment #9020399 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020400 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #9020401 - Attachment is obsolete: true
Assignee

Comment 41

7 months ago
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-.
Attachment #9021415 - Flags: review?(jcoppeard)
Assignee

Updated

7 months ago
Attachment #9020402 - Attachment is obsolete: true
Comment on attachment 9021408 [details] [diff] [review]
Patch 1 - Simplify Compartment::findOutgoingEdges

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

Nice.
Attachment #9021408 - Flags: review?(jcoppeard) → review+
Attachment #9021410 - Flags: review?(jcoppeard) → review+
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?
Attachment #9021411 - Flags: review?(jcoppeard) → review+
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 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.
Attachment #9021413 - Flags: review?(jcoppeard) → review+
Attachment #9021414 - Flags: review?(jcoppeard) → review+
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.
Attachment #9021415 - Flags: review?(jcoppeard)
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.
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 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.
Attachment #9021412 - Flags: review?(jcoppeard)
Attachment #9020404 - Flags: feedback?(choller) → feedback-
Blocks: 1507440

Comment 50

5 months ago
Pushed by sfink@mozilla.com:
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
Assignee

Comment 61

7 hours ago

Depends on D31959

You need to log in before you can comment on or make changes to this bug.