add counting to glyphcache, and refactor some for clarity

add counting to Globals, and refactor some for clarity

BUG=

Committed: https://code.google.com/p/skia/source/detail?r=11484

[bungeman-skia, 2013-09-25 14:46:07]

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp#new...
src/core/SkGlyphCache.cpp:619: sk_atomic_inc(&fCacheCount);
This is going to set off the chromium tsan bots. Our atomic increment provides
no memory barriers, so the next read of fCacheCount on another thread is going
to be flagged (correctly) as a race.

I'm not sure why this is here and in the constructor/destructor and not instead
handled like fTotalMemoryUsed. I understand that we could 'float above' our
total limit, but this is already true of fTotalMemoryUsed. We don't tend to
detach for long, and when we do it's pretty much only one per thread anyway.
This also means that you don't have to introduce some new memory barriers to
make this work.

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp#new...
src/core/SkGlyphCache.cpp:686: void SkGraphics::SetTLSFontCacheLimit(size_t
bytes) {
I realize that this isn't part of this change, but I find this so confusing,
that this needs to be called in order to get the thread local setup. It looks
like (from the name) it's just setting the limit, but there's no default like
there is with the similarly named SkGraphics::SetFontCacheLimit. The fact that
when you set this you also start hiding (or revealing, when setting to 0) the
process wide version is also rather surprising.

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache_Globals.h
File src/core/SkGlyphCache_Globals.h (right):

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache_Globals...
src/core/SkGlyphCache_Globals.h:15: #define SK_DEFAULT_FONT_CACHE_COUNT_LIMIT  
8
8 seems rather small, though I suppose one hardly ever cycles through more than
8 strikes in any useful manner.

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache_Globals...
src/core/SkGlyphCache_Globals.h:29: kNo_UseMutex,  // thread-local cache
These enum names seem a bit silly in that you've named them No and Yes, but then
have short comments as to what thy really mean. More confusingly it changes the
meaning of the fCacheSizeLimit since with the 'mutex' path the limits are per
process and with 'tls' they're per thread. Also, whether or not a mutex is used
seems like an implementation detail that doesn't need to be exposed. It seems
like the values are more

enum CacheLocality {
    kThread_CacheLocality,
    kProcess_CacheLocality,
}

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache_Globals...
src/core/SkGlyphCache_Globals.h:59: SkMutex*        fMutex;
SkAutoTDelete<SkMutex> ?

[reed1, 2013-09-25 17:30:31]

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp#new...
src/core/SkGlyphCache.cpp:619: sk_atomic_inc(&fCacheCount);
On 2013/09/25 14:46:07, bungeman1 wrote:
> This is going to set off the chromium tsan bots. Our atomic increment provides
> no memory barriers, so the next read of fCacheCount on another thread is going
> to be flagged (correctly) as a race.

Help me understand what you're saying. We use sk_atomic_inc all over the place.
Why is this use case different?

> 
> I'm not sure why this is here and in the constructor/destructor and not
instead
> handled like fTotalMemoryUsed.

To make it easy to ensure that I don't miss-count, I thought having the inc/dec
calls inside the constructor/destructor was the least error prone.
TotalMemoryUsed varies in lots of diff ways, but how many caches we have in play
only really changes in those two places.

 I understand that we could 'float above' our
> total limit, but this is already true of fTotalMemoryUsed. We don't tend to
> detach for long, and when we do it's pretty much only one per thread anyway.

> This also means that you don't have to introduce some new memory barriers to
> make this work.

Again, please help me understand why what I am doing is worse than the other
places we use inc/dec.

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp#new...
src/core/SkGlyphCache.cpp:686: void SkGraphics::SetTLSFontCacheLimit(size_t
bytes) {
On 2013/09/25 14:46:07, bungeman1 wrote:
> I realize that this isn't part of this change, but I find this so confusing,
> that this needs to be called in order to get the thread local setup. It looks
> like (from the name) it's just setting the limit, but there's no default like
> there is with the similarly named SkGraphics::SetFontCacheLimit. The fact that
> when you set this you also start hiding (or revealing, when setting to 0) the
> process wide version is also rather surprising.

Agreed, we should rename/document this "feature" better. I suggest I don't
tackle that change in this same CL, but lets write up a bug for it.

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache_Globals.h
File src/core/SkGlyphCache_Globals.h (right):

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache_Globals...
src/core/SkGlyphCache_Globals.h:15: #define SK_DEFAULT_FONT_CACHE_COUNT_LIMIT  
8
On 2013/09/25 14:46:07, bungeman1 wrote:
> 8 seems rather small, though I suppose one hardly ever cycles through more
than
> 8 strikes in any useful manner.

Doh! I had set it to 8 just for testing. I'll reset it to a more normal value
(e.g. 512 was my first thought).

[mtklein, 2013-09-25 18:15:25]

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp#new...
src/core/SkGlyphCache.cpp:619: sk_atomic_inc(&fCacheCount);
On 2013/09/25 17:30:31, reed1 wrote:
> On 2013/09/25 14:46:07, bungeman1 wrote:
> > This is going to set off the chromium tsan bots. Our atomic increment
provides
> > no memory barriers, so the next read of fCacheCount on another thread is
going
> > to be flagged (correctly) as a race.
> 
> Help me understand what you're saying. We use sk_atomic_inc all over the
place.
> Why is this use case different?
> 
> > 
> > I'm not sure why this is here and in the constructor/destructor and not
> instead
> > handled like fTotalMemoryUsed.
> 
> To make it easy to ensure that I don't miss-count, I thought having the
inc/dec
> calls inside the constructor/destructor was the least error prone.
> TotalMemoryUsed varies in lots of diff ways, but how many caches we have in
play
> only really changes in those two places.
> 
>  I understand that we could 'float above' our
> > total limit, but this is already true of fTotalMemoryUsed. We don't tend to
> > detach for long, and when we do it's pretty much only one per thread anyway.
> 
> > This also means that you don't have to introduce some new memory barriers to
> > make this work.
> 
> Again, please help me understand why what I am doing is worse than the other
> places we use inc/dec.
> 

I'm confused too.

Memory barriers are about preventing reordering, by the compiler and more
saliently by the CPU.  We can ask for basically every variant of "make sure the
reads and/or writes before this line are visible to the reads and/or writes past
it".  They're largely unknown because x86 provides very strong ordering
guarantees about this all the time, but platforms like ARM don't.  Does that
capture your understanding of the purpose of memory barriers, or am I confused?

As long as you're only using atomic inc/dec operations, you've got a full memory
barrier on that memory you're incrementing and decrementing.  They're atomic
operations, so there's nothing there that _can_ reorder.  But once you start
adding in other operations, like a non-atomic read or a non-atomic subtract,
there's no guarantee about what you'll see.

The only problem I see here is that fCacheCount is being used non-atomically
inside internalPurge.  If it's not possible for there to make concurrent calls
to internalPurge and to registerCache or unregisterCache, we're safe.  If calls
can be made concurrently, then that code definitely is not safe.  There are at
least 5 places where we're asking for the value of fCacheCount, and they could
all return different values.

The typical case where atomics need extra memory barriers I'm aware of is when
you're using them to indicate something about non-atomic memory.  E.g. you could
imagine you have a producer thread generating data by storing it
(non-atomically) into a shared array and then increment an atomic which
represents the last index in the array that's valid, to be consumed by another
consumer thread.

producer:
  while (not done):
    fill the next spot in the array
    atomically increment last_valid_index

consumer:
   last_consumed_index = -1
   while (last_consumed_index != the_end) {
     if (atomically check last_valid_index and assign to last_consumed_index if
not equal) {
        read the array at last_consumed_index and do something with that data
     }
   }

As I understand it, this code needs a memory barrier between the two lines in
the producer.  Without it, the processor is free to reorder the atomic increment
before we write to memory, and the consumer can then read junk.  (Caveat: this
is a sketch for example only; I'm sure the consumer is broken in tons of little
ways.)

My rule of thumb here is, use a mutex unless you're really, really, really sure
it's too slow.  If there's one thing that you can reason about safely with
concurrency (and I'm not sure there is), it's certainly a mutex.  Admittedly,
this particular code is just about the most likely place in Skia to be an
exception where we really need to be clever because the mutex is killing our
performance, but we should make sure we've got a way of measuring that and that
we have measured it.

[bungeman-skia, 2013-09-25 22:37:08]

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/24447003/diff/1/src/core/SkGlyphCache.cpp#new...
src/core/SkGlyphCache.cpp:619: sk_atomic_inc(&fCacheCount);
On 2013/09/25 17:30:31, reed1 wrote:
> On 2013/09/25 14:46:07, bungeman1 wrote:
> > This is going to set off the chromium tsan bots. Our atomic increment
provides
> > no memory barriers, so the next read of fCacheCount on another thread is
going
> > to be flagged (correctly) as a race.
> 
> Help me understand what you're saying. We use sk_atomic_inc all over the
place.
> Why is this use case different?

This use case probably isn't that different from other places where it may also
be used incorrectly. It seems to be used for reference counts like here (and I
can only state the the actual SkRefCnt/SkWeakCnt are mostly correct, I haven't
reviewed the others) and ids (those seem ok).

> 
> > 
> > I'm not sure why this is here and in the constructor/destructor and not
> instead
> > handled like fTotalMemoryUsed.
> 
> To make it easy to ensure that I don't miss-count, I thought having the
inc/dec
> calls inside the constructor/destructor was the least error prone.
> TotalMemoryUsed varies in lots of diff ways, but how many caches we have in
play
> only really changes in those two places.
> 
>  I understand that we could 'float above' our
> > total limit, but this is already true of fTotalMemoryUsed. We don't tend to
> > detach for long, and when we do it's pretty much only one per thread anyway.
> 
> > This also means that you don't have to introduce some new memory barriers to
> > make this work.
> 
> Again, please help me understand why what I am doing is worse than the other
> places we use inc/dec.
> 

See https://codereview.chromium.org/24644003/#msg1 for a similar race reported
by tsan.

[reed1, 2013-09-26 18:26:42]

ptal

[bungeman-skia, 2013-09-26 18:40:27]

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache.cpp...
src/core/SkGlyphCache.cpp:19: #define SPEW_PURGE_STATUS
Spew!!!

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache_Glo...
File src/core/SkGlyphCache_Globals.h (right):

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache_Glo...
src/core/SkGlyphCache_Globals.h:67: int     getCacheCountLimit() const { return
fCacheCountLimit; }
How are these exposed?

[reed1, 2013-09-26 19:17:14]

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache.cpp
File src/core/SkGlyphCache.cpp (right):

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache.cpp...
src/core/SkGlyphCache.cpp:19: #define SPEW_PURGE_STATUS
On 2013/09/26 18:40:27, bungeman1 wrote:
> Spew!!!

Done.

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache_Glo...
File src/core/SkGlyphCache_Globals.h (right):

https://codereview.chromium.org/24447003/diff/10001/src/core/SkGlyphCache_Glo...
src/core/SkGlyphCache_Globals.h:67: int     getCacheCountLimit() const { return
fCacheCountLimit; }
On 2013/09/26 18:40:27, bungeman1 wrote:
> How are these exposed?

Done.

[reed1, 2013-09-26 19:28:35]

Committed patchset #3 manually as r11484 (presubmit successful).