Bug 56168 – Avoid slow-path for put() in Array.slice()

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Andreas Kling]

Array.slice() currently creates the returned array early on.
We can defer it until we know the number of elements to include, and use the CreateCompact mechanism to avoid the put() slow-path when populating the array.

[Andreas Kling]

Created attachment 85429 [details]
Proposed patch

[Andreas Kling]

Created attachment 85455 [details]
Proposed patch v2

Do the constructEmptyArray() short-circuit for the (begin == end) case as well.

[Darin Adler]

Comment on attachment 85455 [details]
Proposed patch v2

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

I’m pretty sure this is wrong. This doesn’t handle the case where there are holes in the object we are slicing. Both arrays and non-array objects can have empty slots with undefined values. The old code handled this. Are there any test cases that cover it?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:484
> +    JSArray* resObj = new (exec) JSArray(exec->lexicalGlobalObject()->arrayStructure(), sliceCount, CreateCompact);

Lets go nuts with this patch and call this result. There’s no need for a separate local variable named result. JSValue::encode should work fine on a JSArray*.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:489
> +    for (unsigned k = 0; k < sliceCount; k++)
> +        resObj->uncheckedSetIndex(globalData, k, getProperty(exec, thisObj, k + begin));

Shouldn’t we also stop on the first exception we see?

[Darin Adler]

There is probably a great way to do this, but I think we’ll have to have different code for the rare case where there is a hole in the passed-in array. Or perhaps I missed something.

[Oliver Hunt]

> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:489
> > +    for (unsigned k = 0; k < sliceCount; k++)
> > +        resObj->uncheckedSetIndex(globalData, k, getProperty(exec, thisObj, k + begin));
> 
> Shouldn’t we also stop on the first exception we see?

Yes.  And apparently a test for this as well.
var proto =  { get 0() { throw "argh" }, get 1() { fail("called second getter") }, length: 2} 
Array.prototype.slice.call(proto, ...)
Array.prototype.slice.call(Object.create(proto), ...)
var array = [];
array.__proto__ = proto;
array.length = 2;
Array.prototype.slice.call(array, ...)

(In reply to comment #4)
> There is probably a great way to do this, but I think we’ll have to have different code for the rare case where there is a hole in the passed-in array. Or perhaps I missed something.

Then general implementation for all of the array functions is sadly unpleasant.  You query the length and then iterate through as long as you can do canGet(i) (or whatever the function is called, i can't remember off the top of my head).   And then you continue with a generic get() until you reach the length.

We do need a nicer way of doing this, but as yet we haven't come up with one.

[Oliver Hunt]

(In reply to comment #5)
> > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:489
> > > +    for (unsigned k = 0; k < sliceCount; k++)
> > > +        resObj->uncheckedSetIndex(globalData, k, getProperty(exec, thisObj, k + begin));
> > 
> > Shouldn’t we also stop on the first exception we see?
> 
> Yes.  And apparently a test for this as well.
> var proto =  { get 0() { throw "argh" }, get 1() { fail("called second getter") }, length: 2} 
> Array.prototype.slice.call(proto, ...)
> Array.prototype.slice.call(Object.create(proto), ...)
> var array = [];
> array.__proto__ = proto;
> array.length = 2;
> Array.prototype.slice.call(array, ...)
> 
> (In reply to comment #4)
> > There is probably a great way to do this, but I think we’ll have to have different code for the rare case where there is a hole in the passed-in array. Or perhaps I missed something.
> 
> Then general implementation for all of the array functions is sadly unpleasant.  You query the length and then iterate through as long as you can do canGet(i) (or whatever the function is called, i can't remember off the top of my head).   And then you continue with a generic get() until you reach the length.
> 
> We do need a nicer way of doing this, but as yet we haven't come up with one.

And I just see that this code does a completely generic get() anyway (there's no optimisation for this being an array so that comment is unhelpful.  Unless we want to optimise for the array case)

[Andreas Kling]

Oh god I had no idea what I was doing in 2011.
I hope I feel this way about 2015, come 2019.