Bug 92557 - [V8] TypedArray binding performance improvements
Summary: [V8] TypedArray binding performance improvements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: arno.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-27 15:41 PDT by arno.
Modified: 2012-08-01 16:53 PDT (History)
6 users (show)

See Also:


Attachments
patch proposal (11.85 KB, patch)
2012-07-31 10:41 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (11.99 KB, patch)
2012-07-31 11:40 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch to address review (14.82 KB, patch)
2012-07-31 15:38 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (14.81 KB, patch)
2012-08-01 15:51 PDT, arno.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.