Bug 160836 – Array.prototype.map builtin should go on the fast path when constructor===@Array

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Caio Lima]

Created attachment 286038 [details]
Patch

[Caio Lima]

Created attachment 286039 [details]
Benchmark run 03

[Saam Barati]

Comment on attachment 286038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286038&action=review

> Source/JavaScriptCore/builtins/ArrayPrototype.js:264
> +    if (tempConstructor === @Array) {
> +        result = @Array(length);
> +        
> +        for (var i = 0; i < length; i++) {
> +            if (!(i in array))
> +                continue;
> +            var mappedValue = callback.@call(thisArg, array[i], i, array);
> +            @putByValDirect(result, i, mappedValue);
> +        }
> +        
> +        return result;
> +    }

I don't think this is quite right w.r.t the species symbol.

[Saam Barati]

Comment on attachment 286038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286038&action=review

> Source/JavaScriptCore/builtins/ArrayPrototype.js:281
> +    if (constructor === @undefined)

I think you can accomplish a speedup on ES6 bench just by making this "constructor === @Array || constructor === @undefined"

[Saam Barati]

Created attachment 286070 [details]
patch I was thinking of

This is like 6-8% faster on ES6 bench.
Keith, does this look correct to you

[Saam Barati]

(In reply to comment #5)
> Created attachment 286070 [details]
> patch I was thinking of
> 
> This is like 6-8% faster on ES6 bench.
> Keith, does this look correct to you

To elaborate, this will cause the FTL to recognize
that we're doing NewArrayWithSize instead
of compiling
x: JSConstant(Array)
y: Construct(@x, ...)

For example, this program will benefit from this:
```
let x = [1,2,3];
for (let i = 0; i < 10000; i++) {
   x.map(f=>f);
}
```

[Keith Miller]

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 286070 [details]
> > patch I was thinking of
> > 
> > This is like 6-8% faster on ES6 bench.
> > Keith, does this look correct to you
> 
> To elaborate, this will cause the FTL to recognize
> that we're doing NewArrayWithSize instead
> of compiling
> x: JSConstant(Array)
> y: Construct(@x, ...)
> 
> For example, this program will benefit from this:
> ```
> let x = [1,2,3];
> for (let i = 0; i < 10000; i++) {
>    x.map(f=>f);
> }
> ```

Yeah, I can believe that's a speedup. Although, I think we should support the Array constructor as an "intrinsic" in the DFG, it's probably a trivial change. It might also be worthwhile to make @Array a bytecode intrinsic function.

[Saam Barati]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created attachment 286070 [details]
> > > patch I was thinking of
> > > 
> > > This is like 6-8% faster on ES6 bench.
> > > Keith, does this look correct to you
> > 
> > To elaborate, this will cause the FTL to recognize
> > that we're doing NewArrayWithSize instead
> > of compiling
> > x: JSConstant(Array)
> > y: Construct(@x, ...)
> > 
> > For example, this program will benefit from this:
> > ```
> > let x = [1,2,3];
> > for (let i = 0; i < 10000; i++) {
> >    x.map(f=>f);
> > }
> > ```
> 
> Yeah, I can believe that's a speedup. Although, I think we should support
> the Array constructor as an "intrinsic" in the DFG, it's probably a trivial
> change. It might also be worthwhile to make @Array a bytecode intrinsic
> function.
I agree. I'll do this in another patch and make it a bytecode intrinsic:
https://bugs.webkit.org/show_bug.cgi?id=160867

[Saam Barati]

Created attachment 286096 [details]
patch

[Keith Miller]

Comment on attachment 286096 [details]
patch

r=me.

[Keith Miller]

I think we should we also apply this to the other Array prototype functions.

[Saam Barati]

Created attachment 286100 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 286100 [details]
patch for landing

Clearing flags on attachment: 286100

Committed r204488: <http://trac.webkit.org/changeset/204488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.