534467 - optimize CanvasPixelArray

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: implement Uint8ClampedArray

Once we have typed arrays in bug 532774, it's pretty straightforward to extend it to an array type that implements the clamping semantics demanded by CanvasPixelArray.  We can then emit equivalent custom trace code as well.

Here's Uint8ClampedArray.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v1

This works, and implements get/put/crateImageData.  2x speedup on something like http://www.galbraiths.org/benchmarks/pixastic.html, likely more on things that do repeated get/puts.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Just applying the patch for bug 532774 (and bug 533659 and the custom quickstubs thing) doesn't make this patch apply to t-m....  Was it developed against m-c instead?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: part 1, js side typed array changes

Split up the v1 patch into three pieces -- this is the first one, that adds the core Uint8Clamped type that PixelArray needs.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: part 2, js side tracing additions

part 2, tracing support for Uint8Clamped

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: part 3, content side changes

part 3, content side changes -- canvas code fixes and the custom quickstub code.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 423608 [details] [diff] [review]
part 3, content side changes

>+nsIDOMCanvasRenderingContext2D_CreateImageData(JSContext *cx, uintN argc, jsval *vp)
>+    if (!JS_ValueToECMAUint32(cx, argv[0], &w) ||
>+        !JS_ValueToECMAUint32(cx, argv[1], &h))

This doesn't look like it'll do the right thing for negative inputs.

>+    JSObject *result = JS_NewObject(cx, NULL, NULL, NULL);

The comment on why this needs to be created after the array got lost.  Put it back?

>+    JSAutoTempValueRooter rr(cx, result);
>+    if (!result)
>+        return JS_FALSE;

Should this and the array case explicitly throw NS_ERROR_OUT_OF_MEMORY?

>+nsIDOMCanvasRenderingContext2D_GetImageData(JSContext *cx, uintN argc, jsval *vp)
>+        !JS_ValueToECMAUint32(cx, argv[2], &w) ||
>+        !JS_ValueToECMAUint32(cx, argv[3], &h))

See above.

Is there a reason the

>-    if (len > (((PRUint32)0xfff00000)/sizeof(jsval)))

check is done in the _explicit method instead of here?  Does it need to be done at all now?

>+    // then build the object

Again, this lost the comment about _why_ the object allocation needs to come after the array allocation.

>+nsIDOMCanvasRenderingContext2D_PutImageData(JSContext *cx, uintN argc, jsval *vp)
>+    if (!JSVAL_IS_OBJECT(argv[0]) || JSVAL_IS_NULL(argv[0]))

  if (JSVAL_IS_PRIMITIVE(argv[0]))

>+        !JS_ValueToECMAUint32(cx, v, &w))

Again, negative number issues....

>+        !JS_ValueToECMAUint32(cx, v, &h))

And here.

And v needs to be rooted if we're doing a JS_GetProperty into it.

>+    if (!JS_GetProperty(cx, dataObject, "data", &v) ||
>+        !JSVAL_IS_OBJECT(v))
>+        return JS_FALSE;
>+    darray = JSVAL_TO_OBJECT(v);

darray might be null now.  You probably wanted JSVAL_IS_PRIMITIVE, not !JSVAL_IS_OBJECT.

>+    *vp = JSVAL_VOID;

Is this needed?  Please check with Jason.

>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+nsCanvasRenderingContext2D::GetImageData_explicit(PRInt32 x, PRInt32 y, PRUint32 w, PRUint32 h,
>+        src = aData + (destStride * j);

Shouldn't src already have the right value here (just like dest) from the end of the previous iteration?


>+nsCanvasRenderingContext2D::PutImageData_explicit(PRInt32 x, PRInt32 y, PRUint32 w, PRUint32 h,
>+        src = aData + (w * 4 * j);

Again, src should already have the right value from the previous loop iteration.

>+        dst = imgsurf->Data() + imgsurf->Stride() * j;

Can't tell you whether this would, though.

>+            /* Do the awful premultiply */

Leave the SSE/MMX comment that used to be here?

>+++ b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
>+  [noscript] void getImageData_explicit(in long x, in long y, in unsigned long width, in unsigned long height,
>+                                        [array, size_is(dataLen)] in octet dataPtr, in unsigned long dataLen);
>+  [noscript] void putImageData_explicit(in long x, in long y, in unsigned long width, in unsigned long height,
>+                                        [array, size_is(dataLen)] in octet dataPtr, in unsigned long dataLen);

Can we get some docs about how dataLen should be related to width+height, at least?

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> >+    *vp = JSVAL_VOID;
> 
> Is this needed?  Please check with Jason.

Yes.  |*vp| is the callee on entry, and fast natives must set a return value in non-error cases.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 423606 [details] [diff] [review]
part 1, js side typed array changes

>+    inline uint8 operator= (const uint8 x) {

It strikes me as a little funny that these arguments are const and that the return type here is uint8 rather than uint8_clamped&.

But I can't imagine either one makes any difference at all to the compiler.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Part 1 and part 2 checked in; fixed issues in comment #8 on checkin.  (Compiler didn't seem to care for sure, but it looked neater)

http://hg.mozilla.org/tracemonkey/rev/763e4245ae9f
http://hg.mozilla.org/tracemonkey/rev/f7017e7dfb20

Working on updated patch for part 3 now.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: part 3, content side changes, v2

Updated for review comments.  Boris, note that this has to be applied on top of bug 534735 but is pretty much independent of it -- it's just that they both need/want to create Canvas2D_QS.h, which doesn't exist yet.  I'll fix the checkin order for whichever gets r+ first.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 425354 [details] [diff] [review]
part 3, content side changes, v2

> Again, this lost the comment about _why_ the object allocation needs to come
> after the array allocation.

This comment wasn't addressed.

>  if (JSVAL_IS_PRIMITIVE(argv[0]))

Nor this one.

>+    uint32 wi, hi;

Those should be int32.  

r=bzbarsky with those three issues fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

vlad, were you waiting on something else from me here before landing?

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nope, vacation interfered.. waiting on final try server builds now.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Finally landed and stuck -- http://hg.mozilla.org/mozilla-central/rev/c1cc02524fbb