Closed Bug 1297558 Opened 3 years ago Closed 3 years ago

Automatically ExposeToActiveJS when reading out of a Heap<T>

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- affected
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: terrence, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Barrier_ExposeToJS-v0.diff (obsolete) — Splinter Review
I'm pretty sure I already have a bug open from when I tried this before, but I can't find it right now. This new variant is surprisingly svelte: 15 files changed, 142 insertions(+), 41 deletions(-). The combination of |explicit operator bool|, better knowledge of how the CC works, and past experience doing this new effort go much quicker. So far I've booted up a browser and messed around in twitter and some news sites for a few minutes. Hopefully, we'll see how good the coverage was with the following try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f2bcdffddf0

Most of the changes should be fairly obvious, but I'll annotate with my notes inline in the patch so that we're all on the same page (and try is actually green) before asking for reviews.
Blocks: 1296848
See Also: → 1004276
See Also: → 1160275
Attached patch bug1297558-auto-expose v2 (obsolete) — Splinter Review
Work in progress patch.
Assignee: terrence.d.cole → jcoppeard
Attachment #8784195 - Attachment is obsolete: true
Patch to add a read barrier to Heap<T> to perform ExposeToActiveJS automatically.  Most of the patch making sure we don't fire the read barrier where we don't want to do this, e.g. during GC and CC.

The AutoEnterCycleCollection guard object asserts that we don't do this during CC.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d540e41f7f
Attachment #8795766 - Attachment is obsolete: true
Attachment #8796225 - Flags: review?(sphink)
Comment on attachment 8796225 [details] [diff] [review]
bug1297558-auto-expose v3

Requesting review for non SM parts.
Attachment #8796225 - Flags: review?(continuation)
Comment on attachment 8796225 [details] [diff] [review]
bug1297558-auto-expose v3

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

Thanks for fixing this! This is very nice. We could potentially rip out a lot of the manual Expose calls now.

r=me for the XPConnect and non-js/ changes.

::: js/xpconnect/src/xpcprivate.h
@@ +890,5 @@
>          return mGlobalJSObject;
>      }
>  
>      JSObject*
> +    GetGlobalJSObjectPreserveColor() const {return mGlobalJSObject.unbarrieredGet();}

While you are touching these lines, please add a space before the return and after the ;, here and in other places.

::: xpcom/base/nsCycleCollector.cpp
@@ +2659,5 @@
>          mCollector->RemoveObjectFromGraph(o.mPointer);
>          o.mRefCnt->stabilizeForDeletion();
> +        {
> +          JS::AutoEnterCycleCollection autocc(mCollector->Context()->Context());
> +          o.mParticipant->Trace(o.mPointer, *this, nullptr);

I guess this has to be tightly scoped because DeleteCycleCollectable could do all sorts of random stuff.

@@ +2689,5 @@
>    virtual void Trace(JS::Heap<JS::Value>* aValue, const char* aName,
>                       void* aClosure) const override
>    {
> +    const JS::Value& val = aValue->unbarrieredGet();
> +    if (val.isMarkable() && ValueIsGrayCCThing(val)) {

Could you instead change this to take a JS::Heap<JS::Value>* or & argument and do the unbarriedGet inside there? This is the only caller and I think it might be a little clearer.

@@ +3770,5 @@
>      mMergedInARow = 0;
>      return false;
>    }
>  
> +  JS::AutoEnterCycleCollection autocc(Context()->Context());

Hmm, this is peculiar. What is this actually guarding? I would guess UsefulToMergeZones() but both of those calls are empty now. It would be a little nicer if you just moved that into XPCJSContext::UsefulToMergeZones() (or really, in a patch that makes them do something).

@@ +3843,5 @@
>    bool mergeZones = ShouldMergeZones(aCCType);
>    mResults.mMergedZones = mergeZones;
>  
>    JS::AutoAssertOnGC nogc;
> +  JS::AutoEnterCycleCollection autocc(mJSContext->Context());

...or alternatively, you could hoist this autocc above the ShouldMergeZones (maybe put it after the "Set up the data structures for building the graph" comment), and then you should be able to drop it from ShouldMergeZones.
Attachment #8796225 - Flags: review?(continuation) → review+
Comment on attachment 8796225 [details] [diff] [review]
bug1297558-auto-expose v3

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

This looks great. I really hope it sticks. What would be the symptom of excess unmarking? (Other than assertion failures.) More memory usage?

::: js/xpconnect/src/xpcprivate.h
@@ +2225,5 @@
> +    JSCompartment* Compartment() const {
> +        return js::GetObjectCompartment(mJSObj.unbarrieredGet());
> +    }
> +
> +private:

Is the redundant private: intentional? (I'm not sure if you wanted a separate one for the data members or not.)
Attachment #8796225 - Flags: review?(sphink) → review+
(In reply to Andrew McCreight [:mccr8] from comment #7)

Thanks for the speedy review!

> I guess this has to be tightly scoped because DeleteCycleCollectable could
> do all sorts of random stuff.

Yes.

> @@ +2689,5 @@
> >    virtual void Trace(JS::Heap<JS::Value>* aValue, const char* aName,
> >                       void* aClosure) const override
> >    {
> > +    const JS::Value& val = aValue->unbarrieredGet();
> > +    if (val.isMarkable() && ValueIsGrayCCThing(val)) {
> 
> Could you instead change this to take a JS::Heap<JS::Value>* or & argument
> and do the unbarriedGet inside there? This is the only caller and I think it
> might be a little clearer.

I tried this but still you need the unbarrieredGet() in Trace for the following two lines.  This seemed more complicated so I left it as was.

> @@ +3843,5 @@
> >    bool mergeZones = ShouldMergeZones(aCCType);
> >    mResults.mMergedZones = mergeZones;
> >  
> >    JS::AutoAssertOnGC nogc;
> > +  JS::AutoEnterCycleCollection autocc(mJSContext->Context());
> 
> ...or alternatively, you could hoist this autocc above the ShouldMergeZones
> (maybe put it after the "Set up the data structures for building the graph"
> comment), and then you should be able to drop it from ShouldMergeZones.

Yes, that looks like the best thing to do.
(In reply to Steve Fink [:sfink] [:s:] from comment #8)
> What would be the symptom of
> excess unmarking? (Other than assertion failures.) More memory usage?

Leaks at shutdown is the usual symptom.

> Is the redundant private: intentional? 

Nope, fixed.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/138dd1cfe696
Use a read barrier on Heap to ExposeToActiveJS r=sfink r=mccr8
Blocks: 1306579
https://hg.mozilla.org/mozilla-central/rev/138dd1cfe696
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
So I'm a little confused.  Doesn't this fundamentally break things like CallbackObject::CallbackPreserveColor()?  I was expecting something that preserved those semantics via an unsafeGet() or whatever, but I'm not seeing anything like that in the patch...
Flags: needinfo?(jcoppeard)
Ah, we _do_ have unbarrieredGet() in the patch, used for some *PreserveColor functions.  Just not consistently?
Also afaict the assert in CallbackObject::CallbackKnownNotGray() is now busted, because of the .get() call it had in there (which shouldn't have been there; my fault).
Sorry, I guess CallbackObject::CallbackPreserveColor is ok because it uses .address(), which is not barriered.  Sorry for the false alarm.

I still think the assert in CallbackKnownNotGray is busted, though.
Depends on: 1307372
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)
You're right, CallbackKnownNotGray is broken.  I've filed bug 1307372.
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1538850bba0f
Remove explicit calls to Expose*ToActiveJS r=mccr8
Do we not need to do something similar for TenuredHeap?
Flags: needinfo?(jcoppeard)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #20)
> Do we not need to do something similar for TenuredHeap?

Bug 1306382
Flags: needinfo?(jcoppeard)
See Also: → 1017650
You need to log in before you can comment on or make changes to this bug.