Closed Bug 1338623 Opened 3 years ago Closed 3 years ago

Investigate making gray marking assertions run all the time again

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch bug1338623-refactor-mark-stack (obsolete) — Splinter Review
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.
Attachment #8837687 - Flags: review?(sphink)
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).
Attachment #8837691 - Flags: review?(sphink)
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.
Attachment #8837687 - Flags: review?(sphink) → review+
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?)
Attachment #8837691 - Flags: review?(sphink) → review+
(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.
Depends on: 1341355
Splitting off bug 1341355 so I can land these patches.
Attachment #8837687 - Attachment is obsolete: true
Attachment #8837691 - Attachment is obsolete: true
Attached patch bug1338623-improve-gray-asserts (obsolete) — Splinter Review
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.
Attachment #8842078 - Flags: review?(sphink)
Comment on attachment 8842078 [details] [diff] [review]
bug1338623-improve-gray-asserts

Requesting review for gecko changes.
Attachment #8842078 - Flags: review?(continuation)
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
Attachment #8842078 - Flags: review?(continuation) → review+
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?
Attachment #8842078 - Flags: review?(sphink) → review+
(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!
(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.
Updated patch following review comments.
Attachment #8842503 - Flags: review+
I found another source of cross-compartment edges to check - the debugger.
Attachment #8842504 - Flags: review?(sphink)
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.
Keywords: leave-open
Pushed by jcoppeard@mozilla.com:
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
Attachment #8842078 - Attachment is obsolete: true
Attachment #8842503 - Flags: checkin+
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.
Attachment #8842933 - Flags: review?(sphink)
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.
Attachment #8842504 - Flags: review?(sphink) → review+
Attachment #8842933 - Flags: review?(sphink) → review+
(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.
Pushed by jcoppeard@mozilla.com:
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/237caed7236d
https://hg.mozilla.org/mozilla-central/rev/ed21480d66e6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(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.
Not fixed on esr52, bug 1352373 landed on esr52 with this bug in the commit message.
You need to log in before you can comment on or make changes to this bug.