Investigate making gray marking assertions run all the time again

RESOLVED FIXED in Firefox -esr52

Status

()

Core
JavaScript: GC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 fixed, firefox54 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8837687 [details] [diff] [review]
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.
Attachment #8837687 - Flags: review?(sphink)
(Assignee)

Comment 2

a year ago
Created attachment 8837691 [details] [diff] [review]
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).
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+
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Updated

a year ago
Depends on: 1341355
(Assignee)

Comment 6

a year ago
Splitting off bug 1341355 so I can land these patches.
(Assignee)

Updated

a year ago
Attachment #8837687 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8837691 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
Created attachment 8842078 [details] [diff] [review]
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.
Attachment #8842078 - Flags: review?(sphink)
(Assignee)

Comment 8

a year ago
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+
(Assignee)

Comment 11

a year ago
(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!
(Assignee)

Comment 12

a year ago
(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.
(Assignee)

Comment 13

a year ago
Created attachment 8842503 [details] [diff] [review]
bug1338623-improve-gray-asserts v2

Updated patch following review comments.
Attachment #8842503 - Flags: review+
(Assignee)

Comment 14

a year ago
Created attachment 8842504 [details] [diff] [review]
bug1338623-include-debugger

I found another source of cross-compartment edges to check - the debugger.
Attachment #8842504 - Flags: review?(sphink)
(Assignee)

Comment 15

a year ago
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

Comment 16

a year ago
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
(Assignee)

Updated

a year ago
Attachment #8842078 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8842503 - Flags: checkin+
(Assignee)

Comment 17

a year ago
Created attachment 8842933 [details] [diff] [review]
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.
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+
(Assignee)

Comment 20

a year ago
(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

a year ago
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
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/237caed7236d
https://hg.mozilla.org/mozilla-central/rev/ed21480d66e6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/edcafd42dd52
status-firefox-esr52: --- → fixed
(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.
You need to log in before you can comment on or make changes to this bug.