From d5e3e6ae1b3434ad1158f441902ff65f1eeaa3a7 Mon Sep 17 00:00:00 2001 From: mtklein Date: Tue, 8 Jul 2014 12:30:39 -0700 Subject: [PATCH] Add SkRacy SkRacy is a zero-overhead wrapper for a T, except it also silences race warnings when TSAN is running. Here we apply in several classes. In SkMatrix and SkPathRef, we use it to opportunistically cache some idempotent work. In SkPixelRef, we wrap the genIDs. We think the worst that can happen here is we'll increment the global next-genID a few times instead of once when we go to get another ID. BUG=skia: R=reed@google.com, mtklein@google.com Author: mtklein@chromium.org Review URL: https://codereview.chromium.org/371363004 --- include/core/SkDynamicAnnotations.h | 38 +++++++++++++++++++++++++++++ include/core/SkMatrix.h | 5 ++-- include/core/SkPathRef.h | 16 ++++++------ include/core/SkPixelRef.h | 5 ++-- src/core/SkPathRef.cpp | 22 ++++++++--------- tools/tsan.supp | 18 -------------- 6 files changed, 63 insertions(+), 41 deletions(-) diff --git a/include/core/SkDynamicAnnotations.h b/include/core/SkDynamicAnnotations.h index 6d21cddb94..f9a80a31ef 100644 --- a/include/core/SkDynamicAnnotations.h +++ b/include/core/SkDynamicAnnotations.h @@ -19,6 +19,8 @@ extern "C" { // TSAN provides these hooks. void AnnotateIgnoreReadsBegin(const char* file, int line); void AnnotateIgnoreReadsEnd(const char* file, int line); +void AnnotateBenignRaceSized(const char* file, int line, + const void* addr, long size, const char* desc); } // extern "C" // SK_ANNOTATE_UNPROTECTED_READ can wrap any variable read to tell TSAN to ignore that it appears to @@ -37,10 +39,46 @@ inline T SK_ANNOTATE_UNPROTECTED_READ(const volatile T& x) { return read; } +// Ignore racy reads and racy writes to this pointer, indefinitely. +// If at all possible, use the more precise SK_ANNOTATE_UNPROTECTED_READ. +template +void SK_ANNOTATE_BENIGN_RACE(T* ptr) { + AnnotateBenignRaceSized(__FILE__, __LINE__, ptr, sizeof(*ptr), "SK_ANNOTATE_BENIGN_RACE"); +} + #else // !DYNAMIC_ANNOTATIONS_ENABLED #define SK_ANNOTATE_UNPROTECTED_READ(x) (x) +#define SK_ANNOTATE_BENIGN_RACE(ptr) #endif +// Can be used to wrap values that are intentionally racy, usually small mutable cached values, e.g. +// - SkMatrix type mask +// - SkPathRef bounds +// - SkPixelRef genIDs +template +class SkTRacy { +public: + SkTRacy() { SK_ANNOTATE_BENIGN_RACE(&fVal); } + + operator const T&() const { + return fVal; + } + + SkTRacy& operator=(const T& val) { + fVal = val; + return *this; + } + + const T* get() const { return &fVal; } + T* get() { return &fVal; } + + const T* operator->() const { return &fVal; } + T* operator->() { return &fVal; } + +private: + T fVal; +}; + #endif//SkDynamicAnnotations_DEFINED diff --git a/include/core/SkMatrix.h b/include/core/SkMatrix.h index 84d1a87e95..bfa03de7c5 100644 --- a/include/core/SkMatrix.h +++ b/include/core/SkMatrix.h @@ -10,6 +10,7 @@ #ifndef SkMatrix_DEFINED #define SkMatrix_DEFINED +#include "SkDynamicAnnotations.h" #include "SkRect.h" class SkString; @@ -643,7 +644,7 @@ private: }; SkScalar fMat[9]; - mutable uint32_t fTypeMask; + mutable SkTRacy fTypeMask; uint8_t computeTypeMask() const; uint8_t computePerspectiveTypeMask() const; @@ -664,7 +665,7 @@ private: void clearTypeMask(int mask) { // only allow a valid mask SkASSERT((mask & kAllMasks) == mask); - fTypeMask &= ~mask; + fTypeMask = fTypeMask & ~mask; } TypeMask getPerspectiveTypeMaskOnly() const { diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h index 47a69b7e73..7ce2d8d319 100644 --- a/include/core/SkPathRef.h +++ b/include/core/SkPathRef.h @@ -9,6 +9,7 @@ #ifndef SkPathRef_DEFINED #define SkPathRef_DEFINED +#include "SkDynamicAnnotations.h" #include "SkMatrix.h" #include "SkPoint.h" #include "SkRect.h" @@ -292,7 +293,7 @@ private: SkDEBUGCODE(this->validate();) SkASSERT(fBoundsIsDirty); - fIsFinite = ComputePtBounds(&fBounds, *this); + fIsFinite = ComputePtBounds(fBounds.get(), *this); fBoundsIsDirty = false; } @@ -300,7 +301,7 @@ private: SkASSERT(rect.fLeft <= rect.fRight && rect.fTop <= rect.fBottom); fBounds = rect; fBoundsIsDirty = false; - fIsFinite = fBounds.isFinite(); + fIsFinite = fBounds->isFinite(); } /** Makes additional room but does not change the counts or change the genID */ @@ -432,11 +433,12 @@ private: kMinSize = 256, }; - mutable SkRect fBounds; - uint8_t fSegmentMask; - mutable uint8_t fBoundsIsDirty; - mutable SkBool8 fIsFinite; // only meaningful if bounds are valid - mutable SkBool8 fIsOval; + mutable SkTRacy fBounds; + mutable SkTRacy fBoundsIsDirty; + mutable SkTRacy fIsFinite; // only meaningful if bounds are valid + + SkBool8 fIsOval; + uint8_t fSegmentMask; SkPoint* fPoints; // points to begining of the allocation uint8_t* fVerbs; // points just past the end of the allocation (verbs grow backwards) diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h index a997993eb1..263145ba3d 100644 --- a/include/core/SkPixelRef.h +++ b/include/core/SkPixelRef.h @@ -9,6 +9,7 @@ #define SkPixelRef_DEFINED #include "SkBitmap.h" +#include "SkDynamicAnnotations.h" #include "SkRefCnt.h" #include "SkString.h" #include "SkFlattenable.h" @@ -349,8 +350,8 @@ private: LockRec fRec; int fLockCount; - mutable uint32_t fGenerationID; - mutable bool fUniqueGenerationID; + mutable SkTRacy fGenerationID; + mutable SkTRacy fUniqueGenerationID; SkTDArray fGenIDChangeListeners; // pointers are owned diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp index de7a8f56ae..e60f618b4f 100644 --- a/src/core/SkPathRef.cpp +++ b/src/core/SkPathRef.cpp @@ -30,9 +30,7 @@ SkPathRef::Editor::Editor(SkAutoTUnref* pathRef, ////////////////////////////////////////////////////////////////////////////// SkPathRef* SkPathRef::CreateEmptyImpl() { - SkPathRef* p = SkNEW(SkPathRef); - p->computeBounds(); // Preemptively avoid a race to clear fBoundsIsDirty. - return p; + return SkNEW(SkPathRef); } SkPathRef* SkPathRef::CreateEmpty() { @@ -85,13 +83,13 @@ void SkPathRef::CreateTransformedCopy(SkAutoTUnref* dst, if (canXformBounds) { (*dst)->fBoundsIsDirty = false; if (src.fIsFinite) { - matrix.mapRect(&(*dst)->fBounds, src.fBounds); - if (!((*dst)->fIsFinite = (*dst)->fBounds.isFinite())) { - (*dst)->fBounds.setEmpty(); + matrix.mapRect((*dst)->fBounds.get(), src.fBounds); + if (!((*dst)->fIsFinite = (*dst)->fBounds->isFinite())) { + (*dst)->fBounds->setEmpty(); } } else { (*dst)->fIsFinite = false; - (*dst)->fBounds.setEmpty(); + (*dst)->fBounds->setEmpty(); } } else { (*dst)->fBoundsIsDirty = true; @@ -441,14 +439,14 @@ void SkPathRef::validate() const { SkASSERT(this->currSize() == fFreeSpace + sizeof(SkPoint) * fPointCnt + sizeof(uint8_t) * fVerbCnt); - if (!fBoundsIsDirty && !fBounds.isEmpty()) { + if (!fBoundsIsDirty && !fBounds->isEmpty()) { bool isFinite = true; for (int i = 0; i < fPointCnt; ++i) { SkASSERT(!fPoints[i].isFinite() || ( - fBounds.fLeft - fPoints[i].fX < SK_ScalarNearlyZero && - fPoints[i].fX - fBounds.fRight < SK_ScalarNearlyZero && - fBounds.fTop - fPoints[i].fY < SK_ScalarNearlyZero && - fPoints[i].fY - fBounds.fBottom < SK_ScalarNearlyZero)); + fBounds->fLeft - fPoints[i].fX < SK_ScalarNearlyZero && + fPoints[i].fX - fBounds->fRight < SK_ScalarNearlyZero && + fBounds->fTop - fPoints[i].fY < SK_ScalarNearlyZero && + fPoints[i].fY - fBounds->fBottom < SK_ScalarNearlyZero)); if (!fPoints[i].isFinite()) { isFinite = false; } diff --git a/tools/tsan.supp b/tools/tsan.supp index 6c2b0909fc..f687014040 100644 --- a/tools/tsan.supp +++ b/tools/tsan.supp @@ -25,21 +25,3 @@ race:is_lcd_supported race:RefFCI race:SkString race:SkPDF - -# These race benignly as used by DMQuiltTask: skia:2725. -# Suppress while I look for a more focused way to silence this. -race:SkPixelRef::callGenIDChangeListeners -race:SkPixelRef::needsNewGenID - -# SkPathRef caches its bounding box the first time it's needed. -# This will be fixed naturally once we create (from a single thread) a -# bounding-box hierarchy for SkRecord-based SkPictures; all bounds will come pre-cached. -# So just shut this up for now. -race:SkPathRef::computeBounds - -# SkMatrix caches a type mask. If we race on this, we'll just calculate the same thing a few times. -race:SkMatrix::getType -race:SkMatrix::rectStaysRect -race:SkMatrix::getPerspectiveTypeMaskOnly - -# TODO: some sort of SkRacy to handle cases like SkMatrix, SkPathRef, SkPixelRef above?