Closed Bug 1363963 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPConnect, enhancement, P2)

Product:
Core
Component:
XPConnect
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ting, Assigned: ting)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1363963 - Check whether there are expando object attached before trying to retrieve it.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
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...
Priority: -- → P2
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Attached patch poc (obsolete) — — Splinter Review 
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.
Flags: needinfo?(bzbarsky)
With the poc, the benchmark (bug 1348095 comment 3) goes down from 43xx to 39xx.
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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8871623 - Flags: review?(bobbyholley) → review-
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().
> a hashmap lookup should be relatively cheaper

I would not be surprised if JSAutoCompartment is cheaper than a hashtable lookup.  Worth measuring.
(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 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.
Attachment #8871623 - Flags: review?(bobbyholley) → review-
(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.
Flags: needinfo?(bobbyholley)
(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.
(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.
Flags: needinfo?(bobbyholley)
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
Attachment #8871623 - Flags: review?(bobbyholley) → review+
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00f292748b3e
Check whether there are expando object attached before trying to retrieve it. r=bholley
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
https://hg.mozilla.org/mozilla-central/rev/00f292748b3e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → janus926
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: