1297558 - Automatically ExposeToActiveJS when reading out of a Heap<T>

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: Barrier_ExposeToJS-v0.diff

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.

Comment 1

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b2b6be86211

TH and/or TC threw a tantrum last night.

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d726b615b41f

Comment 3

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d24b7851b3f5

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1297558-auto-expose v2

Work in progress patch.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1297558-auto-expose v3

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

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8796225 [details] [diff] [review]
bug1297558-auto-expose v3

Requesting review for non SM parts.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Steve Fink [:sfink] [:s:]]

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.)

Comment 9

[Jon Coppeard (:jonco)]

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

Comment 10

[Jon Coppeard (:jonco)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/138dd1cfe696
Use a read barrier on Heap to ExposeToActiveJS r=sfink r=mccr8

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/138dd1cfe696

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, we _do_ have unbarrieredGet() in the patch, used for some *PreserveColor functions.  Just not consistently?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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).

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)
You're right, CallbackKnownNotGray is broken.  I've filed bug 1307372.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1538850bba0f
Remove explicit calls to Expose*ToActiveJS r=mccr8

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1538850bba0f

Comment 20

[Boris Zbarsky [:bzbarsky]]

Do we not need to do something similar for TenuredHeap?

Comment 21

[Steve Fink [:sfink] [:s:]]

(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