Bug 49999 - better V8 bindings for HTML5 ImageData
Summary: better V8 bindings for HTML5 ImageData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-23 18:17 PST by Yuqiang Xian
Modified: 2010-12-02 02:40 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (5.69 KB, patch)
2010-11-23 18:26 PST, Yuqiang Xian
no flags Details | Formatted Diff | Diff
updated patch (6.72 KB, patch)
2010-11-24 17:58 PST, Yuqiang Xian
no flags Details | Formatted Diff | Diff
updated patch (6.72 KB, patch)
2010-11-24 18:07 PST, Yuqiang Xian
no flags Details | Formatted Diff | Diff
updated patch (6.75 KB, patch)
2010-11-25 13:28 PST, Yuqiang Xian
pfeldman: review+
Details | Formatted Diff | Diff
updated patch (6.75 KB, patch)
2010-11-29 21:55 PST, Yuqiang Xian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuqiang Xian 2010-11-23 18:17:04 PST
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.
Comment 1 Yuqiang Xian 2010-11-23 18:26:50 PST
Created attachment 74714 [details]
proposed patch
Comment 2 Vitaly Repeshko 2010-11-24 09:00:29 PST
(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);
}
Comment 3 Yuqiang Xian 2010-11-24 17:58:47 PST
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.
Comment 4 WebKit Review Bot 2010-11-24 18:00:28 PST
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.
Comment 5 Yuqiang Xian 2010-11-24 18:07:57 PST
Created attachment 74823 [details]
updated patch

resolve the style issue.
Comment 6 Vitaly Repeshko 2010-11-25 04:10:45 PST
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.
Comment 7 Pavel Feldman 2010-11-25 04:14:30 PST
Comment on attachment 74823 [details]
updated patch

Rubber stamping so that you could land it once Vitaly's comments are addressed.
Comment 8 Yuqiang Xian 2010-11-25 13:28:23 PST
Created attachment 74894 [details]
updated patch

Thanks for the comments, Vitaly.

Here's the updated patch addressing your comments.

Thanks, -Yuqiang
Comment 9 Pavel Feldman 2010-11-25 20:58:06 PST
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]
Comment 10 Eric Seidel (no email) 2010-11-27 06:38:04 PST
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.
Comment 11 Yuqiang Xian 2010-11-29 21:55:31 PST
Created attachment 75103 [details]
updated patch

Update the patch addressing Pavel's comment.

Thanks, -Yuqiang
Comment 12 WebKit Commit Bot 2010-12-02 02:38:21 PST
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 eric.carlson@apple.com and hclam@google.com.  The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2010-12-02 02:40:23 PST
Comment on attachment 75103 [details]
updated patch

Clearing flags on attachment: 75103

Committed r73119: <http://trac.webkit.org/changeset/73119>
Comment 14 WebKit Commit Bot 2010-12-02 02:40:29 PST
All reviewed patches have been landed.  Closing bug.