Bug 74455 - Uint8ClampedArray support
Summary: Uint8ClampedArray support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords: InRadar
Depends on:
Blocks: 73011
  Show dependency treegraph
 
Reported: 2011-12-13 15:28 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-01-17 19:28 PST (History)
15 users (show)

See Also:


Attachments
WIP (49.37 KB, patch)
2011-12-13 15:28 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (55.64 KB, patch)
2012-01-02 17:17 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (309.10 KB, patch)
2012-01-02 18:39 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (330.73 KB, patch)
2012-01-06 12:48 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (333.90 KB, patch)
2012-01-06 15:25 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (348.32 KB, patch)
2012-01-15 09:26 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (348.66 KB, patch)
2012-01-16 07:01 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (348.29 KB, patch)
2012-01-16 09:55 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (348.37 KB, patch)
2012-01-17 13:48 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2011-12-13 15:28:48 PST
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.
Comment 1 Caio Marcelo de Oliveira Filho 2011-12-13 15:33:27 PST
This is a work in progress. I would like some feedback whether is going on the right direction.
Comment 2 Kenneth Russell 2011-12-13 15:50:01 PST
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 3 Filip Pizlo 2012-01-01 16:40:47 PST
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()).
Comment 4 Oliver Hunt 2012-01-01 18:26:37 PST
Uint8ClampedArray is already supported, just with the name ByteArray, we really shouldn't have two distinct implementations of the same semantics.
Comment 5 Caio Marcelo de Oliveira Filho 2012-01-02 13:48:39 PST
(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).
Comment 6 Caio Marcelo de Oliveira Filho 2012-01-02 17:17:34 PST
Created attachment 120901 [details]
Patch
Comment 7 Filip Pizlo 2012-01-02 17:20:55 PST
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 8 Caio Marcelo de Oliveira Filho 2012-01-02 17:21:25 PST
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.
Comment 9 Filip Pizlo 2012-01-02 17:22:14 PST
One more thing: can you add a more exhausting, DFG-targetted test, in the style of fast/js/dfg-int8array?
Comment 10 Filip Pizlo 2012-01-02 17:25:05 PST
> >> 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.
Comment 11 Caio Marcelo de Oliveira Filho 2012-01-02 17:30:19 PST
(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.
Comment 12 Filip Pizlo 2012-01-02 17:34:18 PST
(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.
Comment 13 Caio Marcelo de Oliveira Filho 2012-01-02 18:39:56 PST
Created attachment 120906 [details]
Patch
Comment 14 Caio Marcelo de Oliveira Filho 2012-01-02 18:44:07 PST
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 15 Early Warning System Bot 2012-01-02 19:02:48 PST
Comment on attachment 120906 [details]
Patch

Attachment 120906 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11073136
Comment 16 Gyuyoung Kim 2012-01-02 19:09:46 PST
Comment on attachment 120906 [details]
Patch

Attachment 120906 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11039786
Comment 17 Collabora GTK+ EWS bot 2012-01-02 19:12:37 PST
Comment on attachment 120906 [details]
Patch

Attachment 120906 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11039788
Comment 18 WebKit Review Bot 2012-01-02 19:47:46 PST
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 19 WebKit Review Bot 2012-01-02 20:51:59 PST
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
Comment 20 Filip Pizlo 2012-01-03 12:09:42 PST
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.
Comment 21 Caio Marcelo de Oliveira Filho 2012-01-06 12:48:29 PST
Created attachment 121474 [details]
Patch
Comment 22 Kenneth Russell 2012-01-06 14:13:52 PST
Comment on attachment 121474 [details]
Patch

Please rebaseline the patch so the EWS bots can test it.
Comment 23 Caio Marcelo de Oliveira Filho 2012-01-06 15:25:11 PST
Created attachment 121509 [details]
Patch
Comment 24 Early Warning System Bot 2012-01-06 16:08:59 PST
Comment on attachment 121509 [details]
Patch

Attachment 121509 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11167121
Comment 25 Caio Marcelo de Oliveira Filho 2012-01-11 10:13:35 PST
(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 26 Oliver Hunt 2012-01-11 10:58:01 PST
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 27 Oliver Hunt 2012-01-11 11:02:29 PST
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.
Comment 28 Kenneth Russell 2012-01-12 11:13:30 PST
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.
Comment 29 Kenneth Russell 2012-01-12 11:23:41 PST
(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.
Comment 30 Caio Marcelo de Oliveira Filho 2012-01-13 05:51:36 PST
(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.
Comment 31 Caio Marcelo de Oliveira Filho 2012-01-13 16:43:28 PST
(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?
Comment 32 Caio Marcelo de Oliveira Filho 2012-01-15 09:26:36 PST
Created attachment 122567 [details]
Patch
Comment 33 Early Warning System Bot 2012-01-15 09:50:21 PST
Comment on attachment 122567 [details]
Patch

Attachment 122567 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11129224
Comment 34 Caio Marcelo de Oliveira Filho 2012-01-16 07:01:35 PST
Created attachment 122629 [details]
Patch
Comment 35 Early Warning System Bot 2012-01-16 07:25:31 PST
Comment on attachment 122629 [details]
Patch

Attachment 122629 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11255184
Comment 36 Caio Marcelo de Oliveira Filho 2012-01-16 09:19:03 PST
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 37 Csaba Osztrogonác 2012-01-16 09:20:20 PST
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.
Comment 38 Caio Marcelo de Oliveira Filho 2012-01-16 09:55:45 PST
Created attachment 122658 [details]
Patch
Comment 39 Filip Pizlo 2012-01-17 13:33:42 PST
Comment on attachment 122658 [details]
Patch

R=me!  Nice work!
Comment 40 Caio Marcelo de Oliveira Filho 2012-01-17 13:48:36 PST
Created attachment 122808 [details]
Patch for landing
Comment 41 WebKit Review Bot 2012-01-17 17:11:37 PST
Comment on attachment 122808 [details]
Patch for landing

Clearing flags on attachment: 122808

Committed r105217: <http://trac.webkit.org/changeset/105217>
Comment 42 WebKit Review Bot 2012-01-17 17:11:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 43 Filip Pizlo 2012-01-17 19:28:26 PST
<rdar://problem/10712796>