Bug 92557 – [V8] TypedArray binding performance improvements

• Component: WebGL
• Status: RESOLVED FIXED

[arno.]

Hi,
#90838, #92518 and #92556 improve TypedArray performances in JavaScriptCore.
This bug is to implement those improvements in v8.

[arno.]

Created attachment 155583 [details]
patch proposal

It appears some of the optimisations made in bug #90838 and bug #92556 are not interesting. Actually, iterating over the c++ array elements to set the value is slower than the current copyElements method (about two times slower). We can still improve performances by using memcpy if the array we construct from is the same type. Also, we can benefit from creating arrays with createUninitialized. With this patch, Bindings/typed-array-construct-from-typed.html time goes from 600ms to around 550ms, and Bindings/typed-array-construct-from-same-type.html goes from 400ms to 55ms

[arno.]

Created attachment 155595 [details]
updated patch

[Kenneth Russell]

Comment on attachment 155595 [details]
updated patch

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

Thanks for the patch. It looks good overall, but r- because of one needed null check. Also one comment about sharing code.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:142
> +        RefPtr<ArrayClass> array = ArrayClass::createUninitialized(length);

Even though currently this will never return 0 (Chrome will crash before failing a call to malloc), please do the same null check as below. Also please share the error string.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:150
> +        return args.Holder();

It would be really nice if this block of code weren't duplicated.

[arno.]

Created attachment 155650 [details]
updated patch to address review

[Kenneth Russell]

Comment on attachment 155650 [details]
updated patch to address review

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

Thanks, this looks almost perfect. One issue which is confusing, therefore the r-. If you can clean this up I'll r+ the next version.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:48
> +v8::Handle<v8::Value> wrapWebGLArrayBuffer(const v8::Arguments& args, WrapperTypeInfo* type, ArrayClass array, v8::ExternalArrayType arrayType, bool hasIndexer)

This is a misleading name. It has nothing to do with ArrayBuffers. Please call it "wrapTypedArray" or "wrapArrayBufferView". (The use of "WebGL" throughout this file is historical and is something that should be cleaned up in a separate patch.)

[arno.]

Created attachment 155908 [details]
updated patch

[Kenneth Russell]

Comment on attachment 155908 [details]
updated patch

Thanks, looks good. r=me

[WebKit Review Bot]

Comment on attachment 155908 [details]
updated patch

Clearing flags on attachment: 155908

Committed r124386: <http://trac.webkit.org/changeset/124386>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.