Created attachment 119096 [details] WIP Add Uint8ClampedArray support as defined by http://www.khronos.org/registry/typedarray/specs/latest/#7.1 This is useful for bug 73011.
This is a work in progress. I would like some feedback whether is going on the right direction.
Comment on attachment 119096 [details] WIP It generally looks fine to me. I wonder whether it would be worth splitting off the JSC compiler work into a separate patch and just start by adding the core classes and IDL files, without even compiling them. Then perhaps the JSC and V8 bindings could be done independently. Have you tried submitting this patch to the EWS?
Comment on attachment 119096 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=119096&action=review You're doing it right! :-) I think the only thing that is wrong is the PredictObjectMask. As for the other two issues, it's more of a problem with the code base as-is - so it's up to you whether you want to (a) be consistent or (b) try to clean it up. Probably for the array length code it would be better if you were consistent (i.e. you're doing it right), but for the shouldSpeculateXYZ() method that you've added, I'd sort of prefer that you implement that method the Right Way (call the helper from PredictedType.h rather than doing your own equality comparison). > Source/JavaScriptCore/bytecode/PredictedType.h:54 > +static const PredictedType PredictObjectMask = 0x00002fff; // Bitmask used for testing for any kind of object prediction. I presume you meant 0x00003fff? > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:737 > + case GetUint8ClampedArrayLength: Ugggghhh! This is not your fault, but we should probably unify these GetXYZArrayLength opcodes. I wouldn't worry about it for this patch though unless you were feeling adventurous. > Source/JavaScriptCore/dfg/DFGNode.h:949 > + return prediction() == PredictUint8ClampedArray; I know you're just being consistent but this should really be isUint8ClampedArrayPrediction(prediction()).
Uint8ClampedArray is already supported, just with the name ByteArray, we really shouldn't have two distinct implementations of the same semantics.
(In reply to comment #4) > Uint8ClampedArray is already supported, just with the name ByteArray, we really shouldn't have two distinct implementations of the same semantics. One difference is that Uint8ClampedArray would support using alternative views to manipulate the same data. In the scenario of bug 73011, one can get from the API a Uint8ClampedArray, create a view on this data using Uint32Array and do 32 bit manipulations. This example is expanded in the article http://hacks.mozilla.org/2011/12/faster-canvas-pixel-manipulation-with-typed-arrays/ (you can skip directly to "Image Data Object" section).
Created attachment 120901 [details] Patch
Comment on attachment 120901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120901&action=review Looks good so far, but I think I found a bug. :-) > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:736 > + break; This is good, but aren't you missing cases in GetByVal and PutByVal?
Comment on attachment 119096 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=119096&action=review Thanks for the review. >> Source/JavaScriptCore/bytecode/PredictedType.h:54 >> +static const PredictedType PredictObjectMask = 0x00002fff; // Bitmask used for testing for any kind of object prediction. > > I presume you meant 0x00003fff? Fixed. >> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:737 >> + case GetUint8ClampedArrayLength: > > Ugggghhh! This is not your fault, but we should probably unify these GetXYZArrayLength opcodes. I wouldn't worry about it for this patch though unless you were feeling adventurous. By unify you mean creating a table from GetXYZArrayLength to PredictXYZArray and share the same block of code for all GetXYZArrayLength cases? Or a more deeper change? >> Source/JavaScriptCore/dfg/DFGNode.h:949 >> + return prediction() == PredictUint8ClampedArray; > > I know you're just being consistent but this should really be isUint8ClampedArrayPrediction(prediction()). Fixed.
One more thing: can you add a more exhausting, DFG-targetted test, in the style of fast/js/dfg-int8array?
> >> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:737 > >> + case GetUint8ClampedArrayLength: > > > > Ugggghhh! This is not your fault, but we should probably unify these GetXYZArrayLength opcodes. I wouldn't worry about it for this patch though unless you were feeling adventurous. > > By unify you mean creating a table from GetXYZArrayLength to PredictXYZArray and share the same block of code for all GetXYZArrayLength cases? Or a more deeper change? All of the typed arrays behave the same way for length except for the vtable being tested, and maybe the offset of the length. But even the length offset is probably the same… So I was thinking we should just have a GetTypedArrayLength that takes advantage of this. The bottom line is to not have so many opcodes for getting array length. Even without any other changes, we could have one GetArrayLength opcode that cases on the prediction to decide what to do. But you don't have to do this change; in fact probably putting it into this patch might be a bad idea as it would make the patch really big. It's just something to think about.
(In reply to comment #2) > (From update of attachment 119096 [details]) > It generally looks fine to me. I wonder whether it would be worth splitting off the JSC compiler work into a separate patch and just start by adding the core classes and IDL files, without even compiling them. Then perhaps the JSC and V8 bindings could be done independently. Have you tried submitting this patch to the EWS? Thanks for looking, Kenneth. If it's OK, I prefer trying a concrete implementation, so JSC will come first, then we can work on the V8 one in a separate patch.
(In reply to comment #11) > (In reply to comment #2) > > (From update of attachment 119096 [details] [details]) > > It generally looks fine to me. I wonder whether it would be worth splitting off the JSC compiler work into a separate patch and just start by adding the core classes and IDL files, without even compiling them. Then perhaps the JSC and V8 bindings could be done independently. Have you tried submitting this patch to the EWS? > > Thanks for looking, Kenneth. If it's OK, I prefer trying a concrete implementation, so JSC will come first, then we can work on the V8 one in a separate patch. I think having a concrete implementation in the first patrch is the better approach, so I agree with what you're doing.
Created attachment 120906 [details] Patch
This patch includes suggestions from Pizlo. In I'm also testing the ifdef for V8, so results from Chromium and Qt EWS are relevant. I still need to add to other build systems.
Comment on attachment 120906 [details] Patch Attachment 120906 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11073136
Comment on attachment 120906 [details] Patch Attachment 120906 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11039786
Comment on attachment 120906 [details] Patch Attachment 120906 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11039788
Comment on attachment 120906 [details] Patch Attachment 120906 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11043188 New failing tests: fast/canvas/webgl/array-unit-tests.html fast/js/dfg-uint8clampedarray.html
Comment on attachment 120906 [details] Patch Attachment 120906 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11003166 New failing tests: fast/canvas/webgl/array-unit-tests.html fast/js/dfg-uint8clampedarray.html http/tests/inspector/resource-tree/resource-tree-document-url.html
Looks like you still have some build magic to do! Also, it would be good to add Chromium-specific expectations files for the failing tests. It's OK for tests that are known to fail on a particular platform, and to have expectations that reflect this.
Created attachment 121474 [details] Patch
Comment on attachment 121474 [details] Patch Please rebaseline the patch so the EWS bots can test it.
Created attachment 121509 [details] Patch
Comment on attachment 121509 [details] Patch Attachment 121509 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11167121
(In reply to comment #23) > Created an attachment (id=121509) [details] > Patch This version includes some changes in DFGSpeculativeJIT to avoid duplication with ByteArray (and to make the code a bit more readable). I'll update this patch next days to include Mac build system.
Comment on attachment 121509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121509&action=review Assentially r=me, though you have a few style issues/copyright notices to fix up. I'd also file a follow on bug to remove JSByteArray, and make ImageData use Uint8ClampedArray > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1612 > + int clampedValue(clampDoubleToByte(jsValue.asNumber())); We tend not to use initializer syntax for ints, so clampedValue = clampDoubleTByte... would be better > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1770 > - m_jit.move(Imm32((int)d), scratchReg); > + m_jit.move(Imm32(int(d)), scratchReg); Unnecessary change. Although technically this should probably be static_cast<int>(d) to conform to our coding style anyway. > Source/JavaScriptCore/wtf/Uint8ClampedArray.h:4 > + * Add your copyright here -- you've added code, this isn't simply a copy.
Comment on attachment 121509 [details] Patch You have a few style issues to cleanup (and a missing copyright notice), It would be good to file a follow on bug to remove JSByteArray (and all related ByteArray shenanigans), and make ImageData use the Uint8ClampedArray. But when you do that you'll need to also fix the interpreter and baseline hit to still have decent image data perf.
How soon can these changes land? If it's going to be more than a few days, then I'd like to see the core files land, even if they aren't compiled in yet, so that work on other ports can proceed in parallel.
(In reply to comment #27) > (From update of attachment 121509 [details]) > You have a few style issues to cleanup (and a missing copyright notice), It would be good to file a follow on bug to remove JSByteArray (and all related ByteArray shenanigans), and make ImageData use the Uint8ClampedArray. But when you do that you'll need to also fix the interpreter and baseline hit to still have decent image data perf. This bug already depends on https://bugs.webkit.org/show_bug.cgi?id=73011 , which tracks the remaining work to hook up Uint8ClampedArray to Canvas' ImageData.
(In reply to comment #28) > How soon can these changes land? If it's going to be more than a few days, then I'd like to see the core files land, even if they aren't compiled in yet, so that work on other ports can proceed in parallel. Kenneth, I plan to upload a new version of the patch later today.
(In reply to comment #30) > (In reply to comment #28) > > How soon can these changes land? If it's going to be more than a few days, then I'd like to see the core files land, even if they aren't compiled in yet, so that work on other ports can proceed in parallel. > > Kenneth, I plan to upload a new version of the patch later today. I'm sorry. I couldn't add every file to Xcode today, still having problems to add the JSUint8ClampedArray.{cpp,h}. :-( Adding Sam Weinig to the CC since Oliver thinks he might know how to help. Sam, I created a workspace with the WebCore and JavaScriptCore, and went in WebCore configuration to set Build Products path to the /path/to/webkit/WebKitBuild path. And still, the Derived Sources appear in the navigator as red (not found). Am I missing something? What is the appropriate way to add DerivedSources in Xcode 4?
Created attachment 122567 [details] Patch
Comment on attachment 122567 [details] Patch Attachment 122567 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11129224
Created attachment 122629 [details] Patch
Comment on attachment 122629 [details] Patch Attachment 122629 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11255184
Sorry for the noise. We are almost there, the only missing build system is Qt (!), which has a problem with incremental compilation that Ossy/torarne were looking into and already have a fix to land soon.
Comment on attachment 122629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122629&action=review > Source/WebCore/DerivedSources.pri:242 > + html/canvas/Uint8ClampedArray.idl \ Qt build system has a bug which caused incremental build failure on your patch. I landed the fix for it: http://trac.webkit.org/changeset/105068 Please add "$$PWD/" prefix to this line to avoid conflict during applying this patch.
Created attachment 122658 [details] Patch
Comment on attachment 122658 [details] Patch R=me! Nice work!
Created attachment 122808 [details] Patch for landing
Comment on attachment 122808 [details] Patch for landing Clearing flags on attachment: 122808 Committed r105217: <http://trac.webkit.org/changeset/105217>
All reviewed patches have been landed. Closing bug.
<rdar://problem/10712796>