From 148ec59001ca7d7e54aec580a048c6dd2a085991 Mon Sep 17 00:00:00 2001 From: mtklein Date: Mon, 13 Oct 2014 13:17:56 -0700 Subject: [PATCH] Require SK_DECLARE_STATIC_LAZY_PTR is used in global scope. Function- or method- local scope isn't threadsafe; the pointer is generally zero-initialized on first use in function scope (i.e. lazily... we have to go deeper), but for globals we can be pretty sure the linker will do that for us. BUG=skia: No public API changes. TBR=reed@google.com Review URL: https://codereview.chromium.org/651723003 --- include/core/SkData.h | 3 +-- include/core/SkPathRef.h | 2 +- include/ports/SkFontMgr.h | 2 +- include/ports/SkRemotableFontMgr.h | 3 ++- src/core/SkData.cpp | 9 ++++----- src/core/SkFontHost.cpp | 6 ++++-- src/core/SkGlyphCache.cpp | 3 ++- src/core/SkImageFilter.cpp | 5 +++-- src/core/SkLazyPtr.h | 9 ++++++--- src/core/SkMessageBus.h | 21 +++++++++------------ src/core/SkPathRef.cpp | 6 ++++-- src/core/SkTypeface.cpp | 20 +++++++++++--------- src/core/SkXfermode.cpp | 4 ++-- src/fonts/SkRemotableFontMgr.cpp | 5 +++-- src/lazy/SkDiscardableMemoryPool.cpp | 3 ++- 15 files changed, 55 insertions(+), 46 deletions(-) diff --git a/include/core/SkData.h b/include/core/SkData.h index e25ef50..4f0c213 100644 --- a/include/core/SkData.h +++ b/include/core/SkData.h @@ -171,8 +171,7 @@ private: virtual void internal_dispose() const SK_OVERRIDE; // Called the first time someone calls NewEmpty to initialize the singleton. - static SkData* NewEmptyImpl(); - static void DeleteEmpty(SkData*); + friend SkData* sk_new_empty_data(); // shared internal factory static SkData* PrivateNewWithCopy(const void* srcOrNull, size_t length); diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h index 0b661b4..4b57fc8 100644 --- a/include/core/SkPathRef.h +++ b/include/core/SkPathRef.h @@ -421,7 +421,7 @@ private: /** * Called the first time someone calls CreateEmpty to actually create the singleton. */ - static SkPathRef* CreateEmptyImpl(); + friend SkPathRef* sk_create_empty_pathref(); void setIsOval(bool isOval) { fIsOval = isOval; } diff --git a/include/ports/SkFontMgr.h b/include/ports/SkFontMgr.h index bb8c7b7..181fe9f 100644 --- a/include/ports/SkFontMgr.h +++ b/include/ports/SkFontMgr.h @@ -131,7 +131,7 @@ protected: unsigned styleBits) const = 0; private: static SkFontMgr* Factory(); // implemented by porting layer - static SkFontMgr* CreateDefault(); + friend SkFontMgr* sk_fontmgr_create_default(); typedef SkRefCnt INHERITED; }; diff --git a/include/ports/SkRemotableFontMgr.h b/include/ports/SkRemotableFontMgr.h index bd99497..bf24599 100644 --- a/include/ports/SkRemotableFontMgr.h +++ b/include/ports/SkRemotableFontMgr.h @@ -46,7 +46,8 @@ public: private: SkRemotableFontIdentitySet() : fCount(0), fData() { } - static SkRemotableFontIdentitySet* NewEmptyImpl(); + + friend SkRemotableFontIdentitySet* sk_remotable_font_identity_set_new(); int fCount; SkAutoTMalloc fData; diff --git a/src/core/SkData.cpp b/src/core/SkData.cpp index 11cab7e..c5d1077 100644 --- a/src/core/SkData.cpp +++ b/src/core/SkData.cpp @@ -92,14 +92,13 @@ SkData* SkData::PrivateNewWithCopy(const void* srcOrNull, size_t length) { /////////////////////////////////////////////////////////////////////////////// -SkData* SkData::NewEmptyImpl() { - return new SkData(NULL, 0, NULL, NULL); -} +// As a template argument these must have external linkage. +SkData* sk_new_empty_data() { return new SkData(NULL, 0, NULL, NULL); } +namespace { void sk_unref_data(SkData* ptr) { return SkSafeUnref(ptr); } } -void SkData::DeleteEmpty(SkData* ptr) { SkDELETE(ptr); } +SK_DECLARE_STATIC_LAZY_PTR(SkData, empty, sk_new_empty_data, sk_unref_data); SkData* SkData::NewEmpty() { - SK_DECLARE_STATIC_LAZY_PTR(SkData, empty, NewEmptyImpl, DeleteEmpty); return SkRef(empty.get()); } diff --git a/src/core/SkFontHost.cpp b/src/core/SkFontHost.cpp index c582ba5..ce73491 100644 --- a/src/core/SkFontHost.cpp +++ b/src/core/SkFontHost.cpp @@ -198,12 +198,14 @@ SkTypeface* SkFontMgr::legacyCreateTypeface(const char familyName[], return this->onLegacyCreateTypeface(familyName, styleBits); } -SkFontMgr* SkFontMgr::CreateDefault() { +// As a template argument this must have external linkage. +SkFontMgr* sk_fontmgr_create_default() { SkFontMgr* fm = SkFontMgr::Factory(); return fm ? fm : SkNEW(SkEmptyFontMgr); } +SK_DECLARE_STATIC_LAZY_PTR(SkFontMgr, singleton, sk_fontmgr_create_default); + SkFontMgr* SkFontMgr::RefDefault() { - SK_DECLARE_STATIC_LAZY_PTR(SkFontMgr, singleton, CreateDefault); return SkRef(singleton.get()); } diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp index ab816f9..17f4236 100755 --- a/src/core/SkGlyphCache.cpp +++ b/src/core/SkGlyphCache.cpp @@ -29,9 +29,10 @@ SkGlyphCache_Globals* create_globals() { } // namespace +SK_DECLARE_STATIC_LAZY_PTR(SkGlyphCache_Globals, globals, create_globals); + // Returns the shared globals static SkGlyphCache_Globals& getSharedGlobals() { - SK_DECLARE_STATIC_LAZY_PTR(SkGlyphCache_Globals, globals, create_globals); return *globals.get(); } diff --git a/src/core/SkImageFilter.cpp b/src/core/SkImageFilter.cpp index 6f7762a..c431ece 100644 --- a/src/core/SkImageFilter.cpp +++ b/src/core/SkImageFilter.cpp @@ -97,7 +97,7 @@ bool SkImageFilter::Common::unflatten(SkReadBuffer& buffer, int expectedCount) { if (!buffer.isValid() || !buffer.validate(SkIsValidRect(rect))) { return false; } - + uint32_t flags = buffer.readUInt(); fCropRect = CropRect(rect, flags); if (buffer.isVersionLT(SkReadBuffer::kImageFilterUniqueID_Version)) { @@ -495,7 +495,8 @@ SkImageFilter::Cache* SkImageFilter::Cache::Create(size_t maxBytes) { return SkNEW_ARGS(CacheImpl, (maxBytes)); } +SK_DECLARE_STATIC_LAZY_PTR(SkImageFilter::Cache, cache, CreateCache); + SkImageFilter::Cache* SkImageFilter::Cache::Get() { - SK_DECLARE_STATIC_LAZY_PTR(SkImageFilter::Cache, cache, CreateCache); return cache.get(); } diff --git a/src/core/SkLazyPtr.h b/src/core/SkLazyPtr.h index 13218a7..0612327 100644 --- a/src/core/SkLazyPtr.h +++ b/src/core/SkLazyPtr.h @@ -47,15 +47,18 @@ * We may call Create more than once, but all threads will see the same pointer * returned from get(). Any extra calls to Create will be cleaned up. * - * These macros must be used in a global or function scope, not as a class member. + * These macros must be used in a global scope, not in function scope or as a class member. */ #define SK_DECLARE_STATIC_LAZY_PTR(T, name, ...) \ - static Private::SkLazyPtr name + namespace {} static Private::SkLazyPtr name #define SK_DECLARE_STATIC_LAZY_PTR_ARRAY(T, name, N, ...) \ - static Private::SkLazyPtrArray name + namespace {} static Private::SkLazyPtrArray name +// namespace {} forces these macros to only be legal in global scopes. Chrome has thread-safety +// problems with them in function-local statics because it uses -fno-threadsafe-statics, and even +// in builds with threadsafe statics, those threadsafe statics are just unnecessary overhead. // Everything below here is private implementation details. Don't touch, don't even look. diff --git a/src/core/SkMessageBus.h b/src/core/SkMessageBus.h index 1290ea9..a005c3b 100644 --- a/src/core/SkMessageBus.h +++ b/src/core/SkMessageBus.h @@ -38,19 +38,21 @@ public: private: SkMessageBus(); static SkMessageBus* Get(); - static SkMessageBus* New(); + + // Allow SkLazyPtr to call SkMessageBus::SkMessageBus(). + template friend T* Private::sk_new(); SkTDArray fInboxes; SkMutex fInboxesMutex; }; // This must go in a single .cpp file, not some .h, or we risk creating more than one global -// SkMessageBus per type when using shared libraries. -#define DECLARE_SKMESSAGEBUS_MESSAGE(Message) \ - template <> \ - SkMessageBus* SkMessageBus::Get() { \ - SK_DECLARE_STATIC_LAZY_PTR(SkMessageBus, bus, New); \ - return bus.get(); \ +// SkMessageBus per type when using shared libraries. NOTE: at most one per file will compile. +#define DECLARE_SKMESSAGEBUS_MESSAGE(Message) \ + SK_DECLARE_STATIC_LAZY_PTR(SkMessageBus, bus); \ + template <> \ + SkMessageBus* SkMessageBus::Get() { \ + return bus.get(); \ } // ----------------------- Implementation of SkMessageBus::Inbox ----------------------- @@ -97,11 +99,6 @@ template SkMessageBus::SkMessageBus() {} template -/*static*/ SkMessageBus* SkMessageBus::New() { - return SkNEW(SkMessageBus); -} - -template /*static*/ void SkMessageBus::Post(const Message& m) { SkMessageBus* bus = SkMessageBus::Get(); SkAutoMutexAcquire lock(bus->fInboxesMutex); diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp index 5707693..dffb6a3 100644 --- a/src/core/SkPathRef.cpp +++ b/src/core/SkPathRef.cpp @@ -29,14 +29,16 @@ SkPathRef::Editor::Editor(SkAutoTUnref* pathRef, ////////////////////////////////////////////////////////////////////////////// -SkPathRef* SkPathRef::CreateEmptyImpl() { +// As a template argument, this must have external linkage. +SkPathRef* sk_create_empty_pathref() { SkPathRef* empty = SkNEW(SkPathRef); empty->computeBounds(); // Avoids races later to be the first to do this. return empty; } +SK_DECLARE_STATIC_LAZY_PTR(SkPathRef, empty, sk_create_empty_pathref); + SkPathRef* SkPathRef::CreateEmpty() { - SK_DECLARE_STATIC_LAZY_PTR(SkPathRef, empty, CreateEmptyImpl); return SkRef(empty.get()); } diff --git a/src/core/SkTypeface.cpp b/src/core/SkTypeface.cpp index f948787..81038bc 100644 --- a/src/core/SkTypeface.cpp +++ b/src/core/SkTypeface.cpp @@ -79,8 +79,12 @@ protected: } }; +namespace { + SK_DECLARE_STATIC_MUTEX(gCreateDefaultMutex); -SkTypeface* SkTypeface::CreateDefault(int style) { + +// As a template arguments, these must have external linkage. +SkTypeface* sk_create_default_typeface(int style) { // If backed by fontconfig, it's not safe to call SkFontHost::CreateTypeface concurrently. // To be safe, we serialize here with a mutex so only one call to // CreateTypeface is happening at any given time. @@ -92,16 +96,14 @@ SkTypeface* SkTypeface::CreateDefault(int style) { return t ? t : SkEmptyTypeface::Create(); } -void SkTypeface::DeleteDefault(SkTypeface* t) { - // The SkTypeface returned by SkFontHost::CreateTypeface may _itself_ be a - // cleverly-shared singleton. This is less than ideal. This means we - // cannot just assert our ownership and SkDELETE(t) like we'd want to. - SkSafeUnref(t); -} +void sk_unref_typeface(SkTypeface* ptr) { SkSafeUnref(ptr); } -SkTypeface* SkTypeface::GetDefaultTypeface(Style style) { - SK_DECLARE_STATIC_LAZY_PTR_ARRAY(SkTypeface, defaults, 4, CreateDefault, DeleteDefault); +} // namespace + +SK_DECLARE_STATIC_LAZY_PTR_ARRAY(SkTypeface, defaults, 4, + sk_create_default_typeface, sk_unref_typeface); +SkTypeface* SkTypeface::GetDefaultTypeface(Style style) { SkASSERT((int)style < 4); return defaults[style]; } diff --git a/src/core/SkXfermode.cpp b/src/core/SkXfermode.cpp index 7fcdeb8..e115f6f 100644 --- a/src/core/SkXfermode.cpp +++ b/src/core/SkXfermode.cpp @@ -1211,7 +1211,7 @@ private: return fMode == s.fMode && fBackgroundAccess.getTexture() == s.fBackgroundAccess.getTexture(); } - + virtual void onComputeInvariantOutput(InvariantOutput* inout) const SK_OVERRIDE { inout->setToUnknown(); } @@ -1708,6 +1708,7 @@ SkXfermode* create_mode(int iMode) { } } // namespace +SK_DECLARE_STATIC_LAZY_PTR_ARRAY(SkXfermode, cached, SkXfermode::kLastMode + 1, create_mode); SkXfermode* SkXfermode::Create(Mode mode) { SkASSERT(SK_ARRAY_COUNT(gProcCoeffs) == kModeCount); @@ -1723,7 +1724,6 @@ SkXfermode* SkXfermode::Create(Mode mode) { return NULL; } - SK_DECLARE_STATIC_LAZY_PTR_ARRAY(SkXfermode, cached, kModeCount, create_mode); return SkSafeRef(cached[mode]); } diff --git a/src/fonts/SkRemotableFontMgr.cpp b/src/fonts/SkRemotableFontMgr.cpp index 633e914..3681f2c 100644 --- a/src/fonts/SkRemotableFontMgr.cpp +++ b/src/fonts/SkRemotableFontMgr.cpp @@ -16,11 +16,12 @@ SkRemotableFontIdentitySet::SkRemotableFontIdentitySet(int count, SkFontIdentity *data = fData; } -SkRemotableFontIdentitySet* SkRemotableFontIdentitySet::NewEmptyImpl() { +// As a template argument, this must have external linkage. +SkRemotableFontIdentitySet* sk_remotable_font_identity_set_new() { return SkNEW(SkRemotableFontIdentitySet); } +SK_DECLARE_STATIC_LAZY_PTR(SkRemotableFontIdentitySet, empty, sk_remotable_font_identity_set_new); SkRemotableFontIdentitySet* SkRemotableFontIdentitySet::NewEmpty() { - SK_DECLARE_STATIC_LAZY_PTR(SkRemotableFontIdentitySet, empty, NewEmptyImpl); return SkRef(empty.get()); } diff --git a/src/lazy/SkDiscardableMemoryPool.cpp b/src/lazy/SkDiscardableMemoryPool.cpp index f8f44d0..96d6046 100644 --- a/src/lazy/SkDiscardableMemoryPool.cpp +++ b/src/lazy/SkDiscardableMemoryPool.cpp @@ -260,8 +260,9 @@ SkDiscardableMemoryPool* SkDiscardableMemoryPool::Create(size_t size, SkBaseMute return SkNEW_ARGS(DiscardableMemoryPool, (size, mutex)); } +SK_DECLARE_STATIC_LAZY_PTR(SkDiscardableMemoryPool, global, create_global_pool); + SkDiscardableMemoryPool* SkGetGlobalDiscardableMemoryPool() { - SK_DECLARE_STATIC_LAZY_PTR(SkDiscardableMemoryPool, global, create_global_pool); return global.get(); } -- 2.7.4