830839 - Make rooting analysis ignore atom rooting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: Fix

Bill pointed out that atoms are not going to be subject to generational GC, so there's no point rooting them all.  (It's possible this may change in the future if we implement moving GC).

Here's a patch to do that.  This fixes all the remaining rooting analysis test failures that I can see.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Jon Coppeard (:jonco) from comment #0)
> Bill pointed out that atoms are not going to be subject to generational GC,
> so there's no point rooting them all.  (It's possible this may change in the
> future if we implement moving GC).

Isn't a moving GC needed for implementing a generational GC?

Comment 2

[Terrence Cole [:terrence]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> (In reply to Jon Coppeard (:jonco) from comment #0)
> > Bill pointed out that atoms are not going to be subject to generational GC,
> > so there's no point rooting them all.  (It's possible this may change in the
> > future if we implement moving GC).
> 
> Isn't a moving GC needed for implementing a generational GC?

Well, GGC is a type of moving GC. It just matters what you consider for moving and where you allocate the thing.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 702366 [details] [diff] [review]
Fix

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

I didn't see an r? on this bug in my bugmail, so I went and accidentally crafted basically the same patch. I like yours better, however, so r+ with the one comment addressed.

::: js/src/gc/Verifier.cpp
@@ +56,4 @@
>          return;
> +
> +    /* Don't check atoms as these will never be subject to generational collection. */
> +    if (reinterpret_cast<Cell *>(thing)->compartment() == rt->atomsCompartment)

You should be able to use |IsAtomsCompartment| here.

Comment 5

[Jon Coppeard (:jonco)]

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

Comment 6

[Tom S [:evilpie]]

I don't like the name IsAddressableGCThing with a void* thing pointer. This should be renamed to something like GetAddressableGCThing.

Comment 7

[Terrence Cole [:terrence]]

Good point. I'll file a follow-up.

Comment 8

[Ed Morley [:emorley]]

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