1047120 - The purple buffer does not need to use Heap<T>

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Terrence Cole [:terrence]]

Attachment: no_need_for_heapt_in_purple_buffers-v0.diff

Now that I have more understanding of how the CC works, it's clear that the purple buffer can only contain gray things and therefore can never contain nursery things. Moreover, the CC is interlocked with GC, so we do not need to worry about these pointers for IGC, even if we do make Heap<T> handle this case. The upshot of this is that we can avoid the extra overhead of using Heap<T> and store the raw pointers directly.

The attached patch makes this change and adds the relevant assertions to the insertions.

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8465850 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v0.diff

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

This looks like a worthwhile improvement, but the casting to Heap<T> so we can mark is a bit unpleasant.

Would it be possible instead to add a setUnbarriered() method to Heap<T> that asserts no post barriers are required in debug mode (for both new and existing value)?  Then I think we could just use that and it wouldn't add any overhead.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8465850 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v0.diff

Andrew reviewed the patch which added mTenuredObjects.

Comment 3

[Andrew McCreight [:mccr8]]

Non-gray things are never added to the purple buffer because we filter them out, right?  A gray object in the purple buffer could later become non-gray, but for IGC we only care about objects that are non-gray at the start of the GC?  Is that right?

Also, we don't care about the post-barrier of Heap<> because we only care about the post-barrier for things that might possibly point into the nursery?

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8465850 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v0.diff

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

r=me for the nsCycleCollector changes with the comments addressed, assuming my comment I just posted makes sense. :)

::: xpcom/base/nsCycleCollector.cpp
@@ +2544,5 @@
>    NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(JSPurpleBuffer)
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(JSPurpleBuffer)
>  
>    JSPurpleBuffer*& mReferenceToThis;
> +  SegmentedArray<JS::Value> mValues;

Please add a comment here explaining why these two data structures don't need write barriers.

@@ +2660,5 @@
>  
>    virtual void Trace(JS::Heap<JSObject*>* aObject, const char* aName,
>                       void* aClosure) const
>    {
> +    JSObject *obj = *aObject;

Please factor all of this out here and in the next Trace() into a private method that takes a JSObject*.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8465850 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v0.diff

Well, I guess if you are going to rework the patch anyways I'd like to see it again.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8465850 [details] [diff] [review]
> no_need_for_heapt_in_purple_buffers-v0.diff
> 
> Well, I guess if you are going to rework the patch anyways I'd like to see
> it again.

Makes sense, those are nice improvements.

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Non-gray things are never added to the purple buffer because we filter them
> out, right?  A gray object in the purple buffer could later become non-gray,
> but for IGC we only care about objects that are non-gray at the start of the
> GC?  Is that right?

Yes, precisely. I think if we ever did want to mark the browser's JS roots incrementally, we'd probably not have the purple buffer participating anyway, for that exact reason.
 
> Also, we don't care about the post-barrier of Heap<> because we only care
> about the post-barrier for things that might possibly point into the nursery?

Exactly! IsMarkedGray always returns false for pointers in the nursery, so none of the pointers in the purple buffer can /by definition/ point into the nursery. And if that ever becomes wrong from changes to either engine, the new assertions will catch the discrepancy. I'll add a comment to that effect to the SegmentedArray decls.

Comment 7

[Terrence Cole [:terrence]]

Attachment: no_need_for_heapt_in_purple_buffers-v1.diff

The major change here is that the GC callback now does the dispatch to the trace callback method internally so that the fake Heap<T> is never exposed.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19d9616e17c2

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8467266 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v1.diff

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

r=me for the CC changes

::: xpcom/base/nsCycleCollector.cpp
@@ +2564,5 @@
>    CycleCollectionNoteChild(cb, tmp, "self");
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>  
> +#define NS_TRACE_SEGMENTED_ARRAY(_field, _type)                                \

While you are here, please fix this macro to be 2-indented instead of 4-indented.  Thanks.

@@ +2660,5 @@
>                       void* aClosure) const
>    {
>    }
>  
> +  void AppendObjectToPurpleBuffer(JSObject *obj) const

nit: Please call this AppendJSObjectToPurpleBuffer, as "object" is ambiguous in this file. ;)

@@ +3824,5 @@
>  JSPurpleBuffer*
>  nsCycleCollector::GetJSPurpleBuffer()
>  {
>    if (!mJSPurpleBuffer) {
> +    // The potential Release call confuses the GC analysis.

Well, we're definitely calling Release here.  Perhaps you mean the potential destruction of the object in Release?

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 8467266 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v1.diff

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

Great!

Comment 10

[Terrence Cole [:terrence]]

Try run looks green, modulo a relatively noisy tip. Nothing looks related to this though:
https://tbpl.mozilla.org/?tree=Try&rev=6238086413bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/05c37bb1fc03

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05c37bb1fc03