Change using SET method to direct accessing.
Created attachment 208515 [details] Patch
In this case, the type of pixelArray->data() is unsigned char. It means we can avoid calling of SET function. And I measured the spent time of before and after patching when visit http://www.w3.org/TR/filter-effects/examples/feComponentTransfer.svg. Before adopting this patch, the function spent 190(ms). After patching, it spent only 35(ms). So this could reduce 80% of spent time. Below is about testing environment. OS: Ubuntu 12.04 64bit CPU: Intel Core i7-3770 CPU @ 3.40GHz Memory: 8,123,696 kB
Comment on attachment 208515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208515&action=review > Source/WebCore/ChangeLog:7 > + Missing changelog. Please explain what you did and why it is better / faster. > Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:-171 > - unsigned char c = pixelArray->item(pixelOffset + channel); Removing the char variable reduces the readability a bit IMHO.
Comment on attachment 208515 [details] Patch +1 to Christophe's review.
Created attachment 208582 [details] Patch
Ok, I inserted some comment in ChangeLog. But the code that you checked is the deletion part of my changes.
(In reply to comment #6) > Ok, I inserted some comment in ChangeLog. > But the code that you checked is the deletion part of my changes. Does keeping the 'char c' local variable really affect performance when building in release with -O2 or -O3? This would be surprising. What makes your code really faster is avoiding the call to set() and therefore skipping data validation (since you already know the data is fine).
Comment on attachment 208582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208582&action=review The change looks good overall. > Source/WebCore/ChangeLog:8 > + Use direct writing to target data instead of calling SET function. SET -> Uint8ClampedArray::set() > Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:-171 > - unsigned char c = pixelArray->item(pixelOffset + channel); could we maybe keep "unsigned char c = data[pixelOffset + channel];" just to improve readability a bit. It shouldn't affect performance but let me know if I'm wrong.
Created attachment 208600 [details] Patch
Comment on attachment 208600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208600&action=review > Source/WebCore/ChangeLog:11 > + The SET function has index checking code, value checking code and value casting code. nit: Please update other uses of SET in the changelog as well > Source/WebCore/ChangeLog:14 > + Thus all works in SET function is duplicated. duplicated -> redundant > Source/WebCore/ChangeLog:15 > + So deletion of calling SET function do not reduce stability, but improve the performance. Removing the call to set() does not reduce stability but improves performance.
Created attachment 208601 [details] Patch
Comment on attachment 208601 [details] Patch Looks good, thanks. r=me but please let Dean take a final look before landing as he was reviewing this as well.
CC'ing Dean, could you take a final look ? I would land this patch :)
Dean, if you don't have time to review this patch until tonight, I will ask another committer for landing this patch.
Comment on attachment 208601 [details] Patch (In reply to comment #14) > Dean, if you don't have time to review this patch until tonight, I will ask another committer for landing this patch. Looks good.
Comment on attachment 208601 [details] Patch Clearing flags on attachment: 208601 Committed r154042: <http://trac.webkit.org/changeset/154042>
All reviewed patches have been landed. Closing bug.