Incrementalize weak marking phase
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
Performance Impact | medium |
People
(Reporter: sfink, Assigned: sfink)
References
(Blocks 2 open bugs)
Details
(Keywords: perf)
Attachments
(23 files, 25 obsolete files)
(deleted),
patch
|
decoder
:
feedback-
gkw
:
feedback-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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.
We should do this soon. Bug 1314828 shows that we're spending a lot of time in weak marking.
Assignee | ||
Comment 2•8 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•8 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
Version that passes shell tests, fails lots of mochitests
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 5•7 years ago
|
||
Version that passes shell tests, fails lots of mochitests
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 6•7 years ago
|
||
Version that passes shell tests, fails lots of mochitests
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 7•7 years ago
|
||
Version that passes shell tests, fails lots of mochitests
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 11•7 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.
Assignee | ||
Comment 12•7 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 | ||
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
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•7 years ago
|
Updated•7 years ago
|
Comment 16•7 years ago
|
||
Steve advised other high priority things will prevent this from getting into 60.
Updated•7 years ago
|
Comment 17•6 years ago
|
||
Steve, is there a chance you'll get to pick this up this year?
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 18•6 years 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.
Assignee | ||
Comment 19•6 years ago
|
||
Assignee | ||
Comment 20•6 years ago
|
||
Assignee | ||
Comment 21•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 22•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 25•6 years 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•6 years ago
|
Assignee | ||
Comment 26•6 years ago
|
||
Assignee | ||
Comment 27•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 28•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 30•6 years 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.
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
![]() |
||
Updated•6 years ago
|
Assignee | ||
Comment 33•6 years 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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 37•6 years ago
|
||
Comment on attachment 9021409 [details] [diff] [review] Patch 2 - Implement isMarked, IsForwarded, and Forwarded for GCCellPtr (sorry, messed up numbering)
Assignee | ||
Comment 38•6 years 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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 41•6 years 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-.
Assignee | ||
Updated•6 years ago
|
Comment 42•6 years ago
|
||
Comment on attachment 9021408 [details] [diff] [review] Patch 1 - Simplify Compartment::findOutgoingEdges Review of attachment 9021408 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Updated•6 years ago
|
Comment 43•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 46•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 50•6 years 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
Comment 51•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d1796e4fc907 https://hg.mozilla.org/mozilla-central/rev/5568f16f0191
Assignee | ||
Comment 52•5 years ago
|
||
Assignee | ||
Comment 62•5 years ago
|
||
Comment 63•5 years ago
|
||
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
Comment 64•5 years ago
|
||
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
Assignee | ||
Comment 65•5 years ago
|
||
Oops, I guess I never tested opt builds. Sorry about that.
Updated•5 years ago
|
Comment 66•5 years ago
|
||
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
Comment 67•5 years ago
|
||
bugherder |
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
Updated•5 years ago
|
Comment 68•5 years ago
|
||
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
Comment 69•5 years ago
|
||
bugherder |
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•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Comment 71•5 years ago
|
||
bugherder |
![]() |
||
Comment 72•5 years ago
|
||
Last changeset got backed. More information about the backout in bug 1514421.
Assignee | ||
Comment 73•5 years ago
|
||
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.
Assignee | ||
Comment 74•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 75•5 years ago
|
||
Backout by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad75b878827 Backed out changeset 617df479fac1
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 76•5 years ago
|
||
Comment 77•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Comment 78•5 years ago
|
||
bugherder |
Assignee | ||
Comment 79•5 years ago
|
||
Assignee | ||
Comment 80•5 years ago
|
||
Assignee | ||
Comment 81•5 years ago
|
||
Assignee | ||
Comment 82•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 83•5 years ago
|
||
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
Assignee | ||
Comment 84•5 years ago
|
||
Comment 85•5 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8bd2751e166 minor warning fix
Comment 86•5 years ago
|
||
bugherder |
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
Assignee | ||
Comment 87•4 years ago
|
||
Resolving bug, actual activation happening in bug 1633176.
Assignee | ||
Comment 88•4 years ago
|
||
Comment 89•4 years ago
|
||
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
Comment 91•4 years ago
|
||
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.
Assignee | ||
Comment 92•4 years ago
|
||
(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!
Updated•3 years ago
|
Description
•