1405064 - AutoWrapperVector/AutoWrapperRooter comment is confusing

Status: ASSIGNED

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

Description

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

http://searchfox.org/mozilla-central/source/js/src/jscompartment.h#1363-1370

Comment 1

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

Attachment: Attempted comment improvement for AutoWrapper{Vector,Rooter}

Then again, I don't know how to fix it. I'm also unsure as to the exact reason why running every slice is necessary; I assume it's something to do with zone mark/sweep groups? But I'm not sure; is this specific to transplanting, or is it a general need of any CCW?

But that aside, the comment seems to be saying (1) you use this to get every-slice marking (more marking!) and (2) you use this to avoid unwanted marking (less marking!), and it's challenging to work out why each of those is correct and needed.

My update adds a bunch more verbiage, but it makes it kind of specific to transplanting (which is, admittedly, the only place these are used). And I don't know if I really clarified anything.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8914431 [details] [diff] [review]
Attempted comment improvement for AutoWrapper{Vector,Rooter}

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

Oh wow this is pretty complicated.  I never really understood this fully before but here's what I make of it after reading this a bunch of times:

The wrapper map contains weak references to wrappers.  They are not marked as roots during collection and if they are unmarked when we come to the sweep phase they are removed from the map and destroyed.  We have a read barrier so that if we read one during an incremental GC then we mark it to prevent it being swept as we may subsequently use it, either directly or by writing a pointer to it into a live object in the heap.  (So far this is just the normal way of dealing with weak references.)

Note that this read barrier is conservative - we may not write the pointer into the heap or in fact use it at all.  Also, it has a cost in that it keeps alive anything we touch during an incremental GC.

For wrapper manipulation this cost is a problem because we often touch wrappers for compartments that are actually dead, and we don't want these compartments to stay alive just because we did this.  In this case we know that we are never going to write the wrapper pointer into the heap, so a read barrier is not necessarily required.  However we do keep these object pointers in registers or on the stack while we manipulate them so we need to ensure they don't get swept while we're doing this.  Therefore we do need to mark then if the GC runs as it could potentially destroy them.

So effectively this is a conditional and delayed read barrier on the wrapper map.  We skip the normal conservative barrier and only execute it if we really need to.

We don't need to mark a wrapper more than once in any GC, but it doesn't hurt to do so and the chance of running more than once GC slice while manipulating the same set of wrappers is probably very small.  We do need to ensure all wrappers that are currently being manipulated and haven't yet been marked due to the skipped read barrier do get marked however, so for simplicity we mark 
all of these on every slice.

Comment 3

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

(In reply to Jon Coppeard (:jonco) from comment #2)
> So effectively this is a conditional and delayed read barrier on the wrapper
> map.  We skip the normal conservative barrier and only execute it if we
> really need to.

Ok, fair enough.

> We don't need to mark a wrapper more than once in any GC, but it doesn't
> hurt to do so and the chance of running more than once GC slice while
> manipulating the same set of wrappers is probably very small.  We do need to
> ensure all wrappers that are currently being manipulated and haven't yet
> been marked due to the skipped read barrier do get marked however, so for
> simplicity we mark 
> all of these on every slice.

Right, but I guess what I'm struggling with is why AutoWrapperRooter is needed at all. Rooted isn't barriered, is it? Why couldn't you just use Rooted in place of AutoWrapperRooter? You would need to avoid the read barrier when getting the wrapper from the wrapper map, but that's no different with AutoWrapperRooter. Perhaps it's done this way purely because the read-barrier avoidance is done within WrapperValue, and you don't want to expose the unsafeGet()? If that's the case, then we could just add a trace() to WrapperValue and use Rooted<WrapperValue>.

AutoWrapperVector is a different story. It makes more sense to me. Although now that I'm saying that, I'm wondering if it could be a Rooted<GCVector<WrapperValue>> or if we don't need WrapperValue, a direct Rooted<GCVector<Value>>.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Why couldn't you just use Rooted in place of AutoWrapperRooter?

Because it only gets marking in the first GC slice.  We may start manipulating a wrapper after this has happened, but we still need it to get marked.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #4)
"it" being the Rooted in the first sentence above.  And /s/marking/marked/.  Sometimes I really wish you could edit your comments.