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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ting, Assigned: ting)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

2 years ago
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]
Assignee

Comment 1

2 years ago
Posted 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)
Assignee

Comment 2

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

Comment 3

2 years ago
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 hidden (mozreview-request)
Comment hidden (obsolete)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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-
Assignee

Comment 8

2 years ago
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.
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
(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 hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
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-
Assignee

Comment 14

2 years ago
(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)
Assignee

Comment 15

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
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+

Comment 20

2 years ago
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
Assignee

Comment 21

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00f292748b3e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Assignee: nobody → janus926
You need to log in before you can comment on or make changes to this bug.