commit to xfermode objects being immutable

commit to xfermode objects being immutable

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11614

[reed1, 2013-10-04 14:18:36]

mike, I can't get my new bench to "converge in 4000ms". What am I doing wrong?

[mtklein, 2013-10-04 14:26:40]

https://codereview.chromium.org/25968004/diff/3001/bench/XfermodeBench.cpp
File bench/XfermodeBench.cpp (right):

https://codereview.chromium.org/25968004/diff/3001/bench/XfermodeBench.cpp#ne...
bench/XfermodeBench.cpp:76: for (int outer = 0; outer < N; ++outer) {
Instead of N, use this->getLoops().  (Or N * this->getLoops() if you like, but
I'd drop the N.)

Here's how I diagnosed:
$ out/Release/bench --match xfermode_create --verbose

xfermode_create 8888: 2.76 1.36 0.679 0.342 0.17 0.0852 0.0425 0.0132 0.00439
0.0022 0.0011 0.000549 0.000275 0.000137 6.91e-05 3.43e-05 1.73e-05 8.58e-06
4.32e-06 2.16e-06 1.08e-06 5.4e-07 2.68e-07 1.34e-07 6.73e-08 3.35e-08 1.68e-08
8.38e-09 4.19e-09 2.09e-09 1.05e-09 Can't get xfermode_create 8888 to converge
in 4000ms.

... (other configs elided)

Notice how the times displayed are halving each time?  That's because it's
ramping up this->getLoops() by two each time, but the bench is ignoring it and
running a constant number of times.

[reed1, 2013-10-04 14:39:57]

bench is fixed.

~25% speed up for creating these xfermodes, but more importantly (I assert) is
that xfermodes will not force a deep-copy in picture.clone.

PTAL

[djsollen, 2013-10-04 14:48:28]

https://codereview.chromium.org/25968004/diff/9001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/25968004/diff/9001/src/core/SkXfermode.cpp#ne...
src/core/SkXfermode.cpp:1691: static SkMutex gCachedXfermodesMutex;
use SK_DECLARE_STATIC_MUTEX

[mtklein, 2013-10-04 15:07:01]

https://codereview.chromium.org/25968004/diff/9001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/25968004/diff/9001/src/core/SkXfermode.cpp#ne...
src/core/SkXfermode.cpp:1722: switch (mode) {
As someone who's not too familiar with this code, some things are tripping me
up:
  - why is kSrcOver special?
  - why do all the other modes get lumped into ProcCoeffXfermode?

I realize this isn't a logical change.  The original code confuses me too. :)

[reed1, 2013-10-04 15:45:07]

On 2013/10/04 15:07:01, mtklein wrote:
> https://codereview.chromium.org/25968004/diff/9001/src/core/SkXfermode.cpp
> File src/core/SkXfermode.cpp (right):
> 
>
https://codereview.chromium.org/25968004/diff/9001/src/core/SkXfermode.cpp#ne...
> src/core/SkXfermode.cpp:1722: switch (mode) {
> As someone who's not too familiar with this code, some things are tripping me
> up:
>   - why is kSrcOver special?

srcover *happens* to be skia's default, so when we see null in a paint, it means
"srcover".

>   - why do all the other modes get lumped into ProcCoeffXfermode?

lazy coders, and perceived unimportance of making the other modes faster, as we
think they are only rarely used.

> 
> I realize this isn't a logical change.  The original code confuses me too. :)

[reed1, 2013-10-04 15:59:18]

dox added.

PTAL

if its ok, go ahead and try the CQ, as I am leaving the office now.

[mtklein, 2013-10-04 16:07:22]

On 2013/10/04 15:59:18, reed1 wrote:
> dox added.
> 
> PTAL
> 
> if its ok, go ahead and try the CQ, as I am leaving the office now.

Derek, can you take a look at PS 5 and see if I've done the static mutex
declaration correctly?

This otherwise LGTM to me.

[djsollen, 2013-10-04 16:19:27]

lgtm

https://codereview.chromium.org/25968004/diff/20001/bench/XfermodeBench.cpp
File bench/XfermodeBench.cpp (right):

https://codereview.chromium.org/25968004/diff/20001/bench/XfermodeBench.cpp#n...
bench/XfermodeBench.cpp:75: for (int outer = 0; outer < this->getLoops() * 10;
++outer) {
do we really need the 10x multiplier here

[mtklein, 2013-10-04 16:24:57]

https://codereview.chromium.org/25968004/diff/20001/bench/XfermodeBench.cpp
File bench/XfermodeBench.cpp (right):

https://codereview.chromium.org/25968004/diff/20001/bench/XfermodeBench.cpp#n...
bench/XfermodeBench.cpp:75: for (int outer = 0; outer < this->getLoops() * 10;
++outer) {
On 2013/10/04 16:19:27, djsollen wrote:
> do we really need the 10x multiplier here

Not really, but it doesn't hurt.  The only effect it'll have is scaling the
output.  I'm seeing 11.38 vs. 1.13.

[commit-bot: I haz the power, 2013-10-04 16:25:44]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/25968004/20001

[commit-bot: I haz the power, 2013-10-04 16:52:56]

Change committed as 11614