Make rooting analysis ignore atom rooting

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 702366 [details] [diff] [review]
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.
(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?
(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 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.
Attachment #702366 - Flags: review+
Duplicate of this bug: 830581
(Assignee)

Updated

5 years ago
Assignee: general → jcoppeard
Status: NEW → ASSIGNED
I don't like the name IsAddressableGCThing with a void* thing pointer. This should be renamed to something like GetAddressableGCThing.
Good point. I'll file a follow-up.

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e201a9901088
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.