Closed
Bug 830839
Opened 11 years ago
Closed 11 years ago
Make rooting analysis ignore atom rooting
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file)
2.51 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Assignee: general → jcoppeard
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e201a9901088
Comment 6•11 years ago
|
||
I don't like the name IsAddressableGCThing with a void* thing pointer. This should be renamed to something like GetAddressableGCThing.
Comment 7•11 years ago
|
||
Good point. I'll file a follow-up.
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e201a9901088
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•