Bug 119671 – FEComponentTransfer could be faster

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Jinwoo Jeong]

Change using SET method to direct accessing.

[Jinwoo Jeong]

Created attachment 208515 [details]
Patch

[Jinwoo Jeong]

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

[Chris Dumez]

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.

[Dean Jackson]

Comment on attachment 208515 [details]
Patch

+1 to Christophe's review.

[Jinwoo Jeong]

Created attachment 208582 [details]
Patch

[Jinwoo Jeong]

Ok, I inserted some comment in ChangeLog.
But the code that you checked is the deletion part of my changes.

[Chris Dumez]

(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).

[Chris Dumez]

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.

[Jinwoo Jeong]

Created attachment 208600 [details]
Patch

[Chris Dumez]

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.

[Jinwoo Jeong]

Created attachment 208601 [details]
Patch

[Chris Dumez]

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.

[Jinwoo Jeong]

CC'ing Dean, could you take a final look ? I would land this patch :)

[Jinwoo Jeong]

Dean, if you don't have time to review this patch until tonight, I will ask another committer for landing this patch.

[Dirk Schulze]

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.

[WebKit Commit Bot]

Comment on attachment 208601 [details]
Patch

Clearing flags on attachment: 208601

Committed r154042: <http://trac.webkit.org/changeset/154042>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.