Closed
Bug 726687
Opened 12 years ago
Closed 11 years ago
GC: rewrite key marking in terms of HashMap::rekey
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: terrence, Assigned: jonco)
References
Details
Attachments
(2 files, 3 obsolete files)
6.59 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
11.17 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
Values as keys: JSCompartment::markCrossCompartmentWrappers
Reporter | ||
Comment 2•12 years ago
|
||
More Values as keys: MapObject::mark SetObject::mark jsids as keys: WatchpointMap::mark{All|Iteratively} Objects as keys: WatchpointMap::mark{All|Iteratively}
Reporter | ||
Comment 3•12 years ago
|
||
More Objects as keys: js_TraceAtomState gc_sharp_table_entry_marker Debugger::markKeysInCompartment
Reporter | ||
Comment 4•12 years ago
|
||
More Objects as keys: js_TraceSharpMap
Reporter | ||
Comment 5•12 years ago
|
||
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*
Comment 6•11 years ago
|
||
> More Objects as keys:
> js_TraceSharpMap
That no longer exists, thankfully.
Assignee | ||
Comment 7•11 years ago
|
||
Here's a patch to address Debugger and AtomSet. The remaining classes look like they've been dealt with already.
Assignee: terrence → jcoppeard
Attachment #713929 -
Flags: review?(terrence)
Reporter | ||
Comment 8•11 years ago
|
||
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.
Attachment #713929 -
Flags: review?(terrence) → review+
Reporter | ||
Comment 9•11 years ago
|
||
Also, since you're currently looking at it, could you verify that the other maps don't have this problem?
Assignee | ||
Comment 10•11 years ago
|
||
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.
Attachment #713929 -
Attachment is obsolete: true
Attachment #714429 -
Flags: review?(terrence)
Assignee | ||
Comment 11•11 years ago
|
||
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...
Attachment #714436 -
Flags: review?(terrence)
Reporter | ||
Updated•11 years ago
|
Assignee | ||
Comment 12•11 years ago
|
||
Rebase fix and extend to cover AutoObjectUnsigned32HashMap and AutoObjectHashSet.
Attachment #714429 -
Attachment is obsolete: true
Attachment #714429 -
Flags: review?(terrence)
Assignee | ||
Comment 13•11 years ago
|
||
Rebase and extend to cover FieldInfoHash in ctypes.
Attachment #789666 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #714436 -
Attachment is obsolete: true
Attachment #714436 -
Flags: review?(terrence)
Assignee | ||
Updated•11 years ago
|
Attachment #789666 -
Attachment is obsolete: false
Assignee | ||
Comment 14•11 years ago
|
||
Terrence, do we want to land this? It's required for GGC, unless we have some other plan for handling hash tables.
Flags: needinfo?(terrence)
Reporter | ||
Comment 15•11 years ago
|
||
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.
Attachment #789666 -
Flags: review+
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(terrence)
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #15) Great, and the second patch too?
Reporter | ||
Comment 17•11 years ago
|
||
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
Attachment #789667 -
Flags: review+
Assignee | ||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3017a949cdae https://hg.mozilla.org/integration/mozilla-inbound/rev/61656d718678
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3017a949cdae https://hg.mozilla.org/mozilla-central/rev/61656d718678
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•