Bug 92556 – TypedArray set method is slow when called with another typed array

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[arno.]

Hi,
in #90838, we made TypedArray constructor much faster by casting the argument to a typed array when possible, and using typed array methods instead of going through the javascript path.
We can probably introduce the same improvement for the set method (more specifically, in the setWebGLArrayHelper function).

[arno.]

Created attachment 155327 [details]
patch proposal

with the patch, times for performance test drops from 2600ms to 250ms on my computer

[Kenneth Russell]

Comment on attachment 155327 [details]
patch proposal

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

Good performance improvement but needs more cleanup.

> Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:63
> +        || offset + length < offset) {

Please try to figure out a way to avoid duplicating these security related checks. For example, if you pushed the majority of this code into the C++ code instead of doing it in the bindings, you could define an enum return code and conditionally call setDOMException based on that return code.

> Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:109
> +        break;

Refactor this so the code isn't duplicated between here and constructArrayBufferViewWithTypedArrayArgument below. Since this block doesn't refer to any JSC concepts, you could even put it in the C++ code so it could be shared between the JSC and V8 bindings.

[arno.]

Created attachment 155643 [details]
updated patch

This patch factorize security check and factorize also buffer copy between constructArrayBufferViewWithTypedArrayArgument and setWebGLArrayWithTypedArrayArgument.

[WebKit Review Bot]

Attachment 155643 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'PerformanceTests/Bindings/typed-array-set-..." exit_code: 1
Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:52:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:58:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:120:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[arno.]

Created attachment 155645 [details]
updated patch

Oups. I didn't commit --amend before submitting

[Kenneth Russell]

Comment on attachment 155645 [details]
updated patch

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

Thanks, this looks better. One minor naming convention issue I'd appreciate if you'd clean up before committing. r=me

> Source/WTF/wtf/TypedArrayBase.h:76
> +    bool inbound(unsigned offset, unsigned pos) const

Please name this some sort of verb like "checkInboundData".

[arno.]

Created attachment 155922 [details]
updated patch

[Kenneth Russell]

Comment on attachment 155922 [details]
updated patch

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

You don't need my re-review, so clearing the r? bit. If you fix up the ChangeLog, any WebKit committer can set the cq+ bit on a revised patch.

> Source/WTF/ChangeLog:12
> +        (WTF::TypedArrayBase::inbound):

Rename it here.

> Source/WebCore/ChangeLog:3
> +        TypedArray set speedup

Did you mean to add this?

[arno.]

Created attachment 156125 [details]
updated patch

[Kenneth Russell]

Comment on attachment 156125 [details]
updated patch

Looks good.

[WebKit Review Bot]

Comment on attachment 156125 [details]
updated patch

Clearing flags on attachment: 156125

Committed r124483: <http://trac.webkit.org/changeset/124483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.