1502946 - Experiment with tightening up gray marking checks

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

Attachment: remove-some-gray-checks

We a bunch of APIs used in the browser to assert that things are not gray (JS::ObjectIsNotGray and related functions).

Internally we make allowances for the fact that the gray marking state may not be currently valid and in particular we have this code to check whether something that is marked gray now will become marked black because of a wrapper that's on the mark stack but hasn't been marked yet.  This situation would usually happen because of a write barrier when overwriting a pointer to a gray GC thing.

I added this a while ago but I now think this is bogus.  I can't imagine a situation where we rely on a write barrier to mark something black (as opposed to the read barrier where we absolutely do).  I'd like to experiment with removing this.

It's green on try, but it's possible that this will cause a ton of gray marking assertion failures, in which case we can back it out pronto.

Comment 1

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

Comment on attachment 9020836 [details] [diff] [review]
remove-some-gray-checks

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

Ok, so let me say that back.

At various times, we would like to assert that something that is exposed to browser code is not gray, because stuff exposed to browser code should never be gray. [Side note: if that's the only use, I wonder if the API should be called ExposedObjectIsNotGray to match up with ExposeObjectToActiveJS, especially after this patch when it really isn't telling you what its name says?] Previously, we thought that we could discover that the object in question *is* gray, but would eventually become black because it is reachable from something on the incremental mark stack. Things are added to the iGC mark stack by write barriers (and nothing else, right?) The idea here is that depending on a write barrier to fix up the grayness of an object you're holding is just really weird; why would you risk getting access to something like that? I guess the idea is that you grab one object (ungraying it), then traverse it to get to another object (without ungraying it). Or you get to an object via some unbarriered path (eg getting a global/incumbent global/some handler) off of a cx or something. We probably shouldn't be doing either of those things.

Or otherwise stated: previously, we were checking the invariant "exposed objects will not be gray by the time we get to CC". This patch simplifies the invariant to "exposed objects will not be gray", which is stronger, albeit we don't actually need that extra strength for correctness.

Sounds good to me, if it sticks. Can you verify my assumption above that between iGC slices, we'll be adding things to the mark stack only via write barriers?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
> At various times, we would like to assert that something that is exposed to
> browser code is not gray, because stuff exposed to browser code should never
> be gray. 

It's more that stuff exposed to the JS engine should never be gray because it might store a reference to it somewhere, and then the cycle collector might free a C++ objects that it has a pointer to and then you have a dangling pointer.

> Previously, we thought that we could discover that the object in
> question *is* gray, but would eventually become black because it is
> reachable from something on the incremental mark stack. Things are added to
> the iGC mark stack by write barriers (and nothing else, right?) 

Well, also by read barriers / ExposeToActiveJS.  But this unmarks gray after it marks the thing and pushes it onto the mark stack.  Gray unmarking doesn't happen incrementally, and isn't limited to marking zones.

One situation that can happen is that we have a white CCW pointing to a gray target in an uncollected zone.  The read barrier will mark the CCW black and push it on the stack but leave the target gray (until incremental marking gets to it).  But, we unmark gray when we get the target of a CCW for this reason.  So this check shouldn't be necessary.

(I guess I forgot about the read barrier case when I posted this patch.)

> The idea
> here is that depending on a write barrier to fix up the grayness of an
> object you're holding is just really weird; why would you risk getting
> access to something like that? I guess the idea is that you grab one object
> (ungraying it), then traverse it to get to another object (without ungraying
> it). Or you get to an object via some unbarriered path (eg getting a
> global/incumbent global/some handler) off of a cx or something. We probably
> shouldn't be doing either of those things.

Yes, and we should find any places we do and fix them.

Comment 3

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

(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Steve Fink [:sfink] [:s:] from comment #1)
> > At various times, we would like to assert that something that is exposed to
> > browser code is not gray, because stuff exposed to browser code should never
> > be gray. 
> 
> It's more that stuff exposed to the JS engine should never be gray because
> it might store a reference to it somewhere, and then the cycle collector
> might free a C++ objects that it has a pointer to and then you have a
> dangling pointer.

Ah, right, so you don't have to expose it to the embedding to get in trouble.

Ok, so in theory you could have a gray source object with a slot holding a gray target object. You get hold of the source object somehow and unmark gray on it. Then you read the target out of a slot and store it somewhere else in the heap.

It's not a problem in practice because when you unmark gray the source, you also push it onto the mark stack (and if you erase the edge between them, the pre barrier will mark the target black).

I guess the code after this patch still probably wouldn't assert, because unless there's a "read a slot, write it somewhere else, and return it" operation, then you still won't have a way to get your hands on the target without unmark greying it. (And if such an operation existed, then it ought to exposeToActiveJS anyway.)

> > Previously, we thought that we could discover that the object in
> > question *is* gray, but would eventually become black because it is
> > reachable from something on the incremental mark stack. Things are added to
> > the iGC mark stack by write barriers (and nothing else, right?) 
> 
> Well, also by read barriers / ExposeToActiveJS.  But this unmarks gray after
> it marks the thing and pushes it onto the mark stack.  Gray unmarking
> doesn't happen incrementally, and isn't limited to marking zones.

Oh, right. I wasn't really counting the unmark-gray stuff because it's a recursive marking, so does not create the situation where a black mark is lurking on the pending mark stack.

> One situation that can happen is that we have a white CCW pointing to a gray
> target in an uncollected zone.  The read barrier will mark the CCW black and
> push it on the stack but leave the target gray (until incremental marking
> gets to it).  But, we unmark gray when we get the target of a CCW for this
> reason.  So this check shouldn't be necessary.

Oh wow. Someday I need to start remembering that some zones will be uncollected.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/637be7630f26
Tighten up some gray marking checks r=sfink

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/637be7630f26