Do not fire post-barriers during moving GC

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mem][qa-])

Attachments

(1 attachment)

Assignee

Description

6 years ago
The general problem here is when we have a barrier hidden behind a generic type and we use the generic type directly in the store buffer. When we do store buffer marking the generic type passes through the stack and inserts more things into the store buffer. There are two common failure modes here: (1) accidentally messing up the buffer iteration pointer and sending StoreBuffer::mark into an infinite loop and (2) accidentally upcasting Relocatable to Encapsulated and losing an unput.

This has bitten us enough times, both with new code and with changed code triggering the wrong behavior, that we should really just fix the underlying problem. There were only a handful of places that ended up needing attention after removing the generic HashTableWriteBarrierPost interface.

1) Debugger::liveScopes -> no barriers, so just a trivial generic barrier.
2) Debugger::proxiedScopes -> we know the type, so it is trivial to cast to unbarriered.
3) WeakMapObject -> again, we know the type so we can just cast out of it.
4) WeakMap -> we don't know the type here, so I added a template Unbarriered to automatically strip the HeapPtr.
5) MapObject/SetObject -> again we know the types; however, in this case we already have a totally custom generic buffer entry. This is quite a bit of code, but I don't think it is substantially more confusing that what is already there.

Note: the assertions enforcing this change are in bug 902174 and should land soon after this.
Attachment #8339471 - Flags: review?(jcoppeard)
Comment on attachment 8339471 [details] [diff] [review]
no_barriered_on_stack-v0.diff

Review of attachment 8339471 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

It would have been nice to somehow do this automatically, but like you said, I think it would have actually been more code.

::: js/src/vm/ScopeObject.cpp
@@ +1506,5 @@
> +     * This will automatically ensure that barriers do not fire during GC.
> +     */
> +    typedef WeakMap<JSObject *, JSObject *> UnbarrieredMap;
> +    typedef gc::HashKeyRef<UnbarrieredMap, JSObject *> Ref;
> +    if (key && !IsInsideNursery(rt, key))

I think this should be |IsInsideNursery(rt, key))| rather than |!IsInsideNursery(rt, key))|.
Attachment #8339471 - Flags: review?(jcoppeard) → review+
Assignee

Comment 2

6 years ago
(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8339471 [details] [diff] [review]
> ::: js/src/vm/ScopeObject.cpp
> @@ +1506,5 @@
> > +     * This will automatically ensure that barriers do not fire during GC.
> > +     */
> > +    typedef WeakMap<JSObject *, JSObject *> UnbarrieredMap;
> > +    typedef gc::HashKeyRef<UnbarrieredMap, JSObject *> Ref;
> > +    if (key && !IsInsideNursery(rt, key))
> 
> I think this should be |IsInsideNursery(rt, key))| rather than
> |!IsInsideNursery(rt, key))|.

O_O  Good catch! I'm surprised that didn't bite during testing; jit-tests --tbpl is still green after the change though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab7ece2fd805
https://hg.mozilla.org/mozilla-central/rev/ab7ece2fd805
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 949038

Updated

5 years ago
Whiteboard: [mem → [mem][qa-]
You need to log in before you can comment on or make changes to this bug.