965992 - Make own property gets of expandos on DOM proxies faster

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jan, this is the sort of thing that your IC rearch might make much simpler to do...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Old WIP for baseline IC for this

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Old WIP Ion bits

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Jan de Mooij [:jandem]]

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.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Optimize expando properties

Comment 13

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 14

[Tom S [:evilpie]]

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?

Comment 15

[Tom S [:evilpie]]

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/

Comment 16

[Jan de Mooij [:jandem]]

(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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Boris Zbarsky [: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("*").

Comment 19

[Jan de Mooij [:jandem]]

(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.

Comment 20

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 22

[Jan de Mooij [:jandem]]

Attachment: 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*.

Comment 23

[Boris Zbarsky [: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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> CacheIRWriter::GuardDOMExpandoShapeIfPresent is CheckDOMProxyExpandoDoesNotShadow

I meant "The only caller of CacheIRWriter::GuardDOMExpandoShapeIfPresent ... [etc]"

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8824103 [details] [diff] [review]
Part 4 - Tests

r=me; this looks great.  Thank you!

Comment 28

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Rename CacheIR instructions, add comment

This version renames GuardDOMExpandoObject to GuardDOMExpandoMissingOrGuardShape intead of GuardDOMExpandoShapeIfPresent.

Comment 29

[Jan de Mooij [:jandem]]

Attachment: 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).

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add assert to GuardDOMExpandoMissingOrGuardShape

In debug builds assert the Value is an object.

Comment 31

[Jan de Mooij [:jandem]]

(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?

Comment 32

[Boris Zbarsky [: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.

Comment 33

[Tom S [:evilpie]]

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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8824398 [details] [diff] [review]
Part 1 - Rename CacheIR instructions, add comment

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8824399 [details] [diff] [review]
Part 2 - Optimize expando properties

r=me.  Thank you!

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8824400 [details] [diff] [review]
Part 3 - Add assert to GuardDOMExpandoMissingOrGuardShape

r=me

Comment 37

[Boris Zbarsky [:bzbarsky]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 39

[Jan de Mooij [:jandem]]

(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 ;)

Comment 40

[Jan de Mooij [:jandem]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Krzysztof]

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

Comment 43

[Jan de Mooij [:jandem]]

(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.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> so I added some logging to count the number of times these expando stubs *succeed*

Nice!  Thank you again for doing this!