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.
Created attachment 74714 [details] proposed patch
(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); }
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.
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.
Created attachment 74823 [details] updated patch resolve the style issue.
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 on attachment 74823 [details] updated patch Rubber stamping so that you could land it once Vitaly's comments are addressed.
Created attachment 74894 [details] updated patch Thanks for the comments, Vitaly. Here's the updated patch addressing your comments. Thanks, -Yuqiang
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 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.
Created attachment 75103 [details] updated patch Update the patch addressing Pavel's comment. Thanks, -Yuqiang
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 on attachment 75103 [details] updated patch Clearing flags on attachment: 75103 Committed r73119: <http://trac.webkit.org/changeset/73119>
All reviewed patches have been landed. Closing bug.