Bug 74455 – Uint8ClampedArray support

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

This is a work in progress. I would like some feedback whether is going on the right direction.

[Kenneth Russell]

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?

[Filip Pizlo]

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()).

[Oliver Hunt]

Uint8ClampedArray is already supported, just with the name ByteArray, we really shouldn't have two distinct implementations of the same semantics.

[Caio Marcelo de Oliveira Filho]

(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).

[Caio Marcelo de Oliveira Filho]

Created attachment 120901 [details]
Patch

[Filip Pizlo]

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?

[Caio Marcelo de Oliveira Filho]

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.

[Filip Pizlo]

One more thing: can you add a more exhausting, DFG-targetted test, in the style of fast/js/dfg-int8array?

[Filip Pizlo]

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

[Caio Marcelo de Oliveira Filho]

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

[Filip Pizlo]

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

[Caio Marcelo de Oliveira Filho]

Created attachment 120906 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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.

[Early Warning System Bot]

Comment on attachment 120906 [details]
Patch

Attachment 120906 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11073136

[Gyuyoung Kim]

Comment on attachment 120906 [details]
Patch

Attachment 120906 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11039786

[Collabora GTK+ EWS bot]

Comment on attachment 120906 [details]
Patch

Attachment 120906 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11039788

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Filip Pizlo]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 121474 [details]
Patch

[Kenneth Russell]

Comment on attachment 121474 [details]
Patch

Please rebaseline the patch so the EWS bots can test it.

[Caio Marcelo de Oliveira Filho]

Created attachment 121509 [details]
Patch

[Early Warning System Bot]

Comment on attachment 121509 [details]
Patch

Attachment 121509 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11167121

[Caio Marcelo de Oliveira Filho]

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

[Oliver Hunt]

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.

[Oliver Hunt]

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.

[Kenneth Russell]

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 Russell]

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

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

(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?

[Caio Marcelo de Oliveira Filho]

Created attachment 122567 [details]
Patch

[Early Warning System Bot]

Comment on attachment 122567 [details]
Patch

Attachment 122567 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11129224

[Caio Marcelo de Oliveira Filho]

Created attachment 122629 [details]
Patch

[Early Warning System Bot]

Comment on attachment 122629 [details]
Patch

Attachment 122629 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11255184

[Caio Marcelo de Oliveira Filho]

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.

[Csaba Osztrogonác]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 122658 [details]
Patch

[Filip Pizlo]

Comment on attachment 122658 [details]
Patch

R=me!  Nice work!

[Caio Marcelo de Oliveira Filho]

Created attachment 122808 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122808 [details]
Patch for landing

Clearing flags on attachment: 122808

Committed r105217: <http://trac.webkit.org/changeset/105217>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

<rdar://problem/10712796>