Summary: | [V8] TypedArray binding performance improvements | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | arno. <a.renevier> | ||||||||||
Component: | WebGL | Assignee: | 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
arno.
2012-07-27 15:41:45 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 Created attachment 155595 [details]
updated patch
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. Created attachment 155650 [details]
updated patch to address review
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.) Created attachment 155908 [details]
updated patch
Comment on attachment 155908 [details]
updated patch
Thanks, looks good. r=me
Comment on attachment 155908 [details] updated patch Clearing flags on attachment: 155908 Committed r124386: <http://trac.webkit.org/changeset/124386> All reviewed patches have been landed. Closing bug. |