Optimize lookups of plain data properties on WindowProxies

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
Posted patch PatchSplinter Review
It just occurred to me we optimize certain native getters in GetPropIRGenerator::tryAttachWindowProxy, but we can now very easily handle plain data properties there as well (initially I just ported this code from Baseline).

IonBuilder also has code to optimize this, but having IC support will help Baseline and also Ion when type information is bad.

I added some logging to see how many times this stub actually *succeeds* (we optimized the lookup), and pretty much every non-static website has at least a few hundred hits. Just opening Google Maps results in ~10,000 hits. Not too surprising as both |this.foo| and |window.foo| are used a lot.
Attachment #8828759 - Flags: review?(evilpies)
Assignee

Comment 1

2 years ago
Oh and note that it looks like the code should work for missing properties, which would be useful for things like |if (this.foo)|, but unfortunately we don't optimize that currently because of the global scope polluter on the global's prototype chain.

I think we could optimize this by shape guarding the global and objects on its prototype chain until we get to the GSP proxy and then emit CallProxyGetResult (we need to make sure we pass the right receiver, though). That should definitely be faster because it will avoid doing the lookup on the global and its proto. We could do something similar for other lookups with proxies on the prototype chain...
Great, somehow I hadn't noticed this was missing while looking at my CacheIR Analyzer output.

(In reply to Jan de Mooij [:jandem] from comment #1)
> Oh and note that it looks like the code should work for missing properties,
> which would be useful for things like |if (this.foo)|, but unfortunately we
> don't optimize that currently because of the global scope polluter on the
> global's prototype chain.
> 
> I think we could optimize this by shape guarding the global and objects on
> its prototype chain until we get to the GSP proxy and then emit
> CallProxyGetResult (we need to make sure we pass the right receiver,
> though).
Passing the right receiver is already possible from what I can tell. (At least with EmitCallGetterResultNoGuards). We will need to do this anyway if we want to support `super.property`.
> That should definitely be faster because it will avoid doing the
> lookup on the global and its proto. We could do something similar for other
> lookups with proxies on the prototype chain...
The win from not doing two property lookups is probably going to be dwarfed by the Proxy call, but maybe we win enough by not landing in the generic attach an IC logic.
Attachment #8828759 - Flags: review?(evilpies) → review+
Assignee

Comment 3

2 years ago
(In reply to Tom Schuster [:evilpie] from comment #2)
> Great, somehow I hadn't noticed this was missing while looking at my CacheIR
> Analyzer output.

Maybe because we attached a generic proxy stub for the WindowProxy?

> Passing the right receiver is already possible from what I can tell. (At
> least with EmitCallGetterResultNoGuards). We will need to do this anyway if
> we want to support `super.property`.

We will have to pass both the proxy + receiver to CallProxyGetResult, right? Currently we pass a single object because it's both the proxy and receiver.

> The win from not doing two property lookups is probably going to be dwarfed
> by the Proxy call, but maybe we win enough by not landing in the generic
> attach an IC logic.

Yeah but (I forgot to mention this) we also avoid having to "unwrap" the WindowProxy. That's probably pretty fast but it does involve the generic Proxy::get code as well.
(In reply to Jan de Mooij [:jandem] from comment #3)
> > Passing the right receiver is already possible from what I can tell. (At
> > least with EmitCallGetterResultNoGuards). We will need to do this anyway if
> > we want to support `super.property`.
> 
> We will have to pass both the proxy + receiver to CallProxyGetResult, right?
> Currently we pass a single object because it's both the proxy and receiver.
> 
You are of course correct, we were talking about proxies here, not getters. If we always passed the receiver in JIT code we could even remove ProxyGetProperty and use Proxy::get directly.
> > The win from not doing two property lookups is probably going to be dwarfed
> > by the Proxy call, but maybe we win enough by not landing in the generic
> > attach an IC logic.
> 
> Yeah but (I forgot to mention this) we also avoid having to "unwrap" the
> WindowProxy. That's probably pretty fast but it does involve the generic
> Proxy::get code as well.
Oh! This should totally give us some wins.

Comment 5

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e7992f1335
Optimize lookups of data properties on WindowProxies. r=evilpie

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48e7992f1335
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.