Bug 90838 – constructing TypedArray from another TypedArray is slow

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[arno.]

Hi,
we can construct a TypedArray by passing another TypedArray. In that case, the value of the buffer is converted to the new type, and is copied to the new buffer.

This operation is quite slow (at least compared to mozilla). Each value is read as a JSValue from typed array as a JSObject, and then converted to a number.

May be we can detect if the constructor argument is a typed array, and get each value directly with array.item(idx);

[arno.]

Created attachment 151366 [details]
testcase

Testcase.

Average times are on my machine (ubuntu 32 bits):
firefox: 120ms/20ms
chrome: 1500ms/20ms
opera: 1400ms/1500ms
GtkLauncher from webkit trunk: 690ms/650ms

[arno.]

Created attachment 151370 [details]
first patch

first patch. It works by introducing a new JavaScript type: TypedArray. This is just an interface, that can be used from javascript to determine wether an object is an actual JSTypedArray.
TypedArrayBase template classes derive from TypedArray. Also, this patch introduce a virtual getType function. So, typed array can known their actual type. We can use this information to static_cast the TypedArray to the actual type, and then read the values from the source.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmak..." exit_code: 1
Source/WebCore/ChangeLog:21:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 33 files


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

[WebKit Review Bot]

Comment on attachment 151370 [details]
first patch

Attachment 151370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13160615

[Build Bot]

Comment on attachment 151370 [details]
first patch

Attachment 151370 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13176085

[Build Bot]

Comment on attachment 151370 [details]
first patch

Attachment 151370 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13166420

[Gyuyoung Kim]

Comment on attachment 151370 [details]
first patch

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

> Source/WTF/ChangeLog:19
> +        Reviewed by NOBODY (OOPS!).

Generally, "Reviewed by bla bla" is placed above description.

> Source/WebCore/GNUmakefile.am:490
> +# Filters

Is this feature related to this patch ?

> configure.ac:1120
> +# check whether to enable CSS Regions support

ditto.

> configure.ac:1467
> +AM_CONDITIONAL([ENABLE_CSS_FILTERS],[test "$enable_css_filters" = "yes"])

ditto.

> configure.ac:1529
> + CSS Filters support                                      : $enable_css_filters

ditto.

[arno.]

(In reply to comment #7)
> (From update of attachment 151370 [details])

Thanks for noticing that. I had indeed merged two patches.

[arno.]

Created attachment 151481 [details]
updated patch; lets see if he build properly on more platforms

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmak..." exit_code: 1
Source/WebCore/ChangeLog:21:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 33 files


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

[Build Bot]

Comment on attachment 151481 [details]
updated patch; lets see if he build properly on more platforms

Attachment 151481 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13203041

[Build Bot]

Comment on attachment 151481 [details]
updated patch; lets see if he build properly on more platforms

Attachment 151481 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13199046

[arno.]

Created attachment 151506 [details]
try another time

[Build Bot]

Comment on attachment 151506 [details]
try another time

Attachment 151506 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13182164

[arno.]

I don't known how to fix link errors on mac and windows. Do I need to edit WebCore.vcproj and project.pbxproj ? If so, is it possible to do that without a windows/mac box ?
Moreover, is the path I'm taking in my patch correct ?

[Build Bot]

Comment on attachment 151506 [details]
try another time

Attachment 151506 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13207069

[Build Bot]

Comment on attachment 151506 [details]
try another time

Attachment 151506 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13217007

[Gyuyoung Kim]

(In reply to comment #15)
> I don't known how to fix link errors on mac and windows. Do I need to edit WebCore.vcproj and project.pbxproj ?

Yes, you should modify .vcproj and .pbxproj files to support windows and mac ports.

> If so, is it possible to do that without a windows/mac box ?

When I submit a patch for WebCore, I test it on all platforms before submitting to Bugzilla.

[arno.]

(In reply to comment #18)
> (In reply to comment #15)

> > If so, is it possible to do that without a windows/mac box ?
> 
> When I submit a patch for WebCore, I test it on all platforms before submitting to Bugzilla.

Is there something such as a tryserver where developers could test patches on platforms where they don't have access/have difficult access without submitting to bugzilla ?

[Gyuyoung Kim]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> 
> > > If so, is it possible to do that without a windows/mac box ?
> > 
> > When I submit a patch for WebCore, I test it on all platforms before submitting to Bugzilla.
> 
> Is there something such as a tryserver where developers could test patches on platforms where they don't have access/have difficult access without submitting to bugzilla ?

As far as I know, there is no test server. You have to test your patch locally first.

[arno.]

Created attachment 151829 [details]
updated patch

This patch should hopefully build correctly (at least on mac). So it use a jsCast (in toTypedArray) to check if object is actually a typed array. It is the correct way to do? If so, is adding a TypedArray idl interface the correct way to be able to jsCast ?

[arno.]

Created attachment 151992 [details]
updated patch

oh, the patch did not apply. That may be because http://trac.webkit.org/changeset/122410 has been landed in between

[Gyuyoung Kim]

I'm not an expert for this patch. So, I think you need to find proper reviewer on #webkit irc.

[Kenneth Russell]

Comment on attachment 151992 [details]
updated patch

Thanks for the patch. It looks pretty good upon first glance. I'll look at it more thoroughly tomorrow but in the interim would appreciate it if other reviewers would hold off r+'ing it until then.

[Kenneth Russell]

Comment on attachment 151992 [details]
updated patch

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

Thanks for this patch and sorry for taking so long to review it. Originally I was concerned about the naming convention of your newly introduced TypedArray superclass, but upon giving it more thought I don't think it's necessary to expose that concept at all -- therefore the r-. Please contact me if you have any concerns about this review. Looking forward to a revised patch.

> Source/WTF/wtf/TypedArray.h:36
> +        TYPE_Int8,

While there isn't really a naming convention in WebKit for enums, using both capitalization and underscores doesn't match any other I've seen. Please use for example TypeInt8, TypeUint8, etc.

> Source/WTF/wtf/TypedArray.h:44
> +        TYPE_Float64

Please add one for DataView as well.

> Source/WTF/wtf/TypedArray.h:46
> +    virtual ViewType getType() = 0;

Just go ahead and add this virtual to ArrayBufferView. It already contains a bunch of other virtuals for run-time type identification. This will simplify your patch since you won't need to touch any IDL files or the code generators. You will need however a couple more tests in the custom bindings in order to watch for and avoid DataViews as incoming arguments.

Actually, it would be good to clean up these virtuals. You can feel free to remove the others in favor of your new one when you introduce it.

> Source/WTF/wtf/Uint16Array.h:49
> +    ViewType getType() 

All of these overrides should be in the private section to minimize visibility. See for example the current isUnsignedShortArray in this class.

[arno.]

Created attachment 154429 [details]
updated patch to address review

[WebKit Review Bot]

Comment on attachment 154429 [details]
updated patch to address review

Attachment 154429 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13346387

[Kenneth Russell]

Comment on attachment 154429 [details]
updated patch to address review

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

Looks good overall. Please address the build failure.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:415
> +        ArrayBufferView::ViewType type = arrayBufferview->getType();

Typo -- see the cr-linux build results.

[arno.]

Created attachment 154453 [details]
updated patch

[Kenneth Russell]

Comment on attachment 154453 [details]
updated patch

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

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:415
> +        ArrayBufferView::ViewType type = arrayBufferview->getType();

Still not fixed. This will fail the cr-linux EWS bot. arrayBufferview => arrayBufferView

[arno.]

Created attachment 154456 [details]
updated patch

sorry about that, I had only seen the second error

[WebKit Review Bot]

Comment on attachment 154456 [details]
updated patch

Attachment 154456 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341387

[Kenneth Russell]

Comment on attachment 154456 [details]
updated patch

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

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:415
> +        ArrayBufferView::ViewType type = arrayBufferView->getType();

Oops. You've surely already seen the error but "->" should be "." here.

[arno.]

Created attachment 154735 [details]
updated patch

I've figured out how to compile the chromium port. So this patch should be correct

[Kenneth Russell]

Comment on attachment 154735 [details]
updated patch

Very nice, thanks. r=me

[WebKit Review Bot]

Comment on attachment 154735 [details]
updated patch

Clearing flags on attachment: 154735

Committed r123819: <http://trac.webkit.org/changeset/123819>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Is performance tests added by this and http://trac.webkit.org/changeset/124483 really worth tracking the score of? Performance tests aren't regression tests. We don't want to add a new performance test for every bug we fix because that's going to increase the bot cycle time.

Also, we should probably use runPerSecond instead of run.

[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.