1334263 - Expandos are really slow to access

Status: RESOLVED DUPLICATE of bug 613498

(Core :: XPConnect, defect, P1)

Description

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

I'm doing some profiling of LastPass. It does a lot of iteration over <input> elements. One thing I noticed is that our expando code does a lot of work just to access a property. You can see it in this profile:

https://clptr.io/2kyucou

A lot of the stacks in that region contain getExpandoObjectInternal. It would be nice if we could do some caching here.

Comment 1

[Bobby Holley (:bholley)]

Yeah, none of this stuff was ever profiled, so there may be low-hanging fruit.

It would be worth logging how long the expando chain is. If it's more than a couple of entries then something is probably wrong.

It looks like XrayWrappers only use one of the two proxy extra slots. We could potentially use the second one to cache the expando object.

Comment 3

[(no longer active)]

This keeps coming up in my profiles, we should probably fix it for QF.

Comment 4

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

Just to write up my discussion about this with Bobby:

To speed up the expando case, we should add a slot to XrayWrapper that points to the expando object for our own compartment. (Well, I guess it would point to a CCW for it.) We can only do this for sandboxes, since in other cases we might have to share expandos across compartments.

However, that only makes expando access faster. Xrays in general are really slow. The main problem is that we walk the proto chain of the Xray to do property lookup. So each property access has to go through five or so Xrays, looking for the property. And each one of those runs a lot of C++ code. Fixing this will be a lot more difficult.

Comment 5

[Bobby Holley (:bholley)]

Note that speeding up expandos will, in general, speed up all five of those lookups.

If this is still a bottleneck once that's fixed, we should probably schedule a call between me, bill, and some jit-knowledgable people to discuss options.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #4)
> Just to write up my discussion about this with Bobby:
> 
> To speed up the expando case, we should add a slot to XrayWrapper that
> points to the expando object for our own compartment. (Well, I guess it
> would point to a CCW for it.) We can only do this for sandboxes, since in
> other cases we might have to share expandos across compartments.

Hm. I think at some point we may wind up having to share expandos across compartments for extension sandboxes, too. People have already filed bugs about this for content scripts, since that's apparently the way things work in Chrome's separate worlds model.

What are the other circumstances where we'd share expandos that don't apply to sandboxes? I thought system principal sandboxes got the same expandos as other system principal compartments. Although, looking at the code, it looks like I'm wrong, and we treat sandboxes specially, only sharing expandos with the exact same global.

Also, what prevents us from doing this when we have to share expandos across compartments? They should never change after the first time we look them up.

Comment 7

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

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Bill McCloskey (:billm) from comment #4)
> > Just to write up my discussion about this with Bobby:
> > 
> > To speed up the expando case, we should add a slot to XrayWrapper that
> > points to the expando object for our own compartment. (Well, I guess it
> > would point to a CCW for it.) We can only do this for sandboxes, since in
> > other cases we might have to share expandos across compartments.
> 
> Hm. I think at some point we may wind up having to share expandos across
> compartments for extension sandboxes, too. People have already filed bugs
> about this for content scripts, since that's apparently the way things work
> in Chrome's separate worlds model.

Don't we run all content scripts for a given (extension, page) combination in the same sandbox? What more sharing would they need?

> What are the other circumstances where we'd share expandos that don't apply
> to sandboxes? I thought system principal sandboxes got the same expandos as
> other system principal compartments. Although, looking at the code, it looks
> like I'm wrong, and we treat sandboxes specially, only sharing expandos with
> the exact same global.

Expandos are shared among globals with equivalent principals, except for sandboxes where there's no sharing. So two system principal compartments share expandos.

> Also, what prevents us from doing this when we have to share expandos across
> compartments? They should never change after the first time we look them up.

I think we could do it, but we would have to look up every time if there's no expando object yet. That's because some other compartment that you share expandos with could create an expando object.

In the sandbox case, if the expando slot is undefined, then we can assume that there's no expando object.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #7)
> > Hm. I think at some point we may wind up having to share expandos across
> > compartments for extension sandboxes, too. People have already filed bugs
> > about this for content scripts, since that's apparently the way things work
> > in Chrome's separate worlds model.
>
> Don't we run all content scripts for a given (extension, page) combination
> in the same sandbox? What more sharing would they need?

They expect to be able to access expandos from content scripts running in
parent and child windows. Bug 1208775 is still a problem for that, but even
when they define expandos on the window wrapper (or DOM nodes) directly,
they have no way to access them.

> > Also, what prevents us from doing this when we have to share expandos across
> > compartments? They should never change after the first time we look them up.
>
> I think we could do it, but we would have to look up every time if there's
> no expando object yet. That's because some other compartment that you share
> expandos with could create an expando object.

Hm. Right. And I guess we wouldn't want to create an expando object until
someone defines a property on it, so it wouldn't buy us much. Maybe we could
set a flag on the wrapped object when any Xray adds an expando to it?

> In the sandbox case, if the expando slot is undefined, then we can assume
> that there's no expando object.

Assuming we want to give up on the possibility of sharing expandos between a
group of sandboxes, yeah.

Comment 9

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

(In reply to Kris Maglione [:kmag] from comment #8)
> They expect to be able to access expandos from content scripts running in
> parent and child windows. Bug 1208775 is still a problem for that, but even
> when they define expandos on the window wrapper (or DOM nodes) directly,
> they have no way to access them.

But that would only work if the parent and child windows are same-origin, right? Does the add-on somehow know that they are? Seems weird, but okay. I guess the principal equality check we have there will do this right thing. We would just need to remove the exclusive global business.

> Hm. Right. And I guess we wouldn't want to create an expando object until
> someone defines a property on it, so it wouldn't buy us much. Maybe we could
> set a flag on the wrapped object when any Xray adds an expando to it?

That's kinda what happens. We look up the expando chain in a hashtable and see that it's null. We can't store anything on the object itself since we don't have any extra space for that AFAIK.

We could speed this up a little by removing the exclusive global wrapping that happens here:
http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/js/xpconnect/wrappers/XrayWrapper.cpp#1121-1126

The real problem is that a lot of add-ons like to put expandos on common objects like the document, so there will always be some expando. And comparing principals using Equals seems to be really slow. If we can't get rid of that Equals check, we're probably not going to make much progress here.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #9)
> But that would only work if the parent and child windows are same-origin,
> right? Does the add-on somehow know that they are? Seems weird, but okay.

Yes, it only works for same origin windows. I suspect the use cases are mostly
for specific sites that use same-origin subframes in specific ways. But I'm
not sure about the exact use cases, just that people are asking for it.

> > Hm. Right. And I guess we wouldn't want to create an expando object until
> > someone defines a property on it, so it wouldn't buy us much. Maybe we could
> > set a flag on the wrapped object when any Xray adds an expando to it?
>
> That's kinda what happens. We look up the expando chain in a hashtable and
> see that it's null. We can't store anything on the object itself since we
> don't have any extra space for that AFAIK.

Hm. Good point. I guess the only advantage of a flag is that we'd only need a
single bit rather than a whole slot. But if we can't afford a bit either, than
I guess it doesn't help.

> The real problem is that a lot of add-ons like to put expandos on common
> objects like the document, so there will always be some expando. And
> comparing principals using Equals seems to be really slow. If we can't get
> rid of that Equals check, we're probably not going to make much progress
> here.

I suppose we could get around that by creating an expando object for any
compartment that does a lookup once one exists for any other compartment. Not
sure how much overhead that would wind up being, though.

I'd kind of expect the bigger problem to be things like tree walkers, though,
that walk over hundreds, or thousands, of nodes that don't have any expandos.
You get that with things like the jQuery match engine even in the simplest use
cases.

Comment 11

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #9)
> The real problem is that a lot of add-ons like to put expandos on common
> objects like the document, so there will always be some expando. And
> comparing principals using Equals seems to be really slow. If we can't get
> rid of that Equals check, we're probably not going to make much progress
> here.

I have some ideas for making nsIPrincipal::Equals() really fast, which should help here.  I'll file a blocking bug.

Comment 12

[(no longer active)]

After bug 1340710, nothing from nsIPrincipal::Equals shows up in the profile any more.  Here is a profile in case you guys want to look at it before that bug lands.  This is from Gmail's login page with the latest version of LastPass.

Comment 13

[(no longer active)]

Oops: https://perfht.ml/2lDM3Ky

Comment 14

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #12)
> After bug 1340710, nothing from nsIPrincipal::Equals shows up in the profile
> any more.  Here is a profile in case you guys want to look at it before that
> bug lands.  This is from Gmail's login page with the latest version of
> LastPass.

This looks promising. Would be interested in seeing a profile for the WebExtension version, too, though. The SDK doesn't add any internal URLs to the expanded principals for its content scripts, so your fast path should apply there. But WebExtension content scripts do, so I wouldn't expect any speed-up there.

Comment 15

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #14)
> (In reply to :Ehsan Akhgari from comment #12)
> > After bug 1340710, nothing from nsIPrincipal::Equals shows up in the profile
> > any more.  Here is a profile in case you guys want to look at it before that
> > bug lands.  This is from Gmail's login page with the latest version of
> > LastPass.
> 
> This looks promising. Would be interested in seeing a profile for the
> WebExtension version, too, though. The SDK doesn't add any internal URLs to
> the expanded principals for its content scripts, so your fast path should
> apply there. But WebExtension content scripts do, so I wouldn't expect any
> speed-up there.

Did you want me to profile something specific?

Comment 16

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #15)
> Did you want me to profile something specific?

I'm just interested in the difference between the SDK version of LastPass that you profiled and the WebExtension version. So, the same test, but with a different version of the add-on.

Unfortunately, there are about 30 versions of this on AMO, and I have no idea which one should be tested:

https://dxr.mozilla.org/addons/search?q=LastPass+path%3Amanifest.json&redirect=false

Comment 17

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #16)
> (In reply to :Ehsan Akhgari from comment #15)
> > Did you want me to profile something specific?
> 
> I'm just interested in the difference between the SDK version of LastPass
> that you profiled and the WebExtension version. So, the same test, but with
> a different version of the add-on.
> 
> Unfortunately, there are about 30 versions of this on AMO, and I have no
> idea which one should be tested:
> 
> https://dxr.mozilla.org/addons/search?q=LastPass+path%3Amanifest.
> json&redirect=false

Me neither.  :/

Comment 18

[Kris Maglione [:kmag]]

This appears to be the latest version anyone has uploaded to AMO: https://people.mozilla.com/~kmaglione/lastpass_free_password_manager-4.1.36.64-an+fx.xpi

It's 15MB... :( And it loads a 750K minified content script into every page... :(

Comment 19

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #18)
> This appears to be the latest version anyone has uploaded to AMO:
> https://people.mozilla.com/~kmaglione/lastpass_free_password_manager-4.1.36.
> 64-an+fx.xpi
> 
> It's 15MB... :( And it loads a 750K minified content script into every
> page... :(

Anthony, can we somehow raise this issue with LastPass?

Comment 20

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #19)
> (In reply to Kris Maglione [:kmag] from comment #18)
> > This appears to be the latest version anyone has uploaded to AMO:
> > https://people.mozilla.com/~kmaglione/lastpass_free_password_manager-4.1.36.
> > 64-an+fx.xpi
> > 
> > It's 15MB... :( And it loads a 750K minified content script into every
> > page... :(
> 
> Anthony, can we somehow raise this issue with LastPass?

Bill has LastPass contacts.  I have a question from them about which version we should focus on for performance improvement measurements.  Will ask Bill.

Comment 21

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> It looks like XrayWrappers only use one of the two proxy extra slots. We
> could potentially use the second one to cache the expando object.

It turns out that this slot is used by the GC to maintain its gray list. We could potentially store it on the holder object instead, though.

Comment 22

[Andrew Overholt [:overholt]]

(In reply to Bill McCloskey (:billm) from comment #0)
> I'm doing some profiling of LastPass. It does a lot of iteration over
> <input> elements. One thing I noticed is that our expando code does a lot of
> work just to access a property. You can see it in this profile:
> 
> https://clptr.io/2kyucou
> 
> A lot of the stacks in that region contain getExpandoObjectInternal. It
> would be nice if we could do some caching here.

Now that a bunch of the blockers here are fixed is this still a problem?

Comment 23

[Kris Maglione [:kmag]]

According to the last profile I ran, it is. Now most of the time is spent in the hash lookup rather than the principal equality checks, though.

Comment 24

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

Yeah, I think we have a long way to go here, but this bug probably isn't the best place to track things. I'm going to dupe this over to bug 613498, which is more general and has some new dependent bugs.

Comment 25

[Ed Morley [:emorley]]

If it helps there's a new profile in bug 1345633 comment 5 using LastPass 4.1.45.3a.