Closed Bug 1167452 Opened 9 years ago Closed 4 years ago

Incrementalize weak marking phase

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Performance Impact medium

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(23 files, 25 obsolete files)

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
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
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.
Depends on: 1215752
Depends on: 1216744
We should do this soon. Bug 1314828 shows that we're spending a lot of time in 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.
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Whiteboard: [qf:p1]
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Version that passes shell tests, fails lots of mochitests
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Version that passes shell tests, fails lots of mochitests
Attachment #8870587 - Flags: review?(sphink)
Attachment #8870585 - Attachment is obsolete: true
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Version that passes shell tests, fails lots of mochitests
Attachment #8870589 - Flags: review?(sphink)
Attachment #8870587 - Attachment is obsolete: true
Attachment #8870587 - Flags: review?(sphink)
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Version that passes shell tests, fails lots of mochitests
Attachment #8870591 - Flags: review?(sphink)
Attachment #8870589 - Attachment is obsolete: true
Attachment #8870589 - Flags: review?(sphink)
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Version that passes shell tests, fails lots of mochitests
Attachment #8870592 - Flags: review?(sphink)
Attachment #8870591 - Attachment is obsolete: true
Attachment #8870591 - Flags: review?(sphink)
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Attachment #8846775 - Attachment is obsolete: true
Attachment #8874006 - Attachment is obsolete: true
Attachment #8870592 - Attachment is obsolete: true
Attachment #8870592 - Flags: review?(sphink)
Attachment #8874018 - Attachment is obsolete: true
Attachment #8874027 - Attachment is obsolete: true
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
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
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.)
Attached patch Incrementalize weakmap marking (obsolete) — Splinter Review
Rebased
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
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.
Whiteboard: [qf:p1] → [qf:i60][qf:p1]
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)
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]
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]
Whiteboard: [qf:p1:f64] → [qf:p2]
Blocks: 1488435
(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)
Attachment #9020394 - Attachment is obsolete: true
Attachment #9020395 - Attachment is obsolete: true
Attachment #9020396 - Attachment is obsolete: true
Attachment #9020397 - Attachment is obsolete: true
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.
Attachment #9020398 - Attachment is obsolete: true
Attachment #9020399 - Attachment is obsolete: true
Attachment #9020401 - Attachment is obsolete: true
Attachment #9020401 - Attachment is obsolete: false
Attachment #9020399 - Attachment is obsolete: false
Attachment #9020398 - Attachment is obsolete: false
Attachment #9020397 - Attachment is obsolete: false
Attachment #8887121 - Attachment is obsolete: true
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-
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)
Attachment #9020393 - Attachment is obsolete: true
Attachment #9021408 - Attachment description: Simplify Compartment::findOutgoingEdges → Patch 1 - Simplify Compartment::findOutgoingEdges
Attachment #9020397 - Attachment is obsolete: true
Attachment #9020398 - Attachment is obsolete: true
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)
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)
Attachment #9020399 - Attachment is obsolete: true
Attachment #9020400 - Attachment is obsolete: true
Attachment #9020401 - Attachment is obsolete: true
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)
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
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

Depends on D31959

Blocks: 1553332
Pushed by sfink@mozilla.com:
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

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

Flags: needinfo?(sphink)
Attachment #9067492 - Attachment description: Bug 1167452 - Make gcstate() return more information than just the incrementalState. → Bug 1167452 - Make gcstate() return more information than just the incrementalState. r=jonco
Pushed by sfink@mozilla.com:
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
Attachment #9067492 - Attachment description: Bug 1167452 - Make gcstate() return more information than just the incrementalState. r=jonco → Bug 1167452 - Add a currentgc() function that returns lots of info on the internal incremental GC state. r=jonco
Pushed by sfink@mozilla.com:
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
Regressions: 1556193
Regressions: 1514421
Regressions: 1556430
Regressions: 1556209
Regressions: 1556706
Pushed by sfink@mozilla.com:
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
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1557701
Regressions: 1556321

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

Status: RESOLVED → REOPENED
Flags: needinfo?(sphink)
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---

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.

Flags: needinfo?(sphink)

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.

Regressions: 1557283
Regressions: 1556933
See Also: → 1516173
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f27409c81df2
When verifying weakmap marking, consider whether key's zone is collecting r=jonco
Keywords: leave-open
Attachment #9129879 - Attachment is obsolete: true
Attachment #9129878 - Attachment is obsolete: true
Attachment #9129877 - Attachment is obsolete: true
Pushed by sfink@mozilla.com:
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
Blocks: 1633176

Resolving bug, actual activation happening in bug 1633176.

Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Backout by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91ec9a272383
Backed out incremental weakmap marking (bug 1167452 and bug 1633176) to postpone until after Fx77

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.

(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!

Performance Impact: --- → P2
Whiteboard: [qf:p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: