Rewrite memset benches, then use results to add a small-N optimization.

Rewrite memset benches, then use results to add a small-N optimization.

The benches for N <= 10 get around 2x faster on my N7 and N9.  I believe this
is because of the reduced function-call-then-function-pointer-call overhead on
the N7, and additionally because it seems autovectorization beats our NEON code
for small N on the N9.

My desktop is unchanged, though that's probably because N=10 lies well within a
region where memset's performance is essentially constant: N=100 takes only
about 2x as long as N=1 and N=10, which perform nearly identically.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/9ff378b01be0b0a3fc35677a2155ba4ade286cc2

[mtklein_C, 2015-04-09 17:46:16]

mtklein@chromium.org changed reviewers:
+ reed@google.com

[mtklein_C, 2015-04-09 17:46:21]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-09 17:46:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073863002/1

[commit-bot: I haz the power, 2015-04-09 17:53:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-09 17:53:33]

This issue passed the CQ dry run.

[reed1, 2015-04-09 18:55:48]

aside: I see we have sk_memcpy32? I wonder if the same sort of wrapper could
give us wins (acknowledging that that is unrelated to the current ARM ASM attack
strategy).

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

https://codereview.chromium.org/1073863002/diff/1/include/core/SkUtils.h#newc...
include/core/SkUtils.h:20: void sk_memset16_large(uint16_t dst[], uint16_t
value, int count);
Seems like its not much harder to provide both N values...

// These values were determined running ...
#ifdef SK_BUILD_FOR_ARM && NOT_64BIT
    #define kMEMSET_SMALL_CUTOFF    10
#else
    #define kMEMSET_SMALL_CUTOFF    10000
#endif

[reed1, 2015-04-09 18:56:51]

Just read the part of your desc where the desktop was unchanged. Interesting
(and a little surprising).

[mtklein, 2015-04-09 19:11:09]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-04-09 19:11:10]

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

https://codereview.chromium.org/1073863002/diff/1/include/core/SkUtils.h#newc...
include/core/SkUtils.h:20: void sk_memset16_large(uint16_t dst[], uint16_t
value, int count);
On 2015/04/09 18:55:48, reed1 wrote:
> Seems like its not much harder to provide both N values...
> 
> // These values were determined running ...
> #ifdef SK_BUILD_FOR_ARM && NOT_64BIT
>     #define kMEMSET_SMALL_CUTOFF    10
> #else
>     #define kMEMSET_SMALL_CUTOFF    10000
> #endif

Good point!  Done.

[mtklein, 2015-04-09 19:11:52]

The CQ bit was checked by mtklein@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-09 19:12:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073863002/20001

[commit-bot: I haz the power, 2015-04-09 19:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-09 19:19:00]

This issue passed the CQ dry run.

[reed1, 2015-04-09 20:30:24]

lgtm w/ comments about visibility of new symbols in a public header. Fine if we
address those comments later.

https://codereview.chromium.org/1073863002/diff/20001/include/core/SkUtils.h
File include/core/SkUtils.h (right):

https://codereview.chromium.org/1073863002/diff/20001/include/core/SkUtils.h#...
include/core/SkUtils.h:18: #define SK_SMALL_MEMSET 1000
I know I suggested this be a #define. Since this is a public header, and its
#defines will be visible to clients code, I wonder if we think this name should
be longer/more-obscure, and/or a const int, or just leave it as is.

https://codereview.chromium.org/1073863002/diff/20001/include/core/SkUtils.h#...
include/core/SkUtils.h:30: void sk_memset16_large(uint16_t dst[], uint16_t
value, int count);
These two "large" functions. I know they have to be declared for the inliner,
but its a shame we have to expose them to our clients. (ugh if a client started
explicitly using sk_memset16_large).

Maybe the real fix is to move all of these mem functions into a new private
header, since only skia itself should get to use our goodies.

[mtklein, 2015-04-09 20:36:42]

https://codereview.chromium.org/1073863002/diff/20001/include/core/SkUtils.h
File include/core/SkUtils.h (right):

https://codereview.chromium.org/1073863002/diff/20001/include/core/SkUtils.h#...
include/core/SkUtils.h:18: #define SK_SMALL_MEMSET 1000
On 2015/04/09 20:30:24, reed1 wrote:
> I know I suggested this be a #define. Since this is a public header, and its
> #defines will be visible to clients code, I wonder if we think this name
should
> be longer/more-obscure, and/or a const int, or just leave it as is.

Hmm.  Actually, I can just undef it when we're done with it so it doesn't leak
out.

https://codereview.chromium.org/1073863002/diff/20001/include/core/SkUtils.h#...
include/core/SkUtils.h:30: void sk_memset16_large(uint16_t dst[], uint16_t
value, int count);
On 2015/04/09 20:30:24, reed1 wrote:
> These two "large" functions. I know they have to be declared for the inliner,
> but its a shame we have to expose them to our clients. (ugh if a client
started
> explicitly using sk_memset16_large).
> 
> Maybe the real fix is to move all of these mem functions into a new private
> header, since only skia itself should get to use our goodies.

Moving this code around sounds good to me.  I hadn't realized anyone was
actually using SkUtils.h outside Skia itself.

[mtklein, 2015-04-09 20:36:47]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-04-09 20:36:48]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1073863002/#ps40001
(title: "undef")

[commit-bot: I haz the power, 2015-04-09 20:37:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073863002/40001

[commit-bot: I haz the power, 2015-04-09 21:05:23]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/9ff378b01be0b0a3fc35677a2155ba4ade286cc2