better V8 bindings for HTML5 ImageData

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuqiang Xian]

Details please see the Chromium bug - http://code.google.com/p/chromium/issues/detail?id=3473.

V8 has already avoided the CanvasPixelArray element access through the C++ bindings by introducing a PixelArray class in V8 space (http://code.google.com/p/v8/source/detail?r=2549, http://trac.webkit.org/changeset/46495). 

Actually we can go further. Currently V8 still needs to wrap the CanvasPixelArray object through the C++ bindings, and it's still a big performance issue in some cases like the below example -

frame = context.getImageData(0, 0, canvas.width, canvas.height);
…	
for (var i = 0; i < frame.data.length; i += 4) {
	var r = frame.data[i + 0];
	var g = frame.data[i + 1];
	var b = frame.data[i + 2];
	
	var v = r + g + b;
	v /= 3;
	
	frame.data[i + 0] = v;
	frame.data[i + 1] = v;
	frame.data[i + 2] = v;
}	
…	
context.putImageData(frame, 0, 0);

There’ll be frequent invocations from Jitted code to native C++ code and big overhead for object bindings for “frame.data” access. Every “frame.data” access in the loop runs into the object binding code, which then lookups the hash map maintaining the relationship between DOM objects and the corresponding V8 objects, and returns the V8 object for “data” (a CanvasPixelArray object wrapper), and thus the object bindings overhead comes out. 

We can create a normal V8 object which has a PixelArray as the backing storage, and set the "data" property of ImageData object to it. This way we don't need to call through the C++ bindings for ImageData "data" access. This eliminates big overhead in switching between JavaScript and native contexts and performing object bindings when there're frequent "data" property accesses of ImageData.

This is also similar to the approach how JSC handles ImageData.

Back to the example case again, we see >3X performance with the proposed change, when handling a 800x600 sized canvas.

[Yuqiang Xian]

Created attachment 74714 [details]
proposed patch

[Vitaly Repeshko]

(In reply to comment #1)
> Created an attachment (id=74714) [details]
> proposed patch

The key part here is avoiding hitting a C++ callback when reading the "data" property on a JS wrapper of ImageData object. Setting a pure JS property on the wrapper is a good idea. Yet modifying the way the value of the property is computed is not necessary and I think can be only harmful. Instead of creating a new object template and instantiating it, we should reuse the CanvasPixelArray wraper, i.e. do something like this:
if (!wrapper.IsEmpty()) {
    v8::Handle<v8::Value> pixelArray = toV8(impl->data());
    if (!pixelArray.IsEmpty())
        wrapper->Set(v8::String::NewSymbol("data"), pixelArray, v8::ReadOnly);
}

[Yuqiang Xian]

Created attachment 74821 [details]
updated patch

Vitaly, thanks for the comments.

I agree with you. And here's the updated patch. Please help review it.
BTW, I also set the "length" property of CanvasPixelArray to be a pure V8 object.

[WebKit Review Bot]

Attachment 74821 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/WebCore.gypi', u'WebCore/bindings/scripts/CodeGeneratorV8.pm', u'WebCore/bindings/v8/custom/V8CanvasPixelArrayCustom.cpp', u'WebCore/bindings/v8/custom/V8ImageDataCustom.cpp', u'WebCore/html/ImageData.idl', u'WebCore/html/canvas/CanvasPixelArray.idl']" exit_code: 1
WebCore/bindings/v8/custom/V8ImageDataCustom.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8ImageDataCustom.cpp:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuqiang Xian]

Created attachment 74823 [details]
updated patch

resolve the style issue.

[Vitaly Repeshko]

Looks good. A few nits:
o After "Reviewed by" line in the change log add a short one-line summary: "[V8] Speed up data property access for ImageData."
o Update the rest of the change log description to emphasize the fact that pure JS properties are used.
o Comments in code should be formatted like sentences, starting with a capital letter.
o Typo: "elimnating" -> "eliminating".

After these are done you'll need a WebKit reviewer to set r+ (I can'd to this yet). Pavel (cc'ed) agreed to help.

[Pavel Feldman]

Comment on attachment 74823 [details]
updated patch

Rubber stamping so that you could land it once Vitaly's comments are addressed.

[Yuqiang Xian]

Created attachment 74894 [details]
updated patch

Thanks for the comments, Vitaly.

Here's the updated patch addressing your comments.

Thanks, -Yuqiang

[Pavel Feldman]

Comment on attachment 74894 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74894&action=review

> WebCore/ChangeLog:13
> +        https://bugs.webkit.org/show_bug.cgi?id=49999

Nit: bug reference should follow the commit title, before the blank line.

[Title]
[bug link]
<blank line>
[Discription]

[Eric Seidel (no email)]

Comment on attachment 74823 [details]
updated patch

Cleared Pavel Feldman's review+ from obsolete attachment 74823 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Yuqiang Xian]

Created attachment 75103 [details]
updated patch

Update the patch addressing Pavel's comment.

Thanks, -Yuqiang

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75103 [details]:

http/tests/media/video-load-twice.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 75103 [details]
updated patch

Clearing flags on attachment: 75103

Committed r73119: <http://trac.webkit.org/changeset/73119>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.