SkOnce in is_lcd_supported instead of hand rolled double-checked locking.

SkOnce in is_lcd_supported instead of hand rolled double-checked locking.

BUG=skia:

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

[mtklein, 2014-02-05 17:08:37]

TSAN is catching the unprotected if (!gLCDSupportValid) read.

[bungeman-skia, 2014-02-05 18:02:03]

https://codereview.chromium.org/155963003/diff/1/src/ports/SkFontHost_FreeTyp...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/155963003/diff/1/src/ports/SkFontHost_FreeTyp...
src/ports/SkFontHost_FreeType.cpp:179: SkAutoMutexAcquire lock(gFTMutex);
Why are we still taking this lock, just to return a global?

[mtklein, 2014-02-05 18:10:26]

https://codereview.chromium.org/155963003/diff/1/src/ports/SkFontHost_FreeTyp...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/155963003/diff/1/src/ports/SkFontHost_FreeTyp...
src/ports/SkFontHost_FreeType.cpp:179: SkAutoMutexAcquire lock(gFTMutex);
On 2014/02/05 18:02:04, bungeman1 wrote:
> Why are we still taking this lock, just to return a global?

You know, that's a thought provoking question.

I was going to say, for consistency with the rest of the locking discipline
around these freetype globals.  gLCDSupport, gLCDExtra, and gLCDSupportValid are
weird near-consts that are the exception to the normal locking discipline rather
than the norm.

But now I'm looking at it, and clearly this is more complicated but no different
than just writing

static bool is_lcd_supported() {
  SkAutoMutexAcquire lock(gFTMutex);
  if (!gLCDSupportValid) { ... }
  return gLCDSupport;
}

It bothers me to have two locks involved here.  Going back to drawing board.

[mtklein, 2014-02-05 19:00:17]

On 2014/02/05 18:10:26, mtklein wrote:
>
https://codereview.chromium.org/155963003/diff/1/src/ports/SkFontHost_FreeTyp...
> File src/ports/SkFontHost_FreeType.cpp (right):
> 
>
https://codereview.chromium.org/155963003/diff/1/src/ports/SkFontHost_FreeTyp...
> src/ports/SkFontHost_FreeType.cpp:179: SkAutoMutexAcquire lock(gFTMutex);
> On 2014/02/05 18:02:04, bungeman1 wrote:
> > Why are we still taking this lock, just to return a global?
> 
> You know, that's a thought provoking question.
> 
> I was going to say, for consistency with the rest of the locking discipline
> around these freetype globals.  gLCDSupport, gLCDExtra, and gLCDSupportValid
are
> weird near-consts that are the exception to the normal locking discipline
rather
> than the norm.
> 
> But now I'm looking at it, and clearly this is more complicated but no
different
> than just writing
> 
> static bool is_lcd_supported() {
>   SkAutoMutexAcquire lock(gFTMutex);
>   if (!gLCDSupportValid) { ... }
>   return gLCDSupport;
> }
> 
> It bothers me to have two locks involved here.  Going back to drawing board.

Have another look?  I've got it using the same gFTMutex now.

I'm half-convinced, half-puzzled by TSAN's assertion that I need to copy from
gLCDSupport to lcdSupported while inside the mutex.  It complains about "return
gLCDSupport" being read not under the mutex when given this version:

static void determine_lcd_support(int) {
    if (!gLCDSupportValid) {
        // This will determine LCD support as a side effect.
        InitFreetype();
        FT_Done_FreeType(gFTLibrary);
    }
    SkASSERT(gLCDSupportValid);
}
static bool is_lcd_supported() {
    SkOnce(&gLCDSupportValid, &gFTMutex, determine_lcd_support, 0);
    return gLCDSupport;
}

[bungeman-skia, 2014-02-05 19:12:58]

lgtm

https://codereview.chromium.org/155963003/diff/110001/include/core/SkOnce.h
File include/core/SkOnce.h (right):

https://codereview.chromium.org/155963003/diff/110001/include/core/SkOnce.h#n...
include/core/SkOnce.h:43: // If you've already got a lock and a flag to use,
this variant lets you avoid an extra SKOnceFlag.
Lowercase the 'K'? ('SkOnceFlag')

[mtklein, 2014-02-10 18:17:23]

On 2014/02/05 19:12:58, bungeman1 wrote:
> lgtm
> 
> https://codereview.chromium.org/155963003/diff/110001/include/core/SkOnce.h
> File include/core/SkOnce.h (right):
> 
>
https://codereview.chromium.org/155963003/diff/110001/include/core/SkOnce.h#n...
> include/core/SkOnce.h:43: // If you've already got a lock and a flag to use,
> this variant lets you avoid an extra SKOnceFlag.
> Lowercase the 'K'? ('SkOnceFlag')

Done.

[mtklein, 2014-02-10 18:17:46]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-02-10 18:17:50]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/155963003/160001

[commit-bot: I haz the power, 2014-02-10 18:17:53]

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

[commit-bot: I haz the power, 2014-02-10 18:17:53]

Presubmit check for 155963003-160001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[mtklein, 2014-02-10 18:18:23]

+reed for include/core

[reed1, 2014-02-10 19:43:38]

lgtm

[mtklein, 2014-02-10 19:43:52]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-02-10 19:44:10]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/155963003/160001

[commit-bot: I haz the power, 2014-02-10 19:58:52]

Change committed as 13387