769911 - Generate ICs which see through ListBase proxies

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

DOM ListBase objects are proxies that work in pretty straightforward ways when accessing named properties, just checking for expando properties and forwarding to the prototype.  It should be possible for an IC to cache accesses on ListBase proxies which go through the prototype, which would have a couple nice properties:

(a) Speed these accesses up a good deal.
(b) Allow removal of a lot of ancillary code in dombindings.cpp for shape-based caching of accesses on ListBase objects.  Shapes should not be exposed through any APIs at all, and this is one of the few (maybe the only) place where shapes are used outside the JS engine.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Add logic for JM ICs to access properties off the prototype of ListBase proxies, rm shape caching code in dombindings.cpp previously necessary for these accesses, and rm GetObjectShape API.  The removal is necessary because using the caching code in dombindings actually inhibits our ability to generate an IC later, as a profiling bit indicating the PC might call a getter hook will not be set.  Plus it's a ton of logic that seems to just be duplicating what the ICs and property cache already do.  Let me know if there are still realistic use cases where this caching logic helps, can just robustify the IC to capture those too.

For this benchmark, reduced from one from bz:

  var list = document.getElementsByTagName("*");

  iters = 10000000;
  for (var i = 0; i < iters; ++i)
    list.length;

I get 1020ms before, 360ms after.  This could be reduced even further for .length accesses (any others of real interest for lists, besides indexed accesses themselves?), by inlining the length_getter native it's calling.  It's mainly checking for the presence of wrappers on the proxy, which I believe are obviated by the proxy handler check in the IC, and then calling GetLength on the wrapped native, which would be tricky to do because the type of the wrapped native is a template parameter of the ListBase.

Will the GetLength() always be a vcall on some nsISupports object, or can the wrapped list be e.g. an nsTArray?

Comment 2

[Brian Hackett [Laid off!]]

Oh, also, this will be simple to port to IM.  Does IM have ICs for calling getter hooks yet?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Brian Hackett (:bhackett) from comment #1)
> Created attachment 638111 [details] [diff] [review]
> patch
> 
> Add logic for JM ICs to access properties off the prototype of ListBase
> proxies, rm shape caching code in dombindings.cpp previously necessary for
> these accesses, and rm GetObjectShape API.  The removal is necessary because
> using the caching code in dombindings actually inhibits our ability to
> generate an IC later, as a profiling bit indicating the PC might call a
> getter hook will not be set.  Plus it's a ton of logic that seems to just be
> duplicating what the ICs and property cache already do.  Let me know if
> there are still realistic use cases where this caching logic helps, can just
> robustify the IC to capture those too.
> 
> For this benchmark, reduced from one from bz:
> 
>   var list = document.getElementsByTagName("*");
> 
>   iters = 10000000;
>   for (var i = 0; i < iters; ++i)
>     list.length;
> 
> I get 1020ms before, 360ms after.  This could be reduced even further for
> .length accesses (any others of real interest for lists, besides indexed
> accesses themselves?), by inlining the length_getter native it's calling. 
> It's mainly checking for the presence of wrappers on the proxy, which I
> believe are obviated by the proxy handler check in the IC, and then calling
> GetLength on the wrapped native, which would be tricky to do because the
> type of the wrapped native is a template parameter of the ListBase.
> 
> Will the GetLength() always be a vcall on some nsISupports object, or can
> the wrapped list be e.g. an nsTArray?

Currently, yes, but note bug 768692 (which will make length_getter look more like the attribute getters in the Paris bindings case).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Peter, this might obviate the need for your caching work on the new list bindings...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638111 [details] [diff] [review]
patch

Peter is a way better reviewer here than I am, I think.  Note that both he and I are on vacation for the next few days, though...

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 638111 [details] [diff] [review]
patch

I'm working on porting this to the new list bindings (see bug 768692) to see if we can avoid landing those with the same caching mechanism. But this looks broken to me, these proxies are used for DOM lists that have indexed and/or named properties on the object, but afaict this forces getting a non-expando named property to the prototype?

Comment 7

[Peter Van der Beken [:peterv]]

Note that in general WebIDL has this order for property lookup:

1) Indexed properties.
2) Own properties.
3) Properties from the prototype chain.
4) Named properties.

(I simplified to the most general case)

2 is what we call expando properties, I think the patch handles those fine. But I don't see how this allows us to deal with 4.

Comment 8

[Peter Van der Beken [:peterv]]

I'm also slightly worried that setting an expando on the object will kill performance, even if it doesn't shadow anything (I think our shape-based cache deals with that).

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Peter Van der Beken [:peterv] from comment #7)
> Note that in general WebIDL has this order for property lookup:
> 
> 1) Indexed properties.
> 2) Own properties.
> 3) Properties from the prototype chain.
> 4) Named properties.
> 
> (I simplified to the most general case)
> 
> 2 is what we call expando properties, I think the patch handles those fine.
> But I don't see how this allows us to deal with 4.

We only generate inline caches when the property being searched for is actually found.  So in the case where the access is not indexed/expando and the property is not on the prototype chain, a cache will not be generated and the normal getProperty() call will occur.

FWIW, this patch is green on try.

Comment 10

[Brian Hackett [Laid off!]]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> I'm also slightly worried that setting an expando on the object will kill
> performance, even if it doesn't shadow anything (I think our shape-based
> cache deals with that).

bz assured me that this case is rare, which made sense to me; it seems like lists will usually be short lived and won't get expandos from jQuery etc.  Have you measured how often this case occurs?

If expandos-on-lists is a common pattern, it would be straightforward to fix the IC to handle this case.  In the case when the list has an expando, the IC would load the expando object and shape check it.  This could be extended to allow the IC to get properties off the expando itself, and so on, but I'd really rather not add this complexity if websites aren't actually doing this.

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> FWIW, this patch is green on try.

Could you attach the version that compiles? I get

js/src/methodjit/PunboxAssembler.h:349:16: error: no matching constructor for initialization of 'JSC::AbstractMacroAssembler<JSC::X86Assembler>::ImmPtr'
        ImmPtr immPtr(PrivateValue(ptr).asRawBits());

Comment 12

[Peter Van der Beken [:peterv]]

I tried to fix that with just passing ptr, but then I get:

Assertion failure: !f.regs.sp[-2].isMagic(), at js/src/methodjit/PolyIC.cpp:2574

when running mochitest content/base/test/test_classList.html.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: patch

Oops, forgot to attach the updated patch to this bug.  This fixes the compilation error and a couple other bugs.  On try at:

https://tbpl.mozilla.org/?tree=Try&rev=1af98bce4ce7

Comment 14

[Peter Van der Beken [:peterv]]

Sorry for the long wait, I decided to check the expando-on-list case and it took me a while to get to that.

(In reply to Brian Hackett (:bhackett) from comment #10)
> bz assured me that this case is rare, which made sense to me; it seems like
> lists will usually be short lived and won't get expandos from jQuery etc. 
> Have you measured how often this case occurs?

No, but we will convert over more classes to the new proxies. We might not hit it right now for NodeList etc, but as an example we will convert CSSDeclaration to these bindings and with that I hit it almost immediately from our own chrome or on Google Maps (looks like mostly setting Webkit-only properties).

> If expandos-on-lists is a common pattern, it would be straightforward to fix
> the IC to handle this case.  In the case when the list has an expando, the
> IC would load the expando object and shape check it.  This could be extended
> to allow the IC to get properties off the expando itself, and so on, but I'd
> really rather not add this complexity if websites aren't actually doing this.

I think most lists won't have it, but it might be common on a few. I don't think we really care about the speed of getting/setting expandos yet, but I think we should avoid the slowdown for non-shadowed properties on the prototype if we can. Boris, opinions?

Comment 15

[Peter Van der Beken [:peterv]]

As an example, in my tree with CSSDeclaration converted to the new bindings doing this:

var style = document.documentElement.style;
var iters = 10000000;

for (var i = 0; i < iters; ++i)
  style.background;

style.WebKitFoo = "bar";

for (var i = 0; i < iters; ++i)
  style.background;

gives 450ms for the first loop and 2000ms for the second. That seems a rather big slowdown for no apparent reason (from the point of view of the web developer I mean).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, hm.  I hadn't thought about those turning into expandos, but you're right.

In that case, we probably do need to deal with the expando case.  :(

Comment 17

[Brian Hackett [Laid off!]]

Attachment: patch (120e3d9d86f1)

Beef up ListBase stubs to include a shape check on the expando object itself.  I get the same time in a microbenchmark whether the ListBase has an expando or not.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 646676 [details] [diff] [review]
patch (120e3d9d86f1)

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

It might be good to have someone from the JS team review the additional change in GetPropCompiler::generateStub, it looks right to me but I'm not an expert in that code.

::: js/src/methodjit/PolyIC.cpp
@@ +1245,5 @@
> +            if (!shapeMismatches.append(handlerGuard))
> +                return error();
> +
> +            Address expandoAddress(pic.objReg, JSObject::getFixedSlotOffset(GetListBaseExpandoSlot()));
> + 

Drop the trailing whitespace.

@@ +1252,5 @@
> +
> +            // Expando objects just hold any extra properties the object has
> +            // been given by a script, and have no prototype or anything else
> +            // that will complicate property lookups on them.
> +            JS_ASSERT(expando->isNative() && expando->getProto() == NULL);

Given that expando can be null this should probably be JS_ASSERT_IF(expando, ...).

@@ +1257,5 @@
> +
> +            if (expando && expando->nativeLookupNoAllocation(name) == NULL) {
> +                Jump expandoGuard = masm.testObject(Assembler::NotEqual, expandoAddress);
> +                if (!shapeMismatches.append(expandoGuard))
> +                    return error();

We never change the expando slot after it has been set to non-null the first time, does that mean we could drop this check? Is there a debug-only way to assert it if that's the case?

Comment 19

[Brian Hackett [Laid off!]]

The null check is still needed for the expando, as the test on the proxy handler will pass on objects both with and without expando objects attached.

https://hg.mozilla.org/integration/mozilla-inbound/rev/684958bd600b

Comment 20

[Ed Morley [:emorley]]

Backed out for failures in test_bug435425.html, however now that a few more results have come back looks like another bug is the culprit, sorry!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1184b4442755

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78aaaf0d8185

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78aaaf0d8185

Comment 23

[Brian Hackett [Laid off!]]

Followup fix for a typo which luke noticed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e377d8f4508

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e377d8f4508