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);
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
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.
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 on attachment 151370 [details] first patch Attachment 151370 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13160615
Comment on attachment 151370 [details] first patch Attachment 151370 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13176085
Comment on attachment 151370 [details] first patch Attachment 151370 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13166420
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.
(In reply to comment #7) > (From update of attachment 151370 [details]) Thanks for noticing that. I had indeed merged two patches.
Created attachment 151481 [details] updated patch; lets see if he build properly on more platforms
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 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 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
Created attachment 151506 [details] try another time
Comment on attachment 151506 [details] try another time Attachment 151506 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13182164
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 on attachment 151506 [details] try another time Attachment 151506 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13207069
Comment on attachment 151506 [details] try another time Attachment 151506 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13217007
(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.
(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 ?
(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.
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 ?
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
I'm not an expert for this patch. So, I think you need to find proper reviewer on #webkit irc.
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 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.
Created attachment 154429 [details] updated patch to address review
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 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.
Created attachment 154453 [details] updated patch
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
Created attachment 154456 [details] updated patch sorry about that, I had only seen the second error
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 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.
Created attachment 154735 [details] updated patch I've figured out how to compile the chromium port. So this patch should be correct
Comment on attachment 154735 [details] updated patch Very nice, thanks. r=me
Comment on attachment 154735 [details] updated patch Clearing flags on attachment: 154735 Committed r123819: <http://trac.webkit.org/changeset/123819>
All reviewed patches have been landed. Closing bug.
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.
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.