Make gMask_00FF00FF a constant

Make gMask_00FF00FF a constant

This is to optimize SkAlphaMulQ() in PIC mode. With the visibility=default
symbol the constant is not known at compile time (and is not a constant), but
instead is fetched through a double indirection through GOT. The function is
quite hot on one of the chromium benchmarks:
rasterize_and_record_micro.key_silk_cases.

This change replaces the symbol with a compile-time constant. As a bonus the
variable is not exported from the dynamic library, i. e. a cleaner library
interface.

See specific performance improvements on Android here:
  http://goo.gl/iMuTDt

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

[pasko, 2014-05-06 16:07:49]

Tom, Sami, please advise on proper reviewers for this.

How would you suggest to test it? would default tryjobs be enough? FWIW: I ran
skia "bench" as a PIE on gprecise, there is no visible impact (pic code on x64
is more efficient).

[Sami, 2014-05-06 16:15:36]

Neat find. Did you notice any difference in the rasterize_time from
rasterize_and_record_micro?

Do we know why this constant was made extern in the first place? Trying to avoid
immediate encoding overhead?

[pasko, 2014-05-06 16:46:02]

On 2014/05/06 16:15:36, Sami wrote:
> Neat find. Did you notice any difference in the rasterize_time from
> rasterize_and_record_micro?

Yes, I was investigating a ~14% regression in rasterize_time, and after the
patch it goes away (or almost goes away, I did not perform very extensive
testing, that'd be later).

> Do we know why this constant was made extern in the first place? Trying to
avoid
> immediate encoding overhead?

Not sure. The symbol was explicitly exported 3 years ago for no good reason:
https://codereview.appspot.com/4282042/diff/7002/include/core/SkColorPriv.h#n...

I was lazy to track it down further :)

[Sami, 2014-05-06 17:21:23]

+random selection of skia folks since Tom is out.

[reed1, 2014-05-06 17:24:38]

For a while at least, making this a global was in fact faster, because the
compiler was re-generating this mask each time inside loops.

If this sort of change does not slow down any of skia's benchs, then I'm fine
(but lets do run them w/ and w/o). In that case, I see no reason to have a
symbol at all, but the sites should just say 0xFF00FF.

[mtklein, 2014-05-06 17:41:44]

On 2014/05/06 17:24:38, reed1 wrote:
> For a while at least, making this a global was in fact faster, because the
> compiler was re-generating this mask each time inside loops.
> 
> If this sort of change does not slow down any of skia's benchs, then I'm fine
> (but lets do run them w/ and w/o). In that case, I see no reason to have a
> symbol at all, but the sites should just say 0xFF00FF.

Great doc.  If it's better on Android and, as you say, no different on x64, I'm
in.  Let's inline 0xFF00FF.

[pasko, 2014-05-12 14:35:30]

On 2014/05/06 17:24:38, reed1 wrote:
> For a while at least, making this a global was in fact faster, because the
> compiler was re-generating this mask each time inside loops.
> 
> If this sort of change does not slow down any of skia's benchs, then I'm fine
> (but lets do run them w/ and w/o). In that case, I see no reason to have a
> symbol at all, but the sites should just say 0xFF00FF.

I looked a little further into history in attempt to find the rationale for
gMask_00FF00FF. The git blame for the body of SkAlphaMulQ() originates from the
commit 8a1c16ff (December 2008, "grab from latest android"). The version before
that actually contained the literal constant, *not* the global, but it was
deleted entirely. Roughly at that time in 2008 Skia was moved to external/ in
the AOSP. Apparently the history of this change hides somewhere in the android
tree.

Looking at trybot results this change seems to introduce no perf difference with
GCC 4.6. The main benefit should be observable with GCC 4.8 which we are about
to switch to.

Generally it is a good idea to avoid global exports from a DSO, if a compiler
generates worse code in case it knows the constant at compile time, this should
be fixed in the compiler.

[reed1, 2014-05-12 14:48:06]

lgtm

https://codereview.chromium.org/270473003/diff/1/include/core/SkMath.h
File include/core/SkMath.h (right):

https://codereview.chromium.org/270473003/diff/1/include/core/SkMath.h#newcode17
include/core/SkMath.h:17: enum { kMask_00FF00FF = 0xFF00FF };
I think we should just use 0xFF00FF where the sites need it, rather than have an
enum for this.

[reed1, 2014-05-12 15:04:59]

oops, not lgtm (until we can eliminate the enum)

[pasko, 2014-05-12 15:06:18]

https://codereview.chromium.org/270473003/diff/1/include/core/SkMath.h
File include/core/SkMath.h (right):

https://codereview.chromium.org/270473003/diff/1/include/core/SkMath.h#newcode17
include/core/SkMath.h:17: enum { kMask_00FF00FF = 0xFF00FF };
On 2014/05/12 14:48:07, reed1 wrote:
> I think we should just use 0xFF00FF where the sites need it, rather than have
an
> enum for this.

I agree. Done. Also removed one commented-out leftover gMask_00FF00FF.

[reed1, 2014-05-12 15:08:34]

lgtm

I wonder about the 'static' in the filter file, but I concede that need not be
tied to this CL.

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
File src/core/SkBitmapProcState_filter.h (right):

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:29: static const uint32_t mask = 0xFF00FF;
do we want this to still be 'static' ?

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:59: static const uint32_t mask = 0xFF00FF;
do we want this to still be 'static' ?

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:89: static const uint32_t mask = 0xFF00FF;
do we want this to still be 'static' ?

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:111: static const uint32_t mask = 0xFF00FF;
do we want this to still be 'static' ?

[pasko, 2014-05-12 15:19:45]

let me know if you'd prefer s/mask/kMask/

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
File src/core/SkBitmapProcState_filter.h (right):

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:29: static const uint32_t mask = 0xFF00FF;
On 2014/05/12 15:08:35, reed1 wrote:
> do we want this to still be 'static' ?

we don't need it, and the code generated by GCC is identical. Done.

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:59: static const uint32_t mask = 0xFF00FF;
On 2014/05/12 15:08:35, reed1 wrote:
> do we want this to still be 'static' ?

Done.

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:89: static const uint32_t mask = 0xFF00FF;
On 2014/05/12 15:08:35, reed1 wrote:
> do we want this to still be 'static' ?

Done.

https://codereview.chromium.org/270473003/diff/20001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState_filter.h:111: static const uint32_t mask = 0xFF00FF;
On 2014/05/12 15:08:35, reed1 wrote:
> do we want this to still be 'static' ?

Done.

[reed1, 2014-05-12 15:20:46]

lgtm

[tomhudson, 2014-05-12 15:30:44]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-05-12 15:31:50]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/pasko@chromium.org/270473003/40001

[commit-bot: I haz the power, 2014-05-12 15:38:08]

Change committed as 14696