Closed Bug 1162303 Opened 9 years ago Closed 9 years ago

Simplify TenuringTracer's implementation

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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+
(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.
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.