Make own property gets of expandos on DOM proxies faster

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
7 months ago

People

(Reporter: bz, Assigned: jandem)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla53
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(5 attachments, 10 obsolete attachments)

244 bytes, text/html
Details
3.21 KB, patch
Details | Diff | Splinter Review
9.13 KB, patch
Details | Diff | Splinter Review
13.12 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
2.94 KB, patch
Details | Diff | Splinter Review
Created attachment 8368161 [details]
Testcase

The testcase in bug 962256 basically does this:

  document.id = function() { /* stuff */ };

and then later does a bunch of document.id() calls.  For us, the get of document.id ends up taking the slow path through the proxy get IC, because all we know is that the property is shadowed.

But in practice, we have different kinds of shadowing.  There's shadowing due to the property living on the unforgeable holder.  There's shadowing due to the property living on the expando object.  And there's shadowing due to the property being found by the named getter magic.  This case is the second of those.

In our current impl we seem to look at the expando object before the named getter.  Per spec, I think it should be after, actually...

Anyway, if we said in our shadowing return value what sort of shadowing is going on, it seems like the JIT could in fact optimize the "shadows due to expando" case much better than we do right now; basically just get it off the expando object if our guards pass.
Blocks: 580070
Blocks: 962256
Component: JavaScript Engine → JavaScript Engine: JIT
Blocks: 1133423
Blocks: 1128157
Depends on: 1133746
Summary: Make own property access to expandos on DOM proxies faster → Make own property gets of expandos on DOM proxies faster
Blocks: 1218972
Jan, this is the sort of thing that your IC rearch might make much simpler to do...
Created attachment 8822766 [details] [diff] [review]
Old WIP for baseline IC for this
Created attachment 8822767 [details] [diff] [review]
Old WIP Ion bits
I'm not going to have time to work on this stuff in the foreseeable future, and chances are those WIP patches are not the right way forward anyway; instead CacheIR is the way to go.
(Assignee)

Comment 5

8 months ago
Yeah these things should be much easier now. NI myself to see if I can get something working, but if it's complicated I will work on this after doing other IC work.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 6

8 months ago
Created attachment 8822888 [details] [diff] [review]
WIP

Here's the CacheIR version of your Baseline patch. It improves the testcase attached here from 68 ms to 6 ms or so on OS X 64-bit.

bz, a question about ExpandoAndGeneration (see the XXX in the patch). When we have an ExpandoAndGeneration at IC compile time, does the shape guard ensure we always have an ExpandoAndGeneration at runtime as well? If not, we have to add an extra guard instead of the LoadDOMExpandoFromExpandoAndGeneration codegen in this patch.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8822888 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 7

8 months ago
Created attachment 8822935 [details] [diff] [review]
WIP v2

I just remembered the expando getter testcase in bug 1128157, so this patch supports expando getters as well. For the jsperf test (http://jsperf.com/js-getter-as-perfomance-boost) from that bug I get:

Normal node list: 380,326 ops/second (no change)
"Boosted" node list: 24,046 ops/second -> 701,549 ops/second

So the boosted node list is now 30 times faster and much faster than the normal length getter.
(Assignee)

Comment 8

8 months ago
Created attachment 8822936 [details] [diff] [review]
WIP v2

Sorry for the bugspam. This fixes a minor issue where Baseline would fall back to the generic shadowing-proxy code when we had a scripted getter without JIT code. Now we check isTemporarilyUnoptimizable_ and wait until the getter has JIT code instead of attaching the slower stub that calls Proxy::get.

When I disable Ion, I get:

Normal node list: 194,763 ops/second
"Boosted" node list: 347,839 ops/second

So this is very decent for Baseline: Ion is only about twice as fast. Ion could be even faster for the "boosted" case if it inlined the getter, bug 1324561 should let us do that I think.
Attachment #8822935 - Attachment is obsolete: true
Duplicate of this bug: 1128157
Blocks: 1259927
(Assignee)

Comment 10

8 months ago
Created attachment 8823072 [details] [diff] [review]
Part 1 - Rename CacheIR instructions, add comment

This renames some of the CacheIR instructions we use for DOM proxies to be clearer, and also adds some documentation. No change in behavior.
Attachment #8822766 - Attachment is obsolete: true
Attachment #8822767 - Attachment is obsolete: true
Attachment #8822888 - Attachment is obsolete: true
Attachment #8822936 - Attachment is obsolete: true
Attachment #8822888 - Flags: feedback?(bzbarsky)
Attachment #8823072 - Flags: review?(evilpies)
Attachment #8823072 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

8 months ago
Created attachment 8823073 [details] [diff] [review]
Part 2 - Optimize expando properties

Regarding my question in comment 6, I decided to just reuse LoadDOMExpandoValueGuardGeneration. That means we don't need any new CacheIR instructions to implement this, and it's also safer. It does add extra guards in the ExpandoAndGeneration case, but I don't see much of a perf difference.
Attachment #8823073 - Flags: review?(evilpies)
Attachment #8823073 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

8 months ago
Created attachment 8823074 [details] [diff] [review]
Part 2 - Optimize expando properties
Attachment #8823073 - Attachment is obsolete: true
Attachment #8823073 - Flags: review?(evilpies)
Attachment #8823073 - Flags: review?(bzbarsky)
Attachment #8823074 - Flags: review?(evilpies)
Attachment #8823074 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

8 months ago
Created attachment 8823075 [details] [diff] [review]
Part 3 - Fix GuardDOMExpandoShapeIfPresent

Just noticed GuardDOMExpandoShapeIfPresent is missing an is-object check. Regression from bug 1319437, it was in the old Baseline and Ion code.
Attachment #8823075 - Flags: review?(evilpies)
Attachment #8823072 - Flags: review?(evilpies) → review+
Comment on attachment 8823075 [details] [diff] [review]
Part 3 - Fix GuardDOMExpandoShapeIfPresent

Review of attachment 8823075 [details] [diff] [review]:
-----------------------------------------------------------------

This guarding against the PrivateValue for ExpandoAndGeneration, I assume?
Attachment #8823075 - Flags: review?(evilpies) → review+
Comment on attachment 8823074 [details] [diff] [review]
Part 2 - Optimize expando properties

Review of attachment 8823074 [details] [diff] [review]:
-----------------------------------------------------------------

How well is this covered by tests on the DOM side? Especially scripted and native getters on the expando.

::: js/src/jit/CacheIR.cpp
@@ +580,5 @@
> +        CanAttachNativeGetProp(cx_, expandoObj, id, &holder, &propShape, pc_, engine_,
> +                               canAttachGetter_, isTemporarilyUnoptimizable_);
> +    if (canCache != CanAttachReadSlot && canCache != CanAttachCallGetter)
> +        return false;
> +    if (holder != expandoObj)

This sort of obfuscates the condition. holder is either nullptr or expandoObject, because expandos don't have a prototype. I would suggest changing this to

if (!holder)
return false;
MOZ_ASSERT(holder == expandoObj);

@@ +606,5 @@
> +        // Call the getter. Note that we pass objId, the DOM proxy, as |this|
> +        // and not the expando object.
> +        MOZ_ASSERT(canCache == CanAttachCallGetter);
> +        EmitCallGetterResultNoGuards(writer, expandoObj, expandoObj, propShape, objId);
> +    }

So much shared code \o/
Attachment #8823074 - Flags: review?(evilpies) → review+
(Assignee)

Comment 16

8 months ago
(In reply to Tom Schuster [:evilpie] from comment #15)
> How well is this covered by tests on the DOM side? Especially scripted and
> native getters on the expando.

I'll forward this to bz. If we need new tests for this, where should I add them?
Flags: needinfo?(bzbarsky)
(Reporter)

Updated

8 months ago
Blocks: 875815
Sorry for the lag here; I was on PTO.

> When we have an ExpandoAndGeneration at IC compile time, does the shape guard
> ensure we always have an ExpandoAndGeneration at runtime as well?

Whether we have an ExpandoAndGeneration is entirely determined by our JSClass.  So if the shape guard ensures a particular JSClass, then it ensures this.  I believe it does that, right?

> How well is this covered by tests on the DOM side? Especially scripted and native getters on the expando.

Probably not very well.  We should add some tests.  They can go in dom/bindings/test as mochitests.
Flags: needinfo?(bzbarsky)
And we should test both sorts of objects (simple expando and expando-and-generation), I guess.  You can get a thing with a generation by getting document.documentElement.dataset.  You can get a thing with a simple expando by getting document.getElementsByTagName("*").
(Assignee)

Comment 19

7 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> Whether we have an ExpandoAndGeneration is entirely determined by our
> JSClass.  So if the shape guard ensures a particular JSClass, then it
> ensures this.  I believe it does that, right?

Yes, it does. With my current patches, we do guard we have an ExpandoAndGeneration (we did/do the same for the unshadowed guards). I think I'd prefer keeping these guards for now, to not rely too much on the Gecko setup. It doesn't affect perf much - this is a big win anyway. (I suppose a follow-up could add debug-only guards that assert on failure...)

> Probably not very well.  We should add some tests.  They can go in
> dom/bindings/test as mochitests.

Thanks, I'll write them today.
(Assignee)

Updated

7 months ago
Depends on: 1328835
(Assignee)

Comment 20

7 months ago
Created attachment 8824020 [details] [diff] [review]
Part 4 - Tests, WIP

I hit bug 1328835 for the dataset tests, but for NodeList they all pass. I also confirmed these tests fail when commenting out some of the guards we emit.
> I hit bug 1328835 for the dataset tests

Sorry for the bad advice there.  Defining accessor properties on dataset is actually impossible per spec...

Use an HTMLFormElement instead.  See bug 1328835 comment 2.
(Assignee)

Comment 22

7 months ago
Created attachment 8824103 [details] [diff] [review]
Part 4 - Tests

Thanks, everything works fine with HTMLFormElement. I double checked we attach a stub for all of these cases and I also verified HTMLFormElement has an ExpandoAndGeneration*.
Attachment #8824020 - Attachment is obsolete: true
Attachment #8824103 - Flags: review?(bzbarsky)
Comment on attachment 8823072 [details] [diff] [review]
Part 1 - Rename CacheIR instructions, add comment

>+// * GuardDOMExpandoShapeIfPresent: takes an expando Value as input, then guards

This is probably fine, but the key way this is used is to guard that an expando does not shadow something, which just happens to be true if there is no expando.  So it's more like GuardDOMExpandoMissingOrHasMatchingShape, maybe...

Either way, though.

r=me
Attachment #8823072 - Flags: review?(bzbarsky) → review+
Comment on attachment 8823074 [details] [diff] [review]
Part 2 - Optimize expando properties

>+GetPropIRGenerator::tryAttachDOMProxyExpando(HandleObject obj, ObjOperandId objId, HandleId id)

>+        MOZ_ASSERT(!expandoVal.isUndefined());

How about passing "How did a missing expando manage to shadow things?" as the second arg here?

>+        expandoAndGeneration = (ExpandoAndGeneration*)expandoVal.toPrivate();

Not static_cast?  Similar elsewhere.

>+        expandoValId = writer.loadDOMExpandoValueGuardGeneration(objId, expandoAndGeneration);

Hmm.  So I think this could actually bite us in interesting ways in a non-microbenchmark situation.  This adds two unnecessary guards:

1)  A guard on the pointer identity of the ExpandoAndGeneration*.
2)  A guard on the generation.

If we have two different objects (e.g. two different forms) come through here, the first guard will cause us to miss the IC, since it's basically a guard on the object identity of "obj".  If we have the same object come through twice with a DOM mutation in between, the second guard will cause us to miss the IC.

Note that we are already assuming that the shape check on "obj" ensures that we have an ExpandoAndGeneration if it passes; loadDOMExpandoValueGuardGeneration assumes just that.  So I don't think doing an explicit no-guards load of the expando value from the ExpandoAndGeneration would be any less safe than what we have now.

So it really might be a good idea to have a CacheIR instruction for getting the expando from a known expando-and-generation proxy, to avoid those pitfalls.

The rest of this looks beautiful.  r=me, but please to think about the guard situation.
Attachment #8823074 - Flags: review?(bzbarsky) → review+
Comment on attachment 8823075 [details] [diff] [review]
Part 3 - Fix GuardDOMExpandoShapeIfPresent

CacheIRWriter::GuardDOMExpandoShapeIfPresent is CheckDOMProxyExpandoDoesNotShadow which only calls it when expandoVal.isObject().  Since at that point we've already shape-checked the obj argument to CheckDOMProxyExpandoDoesNotShadow, we know "obj" is a DOM proxy that has either undefined or an object in the expando slot.  So this guard that's being added is completely redundant, as far as I can tell.

In a debug build, it might be nice to assert here or something, I guess... But I see no reason to add this guard.
Attachment #8823075 - Flags: review-
> CacheIRWriter::GuardDOMExpandoShapeIfPresent is CheckDOMProxyExpandoDoesNotShadow

I meant "The only caller of CacheIRWriter::GuardDOMExpandoShapeIfPresent ... [etc]"
Comment on attachment 8824103 [details] [diff] [review]
Part 4 - Tests

r=me; this looks great.  Thank you!
Attachment #8824103 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 28

7 months ago
Created attachment 8824398 [details] [diff] [review]
Part 1 - Rename CacheIR instructions, add comment

This version renames GuardDOMExpandoObject to GuardDOMExpandoMissingOrGuardShape intead of GuardDOMExpandoShapeIfPresent.
Attachment #8823072 - Attachment is obsolete: true
Attachment #8824398 - Flags: review?(bzbarsky)
(Assignee)

Comment 29

7 months ago
Created attachment 8824399 [details] [diff] [review]
Part 2 - Optimize expando properties

This adds a LoadDOMExpandoValueIgnoreGeneration instruction that does not have the guards we have for LoadDOMExpandoValueGuardGeneration.

It does check in debug builds that the value is a PrivateValue (well, a double, as we can't distinguish these).
Attachment #8823074 - Attachment is obsolete: true
Attachment #8824399 - Flags: review?(evilpies)
Attachment #8824399 - Flags: review?(bzbarsky)
(Assignee)

Comment 30

7 months ago
Created attachment 8824400 [details] [diff] [review]
Part 3 - Add assert to GuardDOMExpandoMissingOrGuardShape

In debug builds assert the Value is an object.
Attachment #8823075 - Attachment is obsolete: true
Attachment #8824400 - Flags: review?(bzbarsky)
(Assignee)

Comment 31

7 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> In our current impl we seem to look at the expando object before the named
> getter.  Per spec, I think it should be after, actually...

Do you still think we should look at the named getter first? If we need to do that, we do have to the guard on the generation, right?
Flags: needinfo?(bzbarsky)
> Do you still think we should look at the named getter first?

No.  The spec got changed to align with our impl here, because what the spec used to say was an ES invariant violation.  See https://github.com/heycam/webidl/issues/152 and bug 1297411.

So at this point, the expando object always comes first, for both types of DOM proxies.

Will do the reviews in ~2 hours.
Flags: needinfo?(bzbarsky)
Comment on attachment 8824399 [details] [diff] [review]
Part 2 - Optimize expando properties

Review of attachment 8824399 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, I leave it to bz to make sure that ignoring the generation is okay. Honestly I am not even sure what that is good for.
Attachment #8824399 - Flags: review?(evilpies) → review+
Comment on attachment 8824398 [details] [diff] [review]
Part 1 - Rename CacheIR instructions, add comment

r=me
Attachment #8824398 - Flags: review?(bzbarsky) → review+
Comment on attachment 8824399 [details] [diff] [review]
Part 2 - Optimize expando properties

r=me.  Thank you!
Attachment #8824399 - Flags: review?(bzbarsky) → review+
Comment on attachment 8824400 [details] [diff] [review]
Part 3 - Add assert to GuardDOMExpandoMissingOrGuardShape

r=me
Attachment #8824400 - Flags: review?(bzbarsky) → review+
Sorry for the lag here.  :(

> Honestly I am not even sure what that is good for

The generation is for when the set of own properties produced by the named getter changes.  This can affect whether we shadow properties up our proto chain, so anything that finds a prop on the proto chain needs to guard on the generation.  In this case, though, we're working with the expando object, which is looked at before anything else, and hence it doesn't matter what happens to the named getter's property set: if the prop is on the expando object, then we should get it from there.

Comment 38

7 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5fe007fc9c
part 1 - Rename some CacheIR instructions related to DOM proxies. r=bz,evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/744e3371701b
part 2 - Add inline caches for getting DOM expando properties. r=bz,evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/f004cbe6f011
part 3 - Add an is-object debug assert to GuardDOMExpandoMissingOrGuardShape. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9743055774c
part 4 - Add tests. r=bz
(Assignee)

Comment 39

7 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #37)
> Sorry for the lag here.  :(

No worries, after almost 3 years a few days more or less really doesn't matter ;)
(Assignee)

Comment 40

7 months ago
I was wondering how much this matters for normal websites, so I added some logging to count the number of times these expando stubs *succeed* (all guards pass and we optimized the lookup).

Just starting Firefox and closing it results in 5 hits. On pretty much all non-static websites (Gmail, GDocs, Amazon, GitHub) I see at least a few hundred hits, pretty nice.

The more interesting cases:

* Loading Treeherder: > 700 hits
* Loading facebook.com/BarackObama and scrolling down: > 1500 hits
* Loading cnn.com: > 2000 hits
* Loading twitter.com/firefox and scrolling down a little: > 2500 hits

So this should shave off CPU cycles for everyone, and I'm sure there are even more excessive cases.

Comment 41

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f5fe007fc9c
https://hg.mozilla.org/mozilla-central/rev/744e3371701b
https://hg.mozilla.org/mozilla-central/rev/f004cbe6f011
https://hg.mozilla.org/mozilla-central/rev/f9743055774c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 42

7 months ago
Is it possible to uplift this patch to Aurora (52), before it's too late for older Windows?
(Assignee)

Comment 43

7 months ago
(In reply to Krzysztof from comment #42)
> Is it possible to uplift this patch to Aurora (52), before it's too late for
> older Windows?

Unfortunately not. The patches here are pretty small, but for this to work on 52 we would have to backport a lot of other bugs, like bug 1322093.
> so I added some logging to count the number of times these expando stubs *succeed*

Nice!  Thank you again for doing this!
You need to log in before you can comment on or make changes to this bug.