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)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: terrence, Assigned: jonco)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Proposed fix (obsolete) — Splinter Review
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)
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+
Also, since you're currently looking at it, could you verify that the other maps don't have this problem?
Attached patch Updated fix (obsolete) — Splinter Review
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)
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)
Blocks: 650161
No longer blocks: 720522
Target Milestone: --- → mozilla16
Version: Trunk → Other Branch
Attached patch Bug726687-rekeySplinter Review
Rebase fix and extend to cover AutoObjectUnsigned32HashMap and AutoObjectHashSet.
Attachment #714429 - Attachment is obsolete: true
Attachment #714429 - Flags: review?(terrence)
Attached patch rekey-no-barrierSplinter Review
Rebase and extend to cover FieldInfoHash in ctypes.
Attachment #789666 - Attachment is obsolete: true
Attachment #714436 - Attachment is obsolete: true
Attachment #714436 - Flags: review?(terrence)
Attachment #789666 - Attachment is obsolete: false
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)
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+
Flags: needinfo?(terrence)
(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
Attachment #789667 - Flags: review+
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.

Attachment

General

Created:
Updated:
Size: