878160 - GC: post barrier weak references in the browser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Specifically, JS_IsAboutToBeFinalized should take Heap<T> type.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0

Preemptively flagging for review, feel free to ignore if the Try run is not green:

https://tbpl.mozilla.org/?tree=Try&rev=ad61b67410e0

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 758881 [details] [diff] [review]
v0

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

I need a fair amount more context than this bug contains in order to provide meaningful review.

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, you all really need to explain the reasons for your various bits of moving GC arcana in these various bugs. :)

Comment 4

[Terrence Cole [:terrence]]

Sorry, yes, I suppose I do.

The major optimization of generational GC is that it avoids tracing the full heap. It does this by inspecting all writes to a GC thing pointer and recording in a buffer (js/src/gc/StoreBuffer.h) each write to the tenured generation that that points into the nursery. When we collect the nursery, we use this buffer as a substitute for tracing the full heap.

With the magic of C++, we can capture these writes automatically by overriding |operator=|: all we need to do is change the type of all of the GC things in the system which are normally kept alive by tracing. This magic class is Heap<T> (js/public/RootingAPI.h). Heap<T> ensures that the GC thing it is pointing two gets kept alive during a minor GC, despite us not calling the browser's trace function.

The problem is finding all of these GC thing pointers so we can change their type. This is actually fairly easy: we change the input type of the tracing functions to only accept Heap<T> things and add Heap<T> until everything compiles. This particular patch changes the "weak tracing" function: JS_IsAboutToBeFinalized. Jon is currently working on the cc-participant edges in Bug 877762 using the same technique.

I hope that helps. If not, catch me on IRC and I will try to explain in more detail. Given all that, who should I ask for review here?

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 758881 [details] [diff] [review]
v0

I appreciate the explanation. I think billm should probably view the patch though. Mccr8 would work too if he understands it.

Comment 6

[Andrew McCreight [:mccr8]]

Bill knows the XPConnect GC tracing stuff better than me, so I think he's a better reviewer.  But Bill, if you are overwhelmed with post-vacation backlog, I can take a look at it.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 758881 [details] [diff] [review]
v0

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

This looks fine, but I don't think it solves the problem because we don't call the xpconnect sweeping code during minor GC. I think our options are:

1. Figure out some way to call this code, but make sure we don't do anything too expensive.
2. Make sure none of this stuff ends up in the nursery in the first place.

My preference would be for #2 if it's possible. I'm pretty certain that it would be easy to allocate the mPrototypeNoHelper tenured. The mWaiverWrapperMap stuff I'm a little less certain about.

::: js/xpconnect/src/XPCMaps.h
@@ +602,5 @@
>  /***************************************************************************/
>  
>  class JSObject2JSObjectMap
>  {
> +    typedef js::HashMap<JS::Heap<JSObject *>, JS::Heap<JSObject *>, js::PointerHasher<JSObject *, 3>,

I don't think it really helps us to make the key be a JS::Heap. Hashtable keys always need special barriers that remove the entry from the table and re-add them. So I would just keep the key as a JSObject*.

@@ +645,5 @@
> +            /*
> +             * It is safe to store a Heap pointer on the stack during sweeping
> +             * because the store buffer is disabled during GC.
> +             */
> +            JS::Heap<JSObject *> updated(e.front().key);

...and in that case, you won't need this infelicity.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1259,5 @@
>          XPCWrappedNativeTearOff* to = chunk->mTearOffs;
>          for (int i = XPC_WRAPPED_NATIVE_TEAROFFS_PER_CHUNK-1; i >= 0; i--, to++) {
>              JSObject* jso = to->GetJSObjectPreserveColor();
>              if (jso) {
> +                NS_ASSERTION(JS_IsAboutToBeFinalizedUnbarriered(&jso), "bad!");

Can you make this a MOZ_ASSERT while you're here?

Comment 8

[Bobby Holley (:bholley)]

I'm pretty sure we don't need mPrototypeNoHelper anymore. Patches coming up in bug 881517.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: v1

I removed barriering of the XPConnect map keys and added assertions that these are never in the nursery, and this passes a try build (exact rooting enabled):

https://tbpl.mozilla.org/?tree=Try&rev=57ef86746d73

I don't claim to know why this is the case however, or if it is guaranteed.

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #9)

So the reason it passes is because these asserts do nothing unless generational GC is enabled.

XPCWrappedNativeScope::mWaiverWrapperMap has XPCWrappedNative keys, and as these have finalizers they will not be allocated in the nursery (however we could make this more explicit).

XPCJSRuntime::mWrappedJSMap however can contain any JSObject as key as far as I can see, so that is more of a problem.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: 1 - JS engine changes

This adds the following:
 - JS::AssertObjectIsTenured()
 - HashMap::rekey() method for updating a single entry
 - JS_EmitPostBarrierCallback for custom post barrier implmentation outside the engine
 - Heap<T> == and != operators for comparison with other Heap<T> instances.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: 2 - Post barrier weak references in the browser

Convert JS_IsAboutToBeFinalized to take Heap<JSObject*> and update callers.

Terrence, I'll ask bholley for review too if you think this is ok.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 768989 [details] [diff] [review]
1 - JS engine changes

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

This is very nice, just a few small nits.

r=me

::: js/public/HashTable.h
@@ +256,5 @@
>      }
>  
> +    // Infallibly rekey one entry, if present.
> +    void rekey(const Lookup &old_key, const Key &new_key) {
> +        if (Ptr p = lookup(old_key))

If you add |old_key != new_key &&| to the front of this if(), I think we should be able to remove the same checks around a bunch of users and save some cycles on users that are currently missing it.

@@ +1463,5 @@
> +        JS_ASSERT(p.found());
> +        typename HashTableEntry<T>::NonConstT t(Move(*p));
> +        HashPolicy::setKey(t, const_cast<Key &>(k));
> +        remove(*p.entry_);
> +        putNewInfallible(l, Move(t));

Very nice!

::: js/src/gc/StoreBuffer.h
@@ +212,5 @@
>          void put(const T &t) {
>              JS_ASSERT(!owner->inParallelSection());
>  
> +            /* Ensure T is derived from BufferableRef. */
> +            (void)static_cast<const BufferableRef*>(&t);

Oh, wow, I am so stealing that trick!

@@ +350,5 @@
>  
>          void mark(JSTracer *trc);
>      };
>  
> +    struct CallbackRef : public BufferableRef

Unless I'm missing a user, I think you should be able to make this a class and hide the data definitions, like HashKeyRef.

::: js/src/jsfriendapi.h
@@ +1763,5 @@
>  js_ReportIsNotFunction(JSContext *cx, const JS::Value& v);
>  
> +#ifdef JSGC_GENERATIONAL
> +extern JS_FRIEND_API(void)
> +JS_EmitPostBarrierCallback(JSContext* cx, void (*callback)(JSTracer *trc, void *key), void *key);

I think this should have an #else with an inline, no-body definition so that callers don't need macro check around every use.

::: js/src/jsgc.cpp
@@ +5059,5 @@
> +
> +JS_FRIEND_API(void)
> +JS::AssertObjectIsTenured(JSObject *obj)
> +{
> +    JS_ASSERT(obj->isTenured());

I think, if we make the assertion a bit softer, we can get the spirit of the assertion even in non-GGC builds. How about:

  (!IsNurseryAllocable(obj->tenuredAllocKind()) || obj->getClass()->finalize) && obj->isTenured()

That would change what the assertion is doing so we should change the name to match. How about AssertObjectMustBeTenured.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 768991 [details] [diff] [review]
2 - Post barrier weak references in the browser

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

Looks good to me.

r=me

::: js/xpconnect/src/XPCMaps.h
@@ +84,5 @@
>  
> +#ifdef JSGC_GENERATIONAL
> +    /*
> +     * This function is called during minor GC when one of our hash keys is in
> +     * the storebuffer.

The store buffer reference is a bit of an implementation detail. How about: This function is called during minor GCs for each key in the HashMap that has been moved.

@@ +93,5 @@
> +    static void KeyMarkCallback(JSTracer *trc, void *k) {
> +        JSObject *key = static_cast<JSObject*>(k);
> +        JSObject *prior = key;
> +        JS_CallObjectTracer(trc, &key, "XPCJSRuntime::mWrappedJSMap key");
> +        if (key != prior) {

Depending on how expensive GetRuntimeInstance and GetWrappedJSMap are, it may be worth letting the check in rekey do this comparison.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 768991 [details] [diff] [review]
2 - Post barrier weak references in the browser

I think Andrew should probably review this.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 768991 [details] [diff] [review]
2 - Post barrier weak references in the browser

Not to kick the can down the road again, but I think it makes sense for bill to look at this. In particular, it isn't obvious to me that his concerns from his prior review are addressed.

Comment 17

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #13)

Thanks for the comments.

With the stricter assertion, it turned out that those keys were not always tenured, which meant I had to use the callback postbarrier for both types of map in XPConnect.  In turn, this meant I didn't need the assertion so I removed it again.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: 1 - JS engine changes

Comment 19

[Jon Coppeard (:jonco)]

Attachment: 2 - Post barrier weak references in the browser

Comment 20

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

Comment on attachment 769713 [details] [diff] [review]
1 - JS engine changes

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

::: js/src/jsfriendapi.h
@@ +1763,5 @@
>  js_ReportIsNotFunction(JSContext *cx, const JS::Value& v);
>  
> +#ifdef JSGC_GENERATIONAL
> +extern JS_FRIEND_API(void)
> +JS_EmitPostBarrierCallback(JSContext* cx, void (*callback)(JSTracer *trc, void *key), void *key);

The "emit" name was a little confusing to me until I thought it through. What about "store"?

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 769714 [details] [diff] [review]
2 - Post barrier weak references in the browser

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

Looks nice!

::: js/xpconnect/src/XPCMaps.h
@@ +93,5 @@
> +        JS_CallObjectTracer(trc, &key, "XPCJSRuntime::mWrappedJSMap key");
> +        XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
> +        JSObject2WrappedJSMap* self = rt->GetWrappedJSMap();
> +        self->mTable.rekey(prior, key);
> +        }

This is indented too far.

@@ +702,5 @@
> +        JSObject *prior = key;
> +        JS_CallObjectTracer(trc, &key, "XPCWrappedNativeScope::mWaiverWrapperMap key");
> +        JSObject2JSObjectMap *self = xpc::GetObjectScope(key)->mWaiverWrapperMap;
> +        self->mTable.rekey(prior, key);
> +        }

Same here.

Comment 22

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef38d43fd49

Comment 23

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f08b7f90b7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ef38d43fd49
https://hg.mozilla.org/mozilla-central/rev/d1f08b7f90b7

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 769714 [details] [diff] [review]
2 - Post barrier weak references in the browser

Meant to r+ this.