817218 - Move UnmarkGray to the JS engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This would be nice to have for a couple reasons.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

The previous patch had some bugs.

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 687558 [details] [diff] [review]
patch v2

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

Thanks for fixing this!  Looks good.

::: js/src/gc/Marking.cpp
@@ +1544,5 @@
> +    UnmarkGrayTracer childTracer(tracer, kind == JSTRACE_SHAPE);
> +
> +    if (kind != JSTRACE_SHAPE) {
> +        JS_TraceChildren(&childTracer, thing, kind);
> +        MOZ_ASSERT(!childTracer.previousShape);

nit: JS_ASSERT

@@ +1555,5 @@
> +        return;
> +    }
> +
> +    do {
> +        MOZ_ASSERT(!GCThingIsMarkedGray(thing));

nit: JS_ASSERT

@@ +1567,5 @@
> +js::UnmarkGrayGCThingRecursively(void *thing, JSGCTraceKind kind)
> +{
> +    JS_ASSERT(kind != JSTRACE_SHAPE);
> +
> +    if (!GCThingIsMarkedGray(thing))

Did you drop the assert on (thing) because we're just going to assert/crash on it anyways in GCThingIsMarkedGray?

::: js/src/jsfriendapi.h
@@ +272,5 @@
> +extern JS_FRIEND_API(bool)
> +AreGCGrayBitsValid(JSRuntime *rt);
> +
> +extern JS_FRIEND_API(void)
> +UnmarkGrayGCThingRecursively(void *thing, JSGCTraceKind kind);

Please add a comment here saying that this should only be called on non-null non-shapes.

@@ +277,3 @@
>  
>  typedef void
> +(*GCThingCallback)(void *closure, void *gcthing);

Oops!

::: js/src/jsgc.cpp
@@ +3520,5 @@
>              rt->gcFinalizeCallback(&fop, JSFINALIZE_COLLECTION_END, !isFull);
> +
> +        /* If we finished a full GC, then the gray bits are correct. */
> +        if (isFull)
> +            rt->gcGrayBitsValid = true;

We'll have to keep an eye on unmark gray overflow GCs, as this is going to reset less often than the current code, which looks like it resets at the end of any GC. Your new code does have the advantage of being correct.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce24dbcb88b3

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Followup for a problem introduced while merging:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ced60655d5

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce24dbcb88b3
https://hg.mozilla.org/mozilla-central/rev/d6ced60655d5

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: patch for beta/esr17

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 705642 [details] [diff] [review]
patch for beta/esr17

This didn't change that much, but I'd still appreciate it if you did a high-level scan to see if the places functions are, etc. seem sensible.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 705642 [details] [diff] [review]
patch for beta/esr17

Hmm.  Doesn't seem to build for some reason.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: patch for beta/esr17

I guess my local build failed to pick up the change to Marking.cpp, as when I manually rebuilt it, it also failed. Anyways, hopefully this attempt will be better.
https://tbpl.mozilla.org/?tree=Try&rev=1e4070b9ab2f

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 705861 [details] [diff] [review]
patch for beta/esr17

Does this look okay?  I mostly just moved functions around.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 705861 [details] [diff] [review]
patch for beta/esr17

Yeah, it seems good to me.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 705861 [details] [diff] [review]
patch for beta/esr17

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed to land bug 690970.
Bug caused by (feature/regressing bug #): N/A
User impact if declined: possible security problems from 690970
Testing completed: this has been on trunk since 20
Risk to taking this patch (and alternatives if risky): should be low, it is mostly just moving code around.
String or UUID changes made by this patch: none

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 705861 [details] [diff] [review]
patch for beta/esr17

Approving as this is needed for 690970 already approved on branches & has been well tested on 20.

Please land no later than EOD today/tomorrow morning to make it into 19.0b4.

Comment 14

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8e727004e1e3

Comment 15

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/a3720a409579

Comment 16

[Andrew McCreight [:mccr8]]

Followup patch for bustage...
https://hg.mozilla.org/releases/mozilla-esr17/rev/82941f956b85

Comment 17

[Andrew McCreight [:mccr8]]

I didn't land this on b2g18 yet because 690970 needs some other bug before it can land.

Comment 18

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0adc3b450950