Bug 90838 - constructing TypedArray from another TypedArray is slow
Summary: constructing TypedArray from another TypedArray is slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: arno.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-09 17:18 PDT by arno.
Modified: 2014-04-24 16:45 PDT (History)
16 users (show)

See Also:


Attachments
testcase (687 bytes, text/html)
2012-07-09 17:22 PDT, arno.
no flags Details
first patch (34.46 KB, patch)
2012-07-09 17:47 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch; lets see if he build properly on more platforms (33.92 KB, patch)
2012-07-10 10:18 PDT, arno.
no flags Details | Formatted Diff | Diff
try another time (36.65 KB, patch)
2012-07-10 13:13 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (44.26 KB, patch)
2012-07-11 18:13 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (44.53 KB, patch)
2012-07-12 10:38 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch to address review (28.02 KB, patch)
2012-07-25 13:40 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (28.02 KB, patch)
2012-07-25 14:59 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (28.02 KB, patch)
2012-07-25 15:08 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (28.01 KB, patch)
2012-07-26 13:32 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-09 17:18:26 PDT
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);
Comment 1 arno. 2012-07-09 17:22:02 PDT
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
Comment 2 arno. 2012-07-09 17:47:56 PDT
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.
Comment 3 WebKit Review Bot 2012-07-09 17:50:32 PDT
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.
Comment 4 WebKit Review Bot 2012-07-09 18:00:47 PDT
Comment on attachment 151370 [details]
first patch

Attachment 151370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13160615
Comment 5 Build Bot 2012-07-09 18:02:37 PDT
Comment on attachment 151370 [details]
first patch

Attachment 151370 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13176085
Comment 6 Build Bot 2012-07-09 18:37:11 PDT
Comment on attachment 151370 [details]
first patch

Attachment 151370 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13166420
Comment 7 Gyuyoung Kim 2012-07-09 23:04:24 PDT
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.
Comment 8 arno. 2012-07-10 09:53:24 PDT
(In reply to comment #7)
> (From update of attachment 151370 [details])

Thanks for noticing that. I had indeed merged two patches.
Comment 9 arno. 2012-07-10 10:18:50 PDT
Created attachment 151481 [details]
updated patch; lets see if he build properly on more platforms
Comment 10 WebKit Review Bot 2012-07-10 10:21:32 PDT
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.
Comment 11 Build Bot 2012-07-10 11:27:37 PDT
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
Comment 12 Build Bot 2012-07-10 11:48:39 PDT
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
Comment 13 arno. 2012-07-10 13:13:20 PDT
Created attachment 151506 [details]
try another time
Comment 14 Build Bot 2012-07-10 13:45:44 PDT
Comment on attachment 151506 [details]
try another time

Attachment 151506 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13182164
Comment 15 arno. 2012-07-10 14:25:58 PDT
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 ?
Comment 16 Build Bot 2012-07-10 15:31:13 PDT
Comment on attachment 151506 [details]
try another time

Attachment 151506 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13207069
Comment 17 Build Bot 2012-07-10 15:41:08 PDT
Comment on attachment 151506 [details]
try another time

Attachment 151506 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13217007
Comment 18 Gyuyoung Kim 2012-07-10 17:40:11 PDT
(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.
Comment 19 arno. 2012-07-10 18:00:31 PDT
(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 ?
Comment 20 Gyuyoung Kim 2012-07-10 19:11:10 PDT
(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.
Comment 21 arno. 2012-07-11 18:13:37 PDT
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 ?
Comment 22 arno. 2012-07-12 10:38:07 PDT
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
Comment 23 Gyuyoung Kim 2012-07-18 22:29:53 PDT
I'm not an expert for this patch. So, I think you need to find proper reviewer on #webkit irc.
Comment 24 Kenneth Russell 2012-07-18 23:48:07 PDT
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.
Comment 25 Kenneth Russell 2012-07-23 17:44:43 PDT
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.
Comment 26 arno. 2012-07-25 13:40:26 PDT
Created attachment 154429 [details]
updated patch to address review
Comment 27 WebKit Review Bot 2012-07-25 13:49:48 PDT
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
Comment 28 Kenneth Russell 2012-07-25 14:44:29 PDT
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.
Comment 29 arno. 2012-07-25 14:59:18 PDT
Created attachment 154453 [details]
updated patch
Comment 30 Kenneth Russell 2012-07-25 15:04:51 PDT
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
Comment 31 arno. 2012-07-25 15:08:55 PDT
Created attachment 154456 [details]
updated patch

sorry about that, I had only seen the second error
Comment 32 WebKit Review Bot 2012-07-25 15:26:16 PDT
Comment on attachment 154456 [details]
updated patch

Attachment 154456 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341387
Comment 33 Kenneth Russell 2012-07-25 15:50:43 PDT
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.
Comment 34 arno. 2012-07-26 13:32:43 PDT
Created attachment 154735 [details]
updated patch

I've figured out how to compile the chromium port. So this patch should be correct
Comment 35 Kenneth Russell 2012-07-26 16:19:18 PDT
Comment on attachment 154735 [details]
updated patch

Very nice, thanks. r=me
Comment 36 WebKit Review Bot 2012-07-26 17:25:26 PDT
Comment on attachment 154735 [details]
updated patch

Clearing flags on attachment: 154735

Committed r123819: <http://trac.webkit.org/changeset/123819>
Comment 37 WebKit Review Bot 2012-07-26 17:25:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 Ryosuke Niwa 2012-09-25 19:21:46 PDT
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.
Comment 39 Darin Adler 2014-04-24 16:45:46 PDT
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.