836987 - BaselineCompiler: Fix SetElem_Dense to handle simple adding case

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Handle the case where SetElem is done on an object where the set is dense, and needs to enlarge the initializedLength and length by 1, but otherwise falls within the dense element capacity.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Patch

This kills nearly all the fallback hits for SETELEM in 3d-cube.

In order to do this, we can't re-use the SetElem_Dense stub - we have to create a new stub because there are different sets of conditions that need to be true for the shapes for the optimization to be valid.

The optimized stub in this case is pretty conservative.  It only applies to native objects whose prototype chain is entirely native, and where none of the shape chains for any of the prototypes contain a shape definition for an int32 property.

The only significant unoptimized SetElem case in 3d-cube is now in DrawQube where an array gets initialized from last-to-first (indexes 5 to 0) in reverse order.  This stub should be adaptable to handle that case too, I think, but I need to consider that for a bit.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 708959 [details] [diff] [review]
Patch

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

::: js/src/ion/BaselineIC.cpp
@@ +971,5 @@
>          JS_ASSERT(obj->isNative());
>          types::AddTypePropertyId(cx, obj, JSID_VOID, value);
>          break;
>        }
> +      case ICStub::SetElem_DenseAdd: {

Nit: you can combine this with the previous case (add "case ICStub::SetElem_DenseAdd:" after SetElem_Dense)

@@ +1902,5 @@
> +            return false;
> +
> +        // Scan shape chain.  If any shape exists with any integer property,
> +        // skip this optimization.
> +        RootedShape curShape(cx, curObj->lastProperty());

Instead of looping over the shapes you can use

if (curObj->isIndexed())
    return false;

@@ +1958,5 @@
>      }
>  
>      // Try to generate new stubs.
> +    if (obj->isNative() && index.isInt32() && index.toInt32() >= 0 &&
> +        !DenseSetElemStubExists(cx, stub, obj))

DenseSetElemStubExists should check for Add stubs too, so that we don't add a second stub if the capacity check fails or something.

@@ +1962,5 @@
> +        !DenseSetElemStubExists(cx, stub, obj))
> +    {
> +        bool addingCase;
> +        RootedObject lastProto(cx);
> +            

Nit: some trailing whitespace

@@ +1968,5 @@
> +                                    &addingCase, &lastProto))
> +        {
> +            if (addingCase) {
> +                RootedTypeObject type(cx, obj->getType(cx));
> +                RootedShape shape(cx, obj->lastProperty());

Nit: these are used in both arms of the |if|, so you can move them out of the |if|.

Comment 3

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/projects/ionmonkey/rev/98a7c69373d5

Comment 4

[Paul Wright]

Can this be marked FIXED?