Revert of Add SkRacy (https://codereview.chromium.org/371363004/)
Reason for revert:
hidden symbol 'AnnotateBenignRaceSized' in obj/base/third_party/dynamic_annotations/libdynamic_annotations.a(obj/base/third_party/dynamic_annotations/dynamic_annotations.dynamic_annotations.o) is referenced by DSO lib/libblink_platform.so
Original issue's description:
> 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
R=reed@google.com, mtklein@chromium.org
TBR=mtklein@chromium.org, reed@google.com
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Author: mtklein@google.com
Review URL: https://codereview.chromium.org/377693005
diff --git a/include/core/SkDynamicAnnotations.h b/include/core/SkDynamicAnnotations.h
index f9a80a3..6d21cdd 100644
--- a/include/core/SkDynamicAnnotations.h
+++ b/include/core/SkDynamicAnnotations.h
@@ -19,8 +19,6 @@
// 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
@@ -39,46 +37,10 @@
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 <typename T>
-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 <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 bfa03de..84d1a87 100644
--- a/include/core/SkMatrix.h
+++ b/include/core/SkMatrix.h
@@ -10,7 +10,6 @@
#ifndef SkMatrix_DEFINED
#define SkMatrix_DEFINED
-#include "SkDynamicAnnotations.h"
#include "SkRect.h"
class SkString;
@@ -644,7 +643,7 @@
};
SkScalar fMat[9];
- mutable SkTRacy<uint32_t> fTypeMask;
+ mutable uint32_t fTypeMask;
uint8_t computeTypeMask() const;
uint8_t computePerspectiveTypeMask() const;
@@ -665,7 +664,7 @@
void clearTypeMask(int mask) {
// only allow a valid mask
SkASSERT((mask & kAllMasks) == mask);
- fTypeMask = fTypeMask & ~mask;
+ fTypeMask &= ~mask;
}
TypeMask getPerspectiveTypeMaskOnly() const {
diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h
index 7ce2d8d..47a69b7 100644
--- a/include/core/SkPathRef.h
+++ b/include/core/SkPathRef.h
@@ -9,7 +9,6 @@
#ifndef SkPathRef_DEFINED
#define SkPathRef_DEFINED
-#include "SkDynamicAnnotations.h"
#include "SkMatrix.h"
#include "SkPoint.h"
#include "SkRect.h"
@@ -293,7 +292,7 @@
SkDEBUGCODE(this->validate();)
SkASSERT(fBoundsIsDirty);
- fIsFinite = ComputePtBounds(fBounds.get(), *this);
+ fIsFinite = ComputePtBounds(&fBounds, *this);
fBoundsIsDirty = false;
}
@@ -301,7 +300,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 */
@@ -433,12 +432,11 @@
kMinSize = 256,
};
- mutable SkTRacy<SkRect> fBounds;
- mutable SkTRacy<uint8_t> fBoundsIsDirty;
- mutable SkTRacy<SkBool8> fIsFinite; // only meaningful if bounds are valid
-
- SkBool8 fIsOval;
- uint8_t fSegmentMask;
+ mutable SkRect fBounds;
+ uint8_t fSegmentMask;
+ mutable uint8_t fBoundsIsDirty;
+ mutable SkBool8 fIsFinite; // only meaningful if bounds are valid
+ mutable SkBool8 fIsOval;
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 263145b..a997993 100644
--- a/include/core/SkPixelRef.h
+++ b/include/core/SkPixelRef.h
@@ -9,7 +9,6 @@
#define SkPixelRef_DEFINED
#include "SkBitmap.h"
-#include "SkDynamicAnnotations.h"
#include "SkRefCnt.h"
#include "SkString.h"
#include "SkFlattenable.h"
@@ -350,8 +349,8 @@
LockRec fRec;
int fLockCount;
- mutable SkTRacy<uint32_t> fGenerationID;
- mutable SkTRacy<bool> fUniqueGenerationID;
+ mutable uint32_t fGenerationID;
+ mutable bool fUniqueGenerationID;
SkTDArray<GenIDChangeListener*> fGenIDChangeListeners; // pointers are owned
diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp
index e60f618..de7a8f5 100644
--- a/src/core/SkPathRef.cpp
+++ b/src/core/SkPathRef.cpp
@@ -30,7 +30,9 @@
//////////////////////////////////////////////////////////////////////////////
SkPathRef* SkPathRef::CreateEmptyImpl() {
- return SkNEW(SkPathRef);
+ SkPathRef* p = SkNEW(SkPathRef);
+ p->computeBounds(); // Preemptively avoid a race to clear fBoundsIsDirty.
+ return p;
}
SkPathRef* SkPathRef::CreateEmpty() {
@@ -83,13 +85,13 @@
if (canXformBounds) {
(*dst)->fBoundsIsDirty = false;
if (src.fIsFinite) {
- matrix.mapRect((*dst)->fBounds.get(), src.fBounds);
- if (!((*dst)->fIsFinite = (*dst)->fBounds->isFinite())) {
- (*dst)->fBounds->setEmpty();
+ matrix.mapRect(&(*dst)->fBounds, 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;
@@ -439,14 +441,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 f687014..6c2b090 100644
--- a/tools/tsan.supp
+++ b/tools/tsan.supp
@@ -25,3 +25,21 @@
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?
