1338623 - Investigate making gray marking assertions run all the time again

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Back in bug 1335117 I made gray marking assertions not check during incremental GC (and when the cell in question was in an uncollected zone).  The reason for this is that in that situation the object may not have been marked black yet, even though it will be eventually.

bz pointed out that we're often in the middle of an incremental GC and that this will make the asserts check much less often.

It should be possible to address this another way, by checking to see if the object has a cross-compartment edge from an object that is on the mark stack.  This involves a little work so we could only do that when we are checking the gray state so we can assert it is correct.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1338623-refactor-mark-stack

Tidy up the mark stack a little - MarkStack itself moves into the js::gc namespace, the tag enum moves into MarkStack and various inline methods that are only used in Marking.cpp move into that file instead of the header.  The methods that push tagged pointers can now deduce the tag required from the argument type.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1338623-type-mark-stack-and-add-iterator

This adds more abstraction for the mark stack and allows us have much better assertions that it is being used correctly.  The mark stack is now (mostly) an array of MarkStack::TaggedPtr and there's a MarkStackIter to iterate without removing elements (separate from the interface used during processMarkStackTop that does remove elements).

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8837687 [details] [diff] [review]
bug1338623-refactor-mark-stack

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

Nice!

::: js/src/gc/Marking.h
@@ +116,5 @@
> +    MOZ_MUST_USE bool push(T* ptr);
> +    MOZ_MUST_USE bool push(uintptr_t item1, uintptr_t item2, uintptr_t item3);
> +
> +    // GCMarker::eagerlyMarkChildren uses unused marking stack as storage.
> +    MOZ_MUST_USE bool pushTempRope(JSRope* ptr);

I'm not sure I understand the connection between the comment and the method.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8837691 [details] [diff] [review]
bug1338623-type-mark-stack-and-add-iterator

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

Phew. That's a lot of bit twiddling consolidation.

::: js/src/gc/Marking.h
@@ +181,5 @@
> +};
> +
> +class MarkStackIter
> +{
> +    MarkStack& stack_;

Could this be a ref to a const MarkStack?

@@ +185,5 @@
> +    MarkStack& stack_;
> +    MarkStack::TaggedPtr* pos_;
> +
> +  public:
> +    explicit MarkStackIter(MarkStack& stack);

Maybe guard against the stack getting realloced while an iter is active? Assert that pos_ is still within the stack during destruction, or something? (Or make it stronger, and register the count of live iterators with the stack, and assert that it is zero in ensureSpace?)

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> I'm not sure I understand the connection between the comment and the method.

Yeah, it was pretty cryptic.  I'll expand it.

> Maybe guard against the stack getting realloced while an iter is active?

That's a good idea, I'll add an iterator count.

Comment 6

[Jon Coppeard (:jonco)]

Splitting off bug 1341355 so I can land these patches.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1338623-improve-gray-asserts

This adds API functions to check that something is not marked gray (in debug builds only) and changes gray marking assertions in gecko to call these instead.

The implementation takes account of the fact that the targets of CCWs on the mark stack will end up getting marked black, so that we can make this check even during an incremental GC.

In addition it tidies up some of the internal gray marking checks.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8842078 [details] [diff] [review]
bug1338623-improve-gray-asserts

Requesting review for gecko changes.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8842078 [details] [diff] [review]
bug1338623-improve-gray-asserts

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

nit: "bug more eaxact" in patch description.

Can you make CheckObjectIsNotMarkedGray into ObjectIsNotMarkedGray, returning a bool and keep the MOZ_ASSERTs? It really is not obvious looking at the code that they are no-ops in opt builds. Or maybe if the name was AssertObjectIsNotMarkedGray that would be enough.

r=me for the browser changes

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8842078 [details] [diff] [review]
bug1338623-improve-gray-asserts

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

r=me modulo the stackContainsCrossCompartmentPointerTo thing.

::: js/public/RootingAPI.h
@@ +323,5 @@
>      return ObjectIsMarkedGray(obj.unbarrieredGet());
>  }
>  
> +MOZ_ALWAYS_INLINE void
> +CheckCellIsNotMarkedGray(js::gc::Cell* maybeCell)

Personally, I'd probably go for an overloaded

  IsNotGray(Cell*)
  IsNotGray(Value)
  ...

and keep the MOZ_ASSERT as mccr8 said. Heck, I'd even prefer if these were both inline and emitted out-of-line for use in gdb. ("IsNotGray" to distinguish from the existing "*IsMarkedGray" because we're not just looking at the mark color, but asking whether the derivable current state is that it is not CC-gray.)

That may seem too cryptic, so it's up to you. I suppose you could use WillNotBeGray or WillNotBeGrayDuringCC.

@@ +337,2 @@
>  {
> +    CheckCellIsNotMarkedGray(reinterpret_cast<js::gc::Cell*>(maybeObj));

Huh? Why is the cast even needed here? JSObject directly inherits from Cell.

Though if you go for the overload, then you'd need the cast, but it still seems like it could be a static_cast.

::: js/src/gc/Marking.cpp
@@ +2569,5 @@
>  }
>  
> +#ifdef DEBUG
> +bool
> +GCMarker::stackContainsCrossCompartmentPointerTo(const Cell* target) const

The name is a little bit of a lie; this is really stackContainsProxyTargetOf(target).

Is that what you need here? eg, if you had a same-compartment Proxy with the given target, couldn't this erroneously return true?

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #10)

Thanks for the feedback.  I'll change this back to using asserts - I agree that this makes it more obvious what's happening.  I did it like this so people wouldn't mistakenly call the slower check when they meant to query the mark state.

> @@ +337,2 @@
> >  {
> > +    CheckCellIsNotMarkedGray(reinterpret_cast<js::gc::Cell*>(maybeObj));
> 
> Huh? Why is the cast even needed here? JSObject directly inherits from Cell.

We don't know that yet though, because RootingAPI.h is indirectly included in jsobj.h before the class definition.

> The name is a little bit of a lie; this is really
> stackContainsProxyTargetOf(target).
> 
> Is that what you need here? eg, if you had a same-compartment Proxy with the
> given target, couldn't this erroneously return true?

Oh, good catch!

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #11)
Thinking more about this, what is important is that we detect the case where TraceCrossCompartmentEdge / ShouldTraceCrossCompartment will call unmark gray on the target.  ProxyObject's trace hook always calls TraceCrossCompartmentEdge on its target.  I'm going to rename this to stackContainsCrossZonePointerTo and make it check the zone is different as well.

Comment 13

[Jon Coppeard (:jonco)]

Attachment: bug1338623-improve-gray-asserts v2

Updated patch following review comments.

Comment 14

[Jon Coppeard (:jonco)]

Attachment: bug1338623-include-debugger

I found another source of cross-compartment edges to check - the debugger.

Comment 15

[Jon Coppeard (:jonco)]

I'm going to mark this leave-open and check in the first patch now.  I'll have third (hopefully final) patch to post later toady.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a927aeb8fb3
Add a slower but more exact gray marking check for checking correctness r=sfink r=mccr8

Comment 17

[Jon Coppeard (:jonco)]

Attachment: bug1338623-use-gray-assertions-everywhere

Use the new function in all the places we assert things aren't gray.  I thought I'd changed all of them already but it looks like I missed some.

I'm still thinking about renaming all the functions to the same thing and using overloading.  The reason I haven't so far is because I'm not sold on any of the names I can come up with so far.  I think 'IsNotGray' is too short and not descriptive enough but the alternative is probably GCThingIsNotGray but that sounds clunky.  I should probably go with that though.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a927aeb8fb3

Comment 19

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8842504 [details] [diff] [review]
bug1338623-include-debugger

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

::: js/src/vm/Debugger.cpp
@@ +11719,5 @@
> +{
> +    MOZ_ASSERT(target);
> +
> +    auto cls = obj->getClass();
> +    const void* referent = nullptr;

Can this be a const Cell* instead?

@@ +11725,5 @@
> +        referent = GetScriptReferentCell(obj);
> +    } else if (cls == &DebuggerSource_class) {
> +        referent = GetSourceReferentRawObject(obj);
> +    } else if (cls == &DebuggerObject::class_) {
> +        referent = obj->as<NativeObject>().getPrivate();

obj->as<DebuggerObject>().referent()

@@ +11727,5 @@
> +        referent = GetSourceReferentRawObject(obj);
> +    } else if (cls == &DebuggerObject::class_) {
> +        referent = obj->as<NativeObject>().getPrivate();
> +    } else if (cls == &DebuggerEnvironment::class_) {
> +        referent = obj->as<NativeObject>().getPrivate();

obj->as<DebuggerEnvironment>().referent(). Note that this returns an Env* which is currently a typedef of JSObject, but at least at one point it was considered that it should really be something different. If referent were a Cell*, then you'd get a compile error if it changed.

Comment 20

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #19)

> Can this be a const Cell* instead?

Yes, that's better.

> obj->as<DebuggerObject>().referent()
> obj->as<DebuggerEnvironment>().referent(). 

These are private unfortunately.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/237caed7236d
Use IsNotGray in all gray marking assertions r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed21480d66e6
Don't assert on gray debugger proxy targets that will eventaully become black r=sfink

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/237caed7236d
https://hg.mozilla.org/mozilla-central/rev/ed21480d66e6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/edcafd42dd52

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> https://hg.mozilla.org/releases/mozilla-esr52/rev/edcafd42dd52

This came by way of bug 1352373.

Comment 25

[Jon Coppeard (:jonco)]

Not fixed on esr52, bug 1352373 landed on esr52 with this bug in the commit message.