bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in mozilla34

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8465850 [details] [diff] [review]
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.
Attachment #8465850 - Flags: review?(jcoppeard)
Attachment #8465850 - Flags: review?(bugs)
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.
Attachment #8465850 - Flags: review?(jcoppeard)
Comment on attachment 8465850 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v0.diff

Andrew reviewed the patch which added mTenuredObjects.
Attachment #8465850 - Flags: review?(bugs) → review?(continuation)
Target Milestone: flash10 → ---
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 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*.
Attachment #8465850 - Flags: review?(continuation) → review+
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.
Attachment #8465850 - Flags: review+
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 8467266 [details] [diff] [review]
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
Attachment #8465850 - Attachment is obsolete: true
Attachment #8467266 - Flags: review?(jcoppeard)
Attachment #8467266 - Flags: review?(continuation)
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?
Attachment #8467266 - Flags: review?(continuation) → review+
Comment on attachment 8467266 [details] [diff] [review]
no_need_for_heapt_in_purple_buffers-v1.diff

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

Great!
Attachment #8467266 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 10

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/05c37bb1fc03
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.