Add SkRacy
SkRacy<T> 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:
Committed: https://skia.googlesource.com/skia/+/d5e3e6ae1b3434ad1158f441902ff65f1eeaa3a7
CQ_EXTRA_TRYBOTS=tryserver.skia:Canary-Chrome-Ubuntu13.10-Ninja-x86_64-ToT-Trybot,Canary-Chrome-Win7-Ninja-x86-SharedLib_ToT-Trybot,Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-TSAN-Trybot
R=reed@google.com, mtklein@google.com
Author: mtklein@chromium.org
Review URL: https://codereview.chromium.org/371363004
diff --git a/gyp/common_conditions.gypi b/gyp/common_conditions.gypi
index f7c4633..cc3aae4 100644
--- a/gyp/common_conditions.gypi
+++ b/gyp/common_conditions.gypi
@@ -447,7 +447,7 @@
],
'conditions' : [
[ 'skia_sanitizer == "thread"', {
- 'defines': [ 'DYNAMIC_ANNOTATIONS_ENABLED=1' ],
+ 'defines': [ 'SK_DYNAMIC_ANNOTATIONS_ENABLED=1' ],
'cflags': [ '-fPIC' ],
'target_conditions': [
[ '_type == "executable"', {
diff --git a/include/core/SkDynamicAnnotations.h b/include/core/SkDynamicAnnotations.h
index 6d21cdd..63ea7a0 100644
--- a/include/core/SkDynamicAnnotations.h
+++ b/include/core/SkDynamicAnnotations.h
@@ -12,13 +12,14 @@
// namely thread sanitizer. This is a cut-down version of the full dynamic_annotations library with
// only the features used by Skia.
-// We check the same define to know to enable the annotations, but prefix all our macros with SK_.
-#if DYNAMIC_ANNOTATIONS_ENABLED
+#if SK_DYNAMIC_ANNOTATIONS_ENABLED
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 volatile 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 +38,46 @@
return read;
}
-#else // !DYNAMIC_ANNOTATIONS_ENABLED
+// Ignore racy reads and racy writes to this pointer, indefinitely.
+// If at all possible, use the more precise SK_ANNOTATE_UNPROTECTED_READ.
+template <typename T>
+void SK_ANNOTATE_BENIGN_RACE(T* ptr) {
+ AnnotateBenignRaceSized(__FILE__, __LINE__, ptr, sizeof(*ptr), "SK_ANNOTATE_BENIGN_RACE");
+}
+
+#else // !SK_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 <typename T>
+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 84d1a87..bfa03de 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 @@
};
SkScalar fMat[9];
- mutable uint32_t fTypeMask;
+ mutable SkTRacy<uint32_t> fTypeMask;
uint8_t computeTypeMask() const;
uint8_t computePerspectiveTypeMask() const;
@@ -664,7 +665,7 @@
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 47a69b7..7ce2d8d 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 @@
SkDEBUGCODE(this->validate();)
SkASSERT(fBoundsIsDirty);
- fIsFinite = ComputePtBounds(&fBounds, *this);
+ fIsFinite = ComputePtBounds(fBounds.get(), *this);
fBoundsIsDirty = false;
}
@@ -300,7 +301,7 @@
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 @@
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<SkRect> fBounds;
+ mutable SkTRacy<uint8_t> fBoundsIsDirty;
+ mutable SkTRacy<SkBool8> 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 a997993..263145b 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 @@
LockRec fRec;
int fLockCount;
- mutable uint32_t fGenerationID;
- mutable bool fUniqueGenerationID;
+ mutable SkTRacy<uint32_t> fGenerationID;
+ mutable SkTRacy<bool> fUniqueGenerationID;
SkTDArray<GenIDChangeListener*> fGenIDChangeListeners; // pointers are owned
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index de7a8f5..e60f618 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -30,9 +30,7 @@
//////////////////////////////////////////////////////////////////////////////
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 @@
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 @@
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 6c2b090..f687014 100644
--- a/tools/tsan.supp
+++ b/tools/tsan.supp
@@ -25,21 +25,3 @@
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<T> to handle cases like SkMatrix, SkPathRef, SkPixelRef above?
