Closed Bug 863808 Opened 7 years ago Closed 7 years ago

Implement a store buffer for marking whole objects at once

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v0 (obsolete) — Splinter Review
Brian's research in Bug 851057 found that it was useful to be able to add a whole object to be marked during minor GC, rather than just one slot at a time. This patch uses the existing infrastructure to implement such a buffer, including support for the post-barrier verifier.
Attachment #739707 - Flags: review?(wmccloskey)
Attachment #739707 - Flags: feedback?(bhackett1024)
Comment on attachment 739707 [details] [diff] [review]
v0

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

The abstraction for accumulateEdges seems weird. I think it would be better if the verifier kept track of the EdgeSet. It would just ask the StoreBuffer to mark itself using the same methods that the nursery does. It would pass a JSTracer that would record the location in the EdgeSet. Then we wouldn't need any of the accumulateEdges code. Would that work?

It's fine if you don't want to to that right away. Please fix the stuff below though.

::: js/src/gc/StoreBuffer.cpp
@@ +196,5 @@
>  }
>  
> +namespace js {
> +namespace gc {
> +class AccumulateEdgesTracer : public JSTracer

Why does this have to be friended to anything? Is it just because EdgeSet is private? If so, that should be public.

::: js/src/gc/StoreBuffer.h
@@ +379,5 @@
> +        bool operator!=(const WholeObjectEdges &other) const { return tenured != other.tenured; }
> +
> +        template <typename NurseryType>
> +        bool inRememberedSet(NurseryType *nursery) const {
> +            return !IsInsideNursery(nursery->runtime(), tenured);

Why don't you just return true here? Then you don't need to expose the runtime() method. Also, calling the field |tenured| kind of suggests that it's tenured anyway. Could we just assert that in the constructor (using the isTenured method on Cell)?
Attachment #739707 - Flags: review?(wmccloskey)
Attached patch v1 (obsolete) — Splinter Review
(In reply to Bill McCloskey (:billm) from comment #1)
> The abstraction for accumulateEdges seems weird. I think it would be better
> if the verifier kept track of the EdgeSet. It would just ask the StoreBuffer
> to mark itself using the same methods that the nursery does. It would pass a
> JSTracer that would record the location in the EdgeSet. Then we wouldn't
> need any of the accumulateEdges code. Would that work?

I think so. I definitely need to sit down with the post barrier analysis and give it some quality time before I throw it at the browser. I hope we can also use the same technique to get rid of the insufferable |match| method of BufferableRef.
 
> It's fine if you don't want to to that right away. Please fix the stuff
> below though.
> 
> ::: js/src/gc/StoreBuffer.cpp
> @@ +196,5 @@
> >  }
> >  
> > +namespace js {
> > +namespace gc {
> > +class AccumulateEdgesTracer : public JSTracer
> 
> Why does this have to be friended to anything? Is it just because EdgeSet is
> private? If so, that should be public.

Yes. I've moved EdgeSet it out of StoreBuffer for the moment, since it's used in non StoreBuffer classes now.

> ::: js/src/gc/StoreBuffer.h
> @@ +379,5 @@
> > +        bool operator!=(const WholeObjectEdges &other) const { return tenured != other.tenured; }
> > +
> > +        template <typename NurseryType>
> > +        bool inRememberedSet(NurseryType *nursery) const {
> > +            return !IsInsideNursery(nursery->runtime(), tenured);
> 
> Why don't you just return true here? Then you don't need to expose the
> runtime() method. Also, calling the field |tenured| kind of suggests that
> it's tenured anyway. Could we just assert that in the constructor (using the
> isTenured method on Cell)?

Good point. I was torn on whether we should enforce the |isTenured| requirement just because the JITs do, but it will be much easier to just relax the constraint later, if it turns out we really need it.
Attachment #739707 - Attachment is obsolete: true
Attachment #739707 - Flags: feedback?(bhackett1024)
Attachment #745414 - Flags: review?(wmccloskey)
Attachment #745414 - Flags: review?(wmccloskey) → review+
Attachment #747050 - Flags: review+
Attachment #745414 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
https://hg.mozilla.org/mozilla-central/rev/54986162d9bd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.