857345 - Don't UnmarkGray on nursery allocated things

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

They don't have mark bits and, indeed, cannot be gray: we do a minor collection at the start of each GC slice, so the gray marker should never be able to observe a gc thing in the nursery. The only tricky thing here is ensuring that we don't regress performance. 

Talos run at:
https://tbpl.mozilla.org/?tree=Try&rev=d0192b42ccd3

Comment 1

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

The Talos run was done without JSGC_GENERATIONAL set, so it wasn't really testing anything. Can you run it again without the ifdefs?

Comment 2

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #1)
> The Talos run was done without JSGC_GENERATIONAL set, so it wasn't really
> testing anything. Can you run it again without the ifdefs?

Right, I thought we wanted to ensure that flipping the order of the other tests doesn't regress, since switching on GGC is going to have so many other effects. It certainly won't hurt to re-test with the #ifdefs commented out, however: https://tbpl.mozilla.org/?tree=Try&rev=68864c0d9a36

Comment 3

[Terrence Cole [:terrence]]

No regression that I can see, even on my nemesis platform, WinXP.

Comment 4

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

Comment on attachment 732572 [details] [diff] [review]
v0

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

::: js/public/GCAPI.h
@@ +245,5 @@
> +     * each GC slice, so the gray marker never sees nursery things.
> +     *
> +     * This test is incorrect when running the post-barrier verifier, as it does
> +     * not check the verifier nursery. This is safe, however, since the verifier
> +     * uses only the tenured heap.

This second paragraph is confusing and doesn't really add anything IMO. Please remove it.

Comment 5

[Terrence Cole [:terrence]]

Green Talos runs at:
https://tbpl.mozilla.org/?tree=Try&rev=d0192b42ccd3
https://tbpl.mozilla.org/?tree=Try&rev=68864c0d9a36

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e621a3f5db98

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e621a3f5db98

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Is JSGC_GENERATIONAL defined correctly outside jseng?

Comment 8

[Terrence Cole [:terrence]]

(In reply to :Ms2ger from comment #7)
> Is JSGC_GENERATIONAL defined correctly outside jseng?

Not yet (at least to the best of my knowledge). However, these are also used inside spidermonkey, so they did receive testing. Adding --enable-gcgenerational to the top-level configure will be one of the first things I need to do once I start serious work on the browser.