852092 - Improve DOM list ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

We should be more clever about when we want to generate a stub. Right now, at the time that we generate the stub, if the object has an expando that shadows then we generate a stub that bails out if there's an expando. Which means that if we loop over a list that has an expando we keep bailing out and regenerating a stub until we hit the max number of stubs. I think we should just not generate a stub if we have an object with a shadowing expando.
The Ion version of the IC also doesn't have support for slot reads, which means it fails for all DOM methods.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Part 1 v1

This just factors out the code that generates the guards for the DOM lists in the Ion IC. This only moves and reindents code (and changes one comment).

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Part 2 v1

This does two things, it stops us from generating stubs for objects that we know have an expando (so that we don't generate a stub and then immediately hit the guard and generate another stub and so on) and it make the Ion IC work for slot reads on the prototypes of ListBase proxies (all DOM methods are stored in slots on the prototypes).

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 729143 [details] [diff] [review]
Part 1 v1

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

::: js/src/ion/IonCaches.cpp
@@ +334,5 @@
>      CodeOffsetJump exitOffset;
>      CodeOffsetJump rejoinOffset;
>      CodeOffsetLabel stubCodePatchOffset;
>  
> +    void generateListBaseChecks(JSContext *cx, MacroAssembler &masm, JSObject *obj,

Nit: move this out of the class:

static void
GenerateListBaseChecks(...)
{

}

(This patch will conflict with bug 849469, which refactored the other methods.)

@@ +335,5 @@
>      CodeOffsetJump rejoinOffset;
>      CodeOffsetLabel stubCodePatchOffset;
>  
> +    void generateListBaseChecks(JSContext *cx, MacroAssembler &masm, JSObject *obj,
> +                                PropertyName *propName, Register object, Label &stubFailure)

Nit: use "Label *" for consistency with the other functions.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 729155 [details] [diff] [review]
Part 2 v1

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

Nice.

::: js/src/ion/IonCaches.cpp
@@ +404,5 @@
>                                               failures);
>  
> +        bool isCacheableListBase = IsCacheableListBase(obj);
> +        Label listBaseFailures;
> +        if (isCacheableListBase) {

Nit: single line body so no { }.

@@ +526,1 @@
>          }

Nit: missed it in the previous patch, but no {} here either.

@@ +898,4 @@
>      {
>          // With Proxies, we cannot garantee any property access as the proxy can
>          // mask any property from the prototype chain.
> +        if (!checkObj->isProxy())

At the start of function we do:

if (!checkObj || !checkObj->isNative())
    return false;

Proxies are non-native, so you can turn this into an assert:

JS_ASSERT(!checkObj->isProxy());

Comment 5

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/202716eedf73
https://hg.mozilla.org/integration/mozilla-inbound/rev/7538d3ff1e14

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/202716eedf73
https://hg.mozilla.org/mozilla-central/rev/7538d3ff1e14