Closed
Bug 1162303
Opened 9 years ago
Closed 9 years ago
Simplify TenuringTracer's implementation
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
15.18 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
Observation: markObject === DoTenuring(JSObject**) and markSlot === DoTenuring(Value*). Once you notice this you can't un-see how the struct of this algorithm is more or less identical to that of marking. Specifically: mark:move::push:insertIntoFixupList -- the rest is just details about the layouts of objects. We're still quite a ways from getting to this structure, but the first step is to collapse DoTenuring and markObject|Slot.
Attachment #8602392 -
Flags: review?(jcoppeard)
Comment 1•9 years ago
|
||
Comment on attachment 8602392 [details] [diff] [review] 22_moveObject_is_traverse-v0.diff Review of attachment 8602392 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, no major issues. ::: js/src/gc/Barrier.h @@ +823,5 @@ > , allowWrite_(allowWrite) > #endif > {} > > + operator const Value*() const { Should this be unsafeGet() like HeapSlot above? @@ +826,5 @@ > > + operator const Value*() const { > + JS_STATIC_ASSERT(sizeof(HeapValue) == sizeof(Value)); > + JS_STATIC_ASSERT(sizeof(HeapSlot) == sizeof(Value)); > + return (const Value*)array; We probably should be using reinterpret_cast here. ::: js/src/gc/Marking.cpp @@ +1730,2 @@ > void > +js::TenuringTracer::traverse(JSObject** objp) I think I preferred the previous situation of having one generic case that did nothing and specialisations for JSObject** and Value*, but it's no big deal. @@ +1756,5 @@ > +template <> void js::TenuringTracer::traverse(js::Shape**) {} > +template <> void js::TenuringTracer::traverse(JSString**) {} > +template <> void js::TenuringTracer::traverse(JS::Symbol**) {} > +template <> void js::TenuringTracer::traverse(js::ObjectGroup**) {} > +template <> void js::TenuringTracer::traverse(jsid*) {} It seems we lose the assertion from DoTenuring<jsid> here.
Attachment #8602392 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #1) > Comment on attachment 8602392 [details] [diff] [review] > 22_moveObject_is_traverse-v0.diff > > Review of attachment 8602392 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, no major issues. > > ::: js/src/gc/Barrier.h > @@ +823,5 @@ > > , allowWrite_(allowWrite) > > #endif > > {} > > > > + operator const Value*() const { > > Should this be unsafeGet() like HeapSlot above? It seems to be used in that form in quite a few places. At least it's const, so should be a little safe. > @@ +826,5 @@ > > > > + operator const Value*() const { > > + JS_STATIC_ASSERT(sizeof(HeapValue) == sizeof(Value)); > > + JS_STATIC_ASSERT(sizeof(HeapSlot) == sizeof(Value)); > > + return (const Value*)array; > > We probably should be using reinterpret_cast here. Done! > ::: js/src/gc/Marking.cpp > @@ +1730,2 @@ > > void > > +js::TenuringTracer::traverse(JSObject** objp) > > I think I preferred the previous situation of having one generic case that > did nothing and specialisations for JSObject** and Value*, but it's no big > deal. Yeah, I'm not sold on it either. I'm mostly worried about object-derived types getting ignored, since we don't dispatch through the external paths nearly as often here. > @@ +1756,5 @@ > > +template <> void js::TenuringTracer::traverse(js::Shape**) {} > > +template <> void js::TenuringTracer::traverse(JSString**) {} > > +template <> void js::TenuringTracer::traverse(JS::Symbol**) {} > > +template <> void js::TenuringTracer::traverse(js::ObjectGroup**) {} > > +template <> void js::TenuringTracer::traverse(jsid*) {} > > It seems we lose the assertion from DoTenuring<jsid> here. This was mostly an issue because jsid used to have the option of containing JSObject*, until Jason finally removed it when he added Symbols. Now that only JSString and JS::Symbol can be in a jsid, I feel much less concerned about not asserting that JSID_TO_GCTHING is not in the nursery.
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc4313407098
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2993c8d2fbb1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•