1309664 - js::ZoneGlobalsAreAllGray should not call UnmarkGray()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

This method iterates over all compartments in a zone, and checks if all of the zone globals are gray, in order to do a special CC optimization. I think due to some of the unmark gray barrier work, this now ends up calling UnmarkGray on the global, which is obviously a bad idea.

One way to fix this would be to add a globalIsMarkedGray() method to JSCompartment that does not do the unmarkgray. I'm not sure what the right way to do that, as JSCompartment::global_ has the odd type js::ReadBarrieredGlobalObject. I don't know what other read barriers the GC might need.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Add a questionable globalIsMarkedGray method on JSCompartment.

This at least works well enough to run XPConnect mochitests.

Comment 2

[Andrew McCreight [:mccr8]]

Do you know if this read barriered global thing does anything aside from unmark gray? From reading the code I can't really tell. It was added in bug 1024250.

Comment 3

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

There's nothing else going on. The global isn't going to escape or anything, so I think the indirection of globalIsMarkedGray is unnecessary. r=me on a patch where ZoneGlobalsAreAllGray just calls unsafeUnbarrieredMaybeGlobal directly. I'm not even sure why the scary "unsafe" is needed on that name, but never mind that.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> There's nothing else going on. The global isn't going to escape or anything,
> so I think the indirection of globalIsMarkedGray is unnecessary.

I guess that's true. I was just trying to encapsulate things a little more in case of future change, but I suppose it is clear enough why you want to avoid the barrier.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1309664 - Don't unmark gray globals in ZoneGlobalsAreAllGray.

Calling unmark gray will make the method always return false.

Review commit: https://reviewboard.mozilla.org/r/85316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85316/

Comment 6

[Andrew McCreight [:mccr8]]

No need to test on try, as we conveniently never run this code right now.

Comment 7

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

Comment on attachment 8800402 [details]
Bug 1309664 - Don't unmark gray globals in ZoneGlobalsAreAllGray.

https://reviewboard.mozilla.org/r/85316/#review83900

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31de330b852f
Don't unmark gray globals in ZoneGlobalsAreAllGray. r=sfink

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31de330b852f