1363963 - XrayTraits::getExpandoObject() could be done twice in a single js::Proxy::get() call

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Ting-Yu Chou [:ting] (away)]

From the profile of attachment 8848224 [details], I see XrayTraits::getExpandObject() is called twice from a single js::Proxy::get() call when the property is not existed:

a. js::Proxy::get
     js::BaseProxyHandler::hasOwn
       xpc::XrayWrapper::getOwnPropertyDescriptor
         xpc::DOMXrayTraits::resolveOwnProperty
           xpc::XrayTraits::resolveOwnProperty
             xpc::XrayTraits::getExpandoObject

b. js::Proxy::get
     xpc::XrayWrapper::getPrototype
       xpc::XrayTraits::getExpandoObject

getExpandoObject() is quite expensive, if we can eliminate the redundant call...

Comment 1

[Ting-Yu Chou [:ting] (away)]

Attachment: poc

Simply add a cache in getExpandoObjectInternal() for matched target/origin/exclusiveGlobalArg from the last call, the hit rate is >40% from experiments. But I don't know if I am doing this correctly, I checked and it seems that those pointers won't be affected by GC.

I am looking for a feedback about is this something we can take? To pass retrieved expando object around to avoid redundant call is too much to me, I feel better with a cache inside the getter.

Comment 2

[Ting-Yu Chou [:ting] (away)]

With the poc, the benchmark (bug 1348095 comment 3) goes down from 43xx to 39xx.

Comment 3

[Ting-Yu Chou [:ting] (away)]

There're cases need to be considered for invalidating the cache, so probably not a good idea:

- if there's a sequence like this for the same target/consumer/exclusiveGlobal
  ensureExpandoObject
    getExpandoObject
    attachExpandoObject
  getExpandoObject

- the expando chain is in a weakmap, which the cached expando object may have been collected already

But I found getExpandoObject() in most cases just return nullptr, the cache can be a boolean state for whether an expando object has ever been attached, if no just return nullptr.

Comment 4

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

In most cases no expando object has ever been attached, we don't need to do a
lot of things and realize it in the last minute.

Review commit: https://reviewboard.mozilla.org/r/143112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143112/

Comment 5

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8871556 [details] [diff] [review]
poc

># HG changeset patch
># User Ting-Yu Chou <[e-mail]>
># Date 1495764280 -28800
>#      Fri May 26 10:04:40 2017 +0800
># Node ID de85edd58f2ee87ab90fff762fb3899e931e862c
># Parent  55e5723b1e62190a38c00927eda796e2ad14778f
>Bug 1363963 - poc
>
>diff --git a/js/xpconnect/wrappers/XrayWrapper.cpp b/js/xpconnect/wrappers/XrayWrapper.cpp
>--- a/js/xpconnect/wrappers/XrayWrapper.cpp
>+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
>@@ -1120,9 +1120,24 @@ XrayTraits::getExpandoObjectInternal(JSC
>                                      JSObject* exclusiveGlobalArg,
>                                      MutableHandleObject expandoObject)
> {
>+    static struct {
>+        JSObject* target;
>+        nsIPrincipal* origin;
>+        JSObject* exclusiveGlobalArg;
>+        JSObject* expandoObject;
>+    } cache;
>+
>     MOZ_ASSERT(!JS_IsExceptionPending(cx));
>     expandoObject.set(nullptr);
> 
>+    if (target == cache.target &&
>+        origin == cache.origin &&
>+        exclusiveGlobalArg == cache.exclusiveGlobalArg)
>+    {
>+        expandoObject.set(cache.expandoObject);
>+        return true;
>+    }
>+
>     // The expando object lives in the compartment of the target, so all our
>     // work needs to happen there.
>     RootedObject exclusiveGlobal(cx, exclusiveGlobalArg);
>@@ -1130,17 +1145,23 @@ XrayTraits::getExpandoObjectInternal(JSC
>     if (!JS_WrapObject(cx, &exclusiveGlobal))
>         return false;
> 
>+    cache.target = target;
>+    cache.origin = origin;
>+    cache.exclusiveGlobalArg = exclusiveGlobalArg;
>+
>     // Iterate through the chain, looking for a same-origin object.
>     RootedObject head(cx, getExpandoChain(target));
>     while (head) {
>         if (expandoObjectMatchesConsumer(cx, head, origin, exclusiveGlobal)) {
>             expandoObject.set(head);
>+            cache.expandoObject = head;
>             return true;
>         }
>         head = JS_GetReservedSlot(head, JSSLOT_EXPANDO_NEXT).toObjectOrNull();
>     }
> 
>     // Not found.
>+    cache.expandoObject = nullptr;
>     return true;
> }
>

Comment 6

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143112/diff/1-2/

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

https://reviewboard.mozilla.org/r/143112/#review146916

This causes us to do an extra hashmap lookup in the case where there is an expando object, right? If we do this, we should only get it once, and pass the chain to getExpandoObjectInternal as an arg.

Comment 8

[Ting-Yu Chou [:ting] (away)]

I thought about that, but because all the other places access the expando chain use JSAutoCompartment to switch to the target compartment before rooting it, so I guess JSAutoCompartment is necessary but I want to avoid its overhead, a hashmap lookup should be relatively cheaper.

But you're right, what I really need is mXrayExpandos.initialized().

Comment 9

[Boris Zbarsky [:bzbarsky]]

> a hashmap lookup should be relatively cheaper

I would not be surprised if JSAutoCompartment is cheaper than a hashtable lookup.  Worth measuring.

Comment 10

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143112/diff/2-3/

Comment 11

[Ting-Yu Chou [:ting] (away)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #9)
> > a hashmap lookup should be relatively cheaper
> 
> I would not be surprised if JSAutoCompartment is cheaper than a hashtable
> lookup.  Worth measuring.

Sorry I didn't make my self clear, what I meant was:

  XrayTraits::getExpandoObject(...) {
    if (!getExpandoChain(target)
        return true;
    ...
  }

should be relatively cheaper than:

  XrayTraits::getExpandoObject(...) {
    JSAutoCompartment ac(cx, target);
    RootedObject chain(cx, getExpandoChain(target));
    if (!chain)
        return true;
    ...
    return getExpandoObjectInternal(..., chain);
  }

because it's rare that getExpandoChain() does a hashmap lookup.

Comment 12

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143112/diff/3-4/

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

https://reviewboard.mozilla.org/r/143112/#review148340

There's no reason why getExpandoChain needs the caller to have entered the compartment. You can enter the compartment afterwards, assuming it's non-null.

This patch seems strictly worse than my proposal in comment 7, since it presents a performance cliff on the entire scope as soon as there's a single expando on any object.

Comment 14

[Ting-Yu Chou [:ting] (away)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> There's no reason why getExpandoChain needs the caller to have entered the
> compartment. You can enter the compartment afterwards, assuming it's
> non-null.

I see.

Sorry for keep bothering, but then do I need to root:

  RootedObject chain(cx, getExpandoChain(target));

to pass |chain| as a HandleObject to getExpandoObjectInternal()? It seems I should because getExpandoChain() could return a pointer from a WeakMapPtr, but getExpandoChain() passes |exclusiveGlobalArg| a raw JSObject pointer to getExpandoObjectInternal() and root it there.

Comment 15

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #14)
> but getExpandoChain() passes |exclusiveGlobalArg| a raw JSObject pointer to
> getExpandoObjectInternal() and root it there.

Correction, it's getExpandoObject() passes |exlusiveGlobalArg| a raw JSObject pointer.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Ting-Yu Chou [:ting] from comment #14)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> > There's no reason why getExpandoChain needs the caller to have entered the
> > compartment. You can enter the compartment afterwards, assuming it's
> > non-null.
> 
> I see.
> 
> Sorry for keep bothering, but then do I need to root:
> 
>   RootedObject chain(cx, getExpandoChain(target));
> 
> to pass |chain| as a HandleObject to getExpandoObjectInternal()? It seems I
> should because getExpandoChain() could return a pointer from a WeakMapPtr,
> but getExpandoChain() passes |exclusiveGlobalArg| a raw JSObject pointer to
> getExpandoObjectInternal() and root it there.

Rooting it and then passing it is the right thing to do. Whether you can pass it as a raw pointer or need to pass as a HandleObject depends on whether there's anything in between that can GC. The hazard analysis will tell you, but passing a HandleObject is always safer.

Comment 17

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143112/diff/4-5/

Comment 18

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143112/diff/5-6/

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8871623 [details]
Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.

https://reviewboard.mozilla.org/r/143112/#review148874

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00f292748b3e
Check whether there are expando object attached before trying to retrieve it. r=bholley

Comment 21

[Ting-Yu Chou [:ting] (away)]

The numbers from the micro benchmark (20 runs) on my desktop:

Without the patch: AVG=4373, STD=198.34
With the patch: AVG=3644, STD=70.69

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00f292748b3e