Minimize SkTHash object copying

Minimize SkTHash object copying

AKA ref-constify the world.

R=mtklein@google.com

Committed: https://skia.googlesource.com/skia/+/79ca081b906b53ad0ee760042e3ea225f39de794

[f(malita), 2015-02-13 00:11:20]

So I was porting the SVG font family CL to use the new goodness, and just for
kicks (*cough*) I took a look at the number of SkString copies. Turns out that
on my Linux build (gcc 4.8.3) we're generating lots of copies :(

The newly added test for example makes 9 temp copies just for the initial set()
call (and 70+ overall) without this change.

WDYT?

[mtklein, 2015-02-13 00:37:22]

lgtm

Aww shucks.

https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h
File src/core/SkTHash.h (right):

https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h#newco...
src/core/SkTHash.h:87: if (s.val != val) {
Interesting.  Is this one mattering?  We hadn't required V == or != before this.

[f(malita), 2015-02-13 00:47:30]

https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h
File src/core/SkTHash.h (right):

https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h#newco...
src/core/SkTHash.h:87: if (s.val != val) {
On 2015/02/13 00:37:22, mtklein wrote:
> Interesting.  Is this one mattering?  We hadn't required V == or != before
this.

It avoids copies when adding the same value repeatedly. I'm not sure it's worth
the added inequality operator requirements though... unless we really-really
want to keep the number of copies at an absolute minimum.

Now that you pointed it out, I'm inclined to remove it :) My current use case
checks for contains() anyway so maybe this isn't a problem in general. Maybe add
a comment instead?

[f(malita), 2015-02-13 00:59:40]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[f(malita), 2015-02-13 01:00:54]

https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h
File src/core/SkTHash.h (right):

https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h#newco...
src/core/SkTHash.h:87: if (s.val != val) {
On 2015/02/13 00:47:30, f(malita) wrote:
> On 2015/02/13 00:37:22, mtklein wrote:
> > Interesting.  Is this one mattering?  We hadn't required V == or != before
> this.
> 
> It avoids copies when adding the same value repeatedly. I'm not sure it's
worth
> the added inequality operator requirements though... unless we really-really
> want to keep the number of copies at an absolute minimum.
> 
> Now that you pointed it out, I'm inclined to remove it :) My current use case
> checks for contains() anyway so maybe this isn't a problem in general. Maybe
add
> a comment instead?

Done.

[mtklein, 2015-02-13 01:28:42]

On 2015/02/13 01:00:54, f(malita) wrote:
> https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h
> File src/core/SkTHash.h (right):
> 
>
https://codereview.chromium.org/919193002/diff/40001/src/core/SkTHash.h#newco...
> src/core/SkTHash.h:87: if (s.val != val) {
> On 2015/02/13 00:47:30, f(malita) wrote:
> > On 2015/02/13 00:37:22, mtklein wrote:
> > > Interesting.  Is this one mattering?  We hadn't required V == or != before
> > this.
> > 
> > It avoids copies when adding the same value repeatedly. I'm not sure it's
> worth
> > the added inequality operator requirements though... unless we really-really
> > want to keep the number of copies at an absolute minimum.
> > 
> > Now that you pointed it out, I'm inclined to remove it :) My current use
case
> > checks for contains() anyway so maybe this isn't a problem in general. Maybe
> add
> > a comment instead?
> 
> Done.

Yeah... it's pretty uncommon to call add() or set() blindly without a contains()
or find() first, and even then... probably don't care too much what happens.

lgtm++

[mtklein, 2015-02-13 01:28:48]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-02-13 01:29:25]

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

[commit-bot: I haz the power, 2015-02-13 01:32:53]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/79ca081b906b53ad0ee760042e3ea225f39de794