Bug 92557

Summary: [V8] TypedArray binding performance improvements
Product: WebKit Reporter: arno. <a.renevier>
Component: WebGLAssignee: arno. <a.renevier>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, kbr, ulan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch proposal
none
updated patch
none
updated patch to address review
none
updated patch none

Description arno. 2012-07-27 15:41:45 PDT
Hi,
#90838, #92518 and #92556 improve TypedArray performances in JavaScriptCore.
This bug is to implement those improvements in v8.
Comment 1 arno. 2012-07-31 10:41:44 PDT
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
Comment 2 arno. 2012-07-31 11:40:58 PDT
Created attachment 155595 [details]
updated patch
Comment 3 Kenneth Russell 2012-07-31 11:53:10 PDT
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.
Comment 4 arno. 2012-07-31 15:38:18 PDT
Created attachment 155650 [details]
updated patch to address review
Comment 5 Kenneth Russell 2012-08-01 14:42:07 PDT
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.)
Comment 6 arno. 2012-08-01 15:51:21 PDT
Created attachment 155908 [details]
updated patch
Comment 7 Kenneth Russell 2012-08-01 16:06:50 PDT
Comment on attachment 155908 [details]
updated patch

Thanks, looks good. r=me
Comment 8 WebKit Review Bot 2012-08-01 16:53:23 PDT
Comment on attachment 155908 [details]
updated patch

Clearing flags on attachment: 155908

Committed r124386: <http://trac.webkit.org/changeset/124386>
Comment 9 WebKit Review Bot 2012-08-01 16:53:27 PDT
All reviewed patches have been landed.  Closing bug.