Doing this as a second step after making the marking indirect should make both changes easier to review. Until we actually do moving, we are safe simply asserting that we have not moved.

Values as keys: JSCompartment::markCrossCompartmentWrappers

More Values as keys: MapObject::mark SetObject::mark jsids as keys: WatchpointMap::mark{All|Iteratively} Objects as keys: WatchpointMap::mark{All|Iteratively}

More Objects as keys: js_TraceAtomState gc_sharp_table_entry_marker Debugger::markKeysInCompartment

More Objects as keys: js_TraceSharpMap

I've audited the HashMaps where we mark the keys. These are the key types: Map/SetObject | HashableValue -> HeapValue Watchpoint | WatchKey -> HeapPtrObject, HeapId Debugger | HeapPtrObject and HeapPtrScript WeakMap | HeapValue and HeapPtrObject and HeapPtrScript AtomSet | AtomStateEntry -> JSAtom*|bits (stored as uintptr_t) SharpObjectMap | JSObject*

> More Objects as keys: > js_TraceSharpMap That no longer exists, thankfully.

Here's a patch to address Debugger and AtomSet. The remaining classes look like they've been dealt with already.

Comment on attachment 713929 [details] [diff] [review] Proposed fix Review of attachment 713929 [details] [diff] [review]: ----------------------------------------------------------------- Good work figuring out what needed to be done from the bug descriptions alone: this stuff is just crazily complex. I did uncover one more problem very recently when doing the zone() rework that needs to be cleaned up in this code before we can commit it. I've left details inline -- please ping me if my description is inadequate. ::: js/src/jsatom.cpp @@ +186,5 @@ > js::MarkAtoms(JSTracer *trc) > { > JSRuntime *rt = trc->runtime; > + for (AtomSet::Enum e(rt->atoms); !e.empty(); e.popFront()) { > + AtomStateEntry entry = e.front(); I think it should be possible to make |entry| a const-ref, which will neatly solve the problem discussed in my other comment. ::: js/src/vm/Debugger.h @@ +112,5 @@ > + Key key = e.front().key; > + Key prior = key; > + gc::Mark(tracer, &key, "Debugger WeakMap key"); > + if (prior != key) > + e.rekeyFront(key); I discovered a new problem early this week that this new code will be susceptible to (see Bug 839673 and Bug 841065, although this instance is a bit different). The problem is that if |key| and |prior| are in the Nursery and Mark moves |key|, then |prior| will still be pointing into the Nursery. When we destruct |prior| at the loop end, the EncapsulatedPtr pre-barrier will run, which will try to access zone(), which will access shape_, the 1st word in the object, which will now be set to our relocation tag bits, not a valid shape pointer. You could solve this by setting |prior| to NULL before the loop ends, but I would prefer if we just rewrote this with one fewer temporaries. As such: Key key = e.front().key; Mark(tracer, &key, "..."); if (key != e.front().key) e.rekeyFront(key); key = NULL; At the loop end |key| will be pointing at the right object and we won't explode if we fail to set it to NULL, however, running the pre-barrier during GC is still far from ideal (and extra work). Thus, we want to NULL it anyway.

Also, since you're currently looking at it, could you verify that the other maps don't have this problem?

I updated the patch in line with your comments, except that I think doing 'key = NULL' will still trigger the barriers, so I did key.unsafeSet(NULL) instead. Along the same lines, I think that the hashtable implementation of reykeyFront will suffer from the same problem when used with a key of type EncapsulatedPtr, as it just assigns to it. I've got a possible fix for this which I'll post here too.

This adds a new operation to the hash policy to use when rekeying, so that tables using encapsulated pointers can use unsafeSet and not trigger barriers. I feel like this is exposing more implementation details that I would like though...

Rebase fix and extend to cover AutoObjectUnsigned32HashMap and AutoObjectHashSet.

Rebase and extend to cover FieldInfoHash in ctypes.

Terrence, do we want to land this? It's required for GGC, unless we have some other plan for handling hash tables.

Comment on attachment 789666 [details] [diff] [review] Bug726687-rekey Review of attachment 789666 [details] [diff] [review]: ----------------------------------------------------------------- Yes absolutely: sorry I let this linger for so long! I was thinking this patch only dealt with non-runtime marking paths (e.g. shrinking GC stuff), but I was clearly incorrect. I haven't seen any of these trigger on our test suites or benchmarks, but that is probably just hapstance.

(In reply to Terrence Cole [:terrence] from comment #15) Great, and the second patch too?

Comment on attachment 789667 [details] [diff] [review] rekey-no-barrier Review of attachment 789667 [details] [diff] [review]: ----------------------------------------------------------------- This is a nice first step towards getting rid of the global needsBarriers flag. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/3017a949cdae https://hg.mozilla.org/integration/mozilla-inbound/rev/61656d718678

https://hg.mozilla.org/mozilla-central/rev/3017a949cdae https://hg.mozilla.org/mozilla-central/rev/61656d718678