Bug 63644 – [v8] Improve performance of typed array set() taking Array

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The V8 binding for typed arrays' set() function taking a JavaScript Array as argument is disproportionately slower than the other bindings. The reason is the repeated crossings between the binding layer and the VM for each source array element that is fetched. This variant would be much more efficiently implemented in JavaScript using a simple loop. However, it isn't clear whether there are any other examples currently in the bindings of either implementing a method in the IDL in pure JavaScript, or replacing a method previously defined with one written in JS that might delegate to the original. We might need to add a post-initialization hook that runs after the global scope is initialized.

The associated Chromium bug for this issue, which contains a little more information, is http://crbug.com/84007 .

[Kenneth Russell]

http://crbug.com/84007 has been updated to indicate that there is a similar performance problem with the typed array constructors taking JS arrays as arguments.

[Ulan Degenbaev]

Created attachment 106509 [details]
Implement the 'set' method in JS.

[David Levin]

Comment on attachment 106509 [details]
Implement the 'set' method in JS.

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

Just a few comments. Also thanks to dslomov who helped.

> ChangeLog:19
> +        * ../../Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js: Added.

This ChangeLog is messed up. See other entries in this file.

> Source/WebCore/WebCore.gyp/WebCore.gyp:483
> +          'action_name': 'generateV8ArrayBufferViewCustomScript.h',

Similar items are named like this: generateV8ArrayBufferViewCustomScript

> Source/WebCore/WebCore.gyp/WebCore.gyp:497
> +          'message': 'Generating V8ArrayBufferViewCustomScript.h from V8ArrayBuferViewCustomScript.js',

typo: V8ArrayBuferViewCustomScript

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:32
> +namespace WebCore {

add blank line.

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:33
> +void V8BindingScripts::runScriptsInGlobalContext(v8::Handle<v8::Context> v8Context)

s/runScriptsInGlobalContext/runScript/ ?

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:37
> +                        sizeof(V8ArrayBufferViewCustomScript_js));

indentation is wrong.

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:40
> +}

add blank line.

> Source/WebCore/bindings/v8/V8BindingScripts.h:37
> +    static void runScriptsInGlobalContext(v8::Handle<v8::Context> v8Context);

param name adds no info. Omit it.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:2
>   * Copyright (C) 2008, 2009 Google Inc. All rights reserved.

Add 2011.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:34
> +                var n = source.length;

Please use descriptive variable names. (not "n")

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:41
> +            } else {

Don't use {} for single line statements.

> LayoutTests/fast/canvas/webgl/array-unit-tests.html:647
> +function testSettingFromArrayWithMinusZeroOffset(type, name) {

This test isn't used.

[Kenneth Russell]

Comment on attachment 106509 [details]
Implement the 'set' method in JS.

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

Modulo Dave's feedback, the code and new test cases look good. It would be nice to add versions of some of them (like testSettingFromArrayWithNonIntegerOffset) that take typed arrays rather than JavaScript arrays, to test both the new code path as well as the native version.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:40
> +                    this[i + offset] = source[i];

Would it be faster to hoist 'this' into a local variable outside the loop, or will Crankshaft take care of that?

[Ulan Degenbaev]

Created attachment 106692 [details]
Revised patch

Thank you for reviewing!

(In reply to comment #3)
> This ChangeLog is messed up. See other entries in this file.
I simplified it a bit and fixed file names, but I am not sure if you meant this. 

> s/runScriptsInGlobalContext/runScript/
I renamed it to "runScripts" to be consistent with the class name "BindingScripts".

All other comments are addressed as suggested.

(In reply to comment #4)
> It would be nice to add versions of some of them (like testSettingFromArrayWithNonIntegerOffset) that take typed arrays
Done. I added both versions and extracted duplicate code into "setWithValidOffset", "setWithInvalidOffset" functions.

> Would it be faster to hoist 'this' into a local variable outside the loop, or will Crankshaft take care of that?
I tried it but got no performance gain. Crankshaft seems to take care of it.

[Kenneth Russell]

Comment on attachment 106692 [details]
Revised patch

This looks great. Thanks for pushing it forward. Setting r+ / cq+.

[WebKit Review Bot]

Comment on attachment 106692 [details]
Revised patch

Clearing flags on attachment: 106692

Committed r94783: <http://trac.webkit.org/changeset/94783>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.