commit b9b5de5c66a8bb4aff0aef9da350a1f3883ea6ad
author Mike Klein <mtklein@chromium.org> Wed Sep 27 13:26:22 2017 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Sep 28 01:59:06 2017 +0000
tree ae6276e7b2acbe7f073bfabcd584ce7433013deb
parent 809cbedd4b252be221b2ac3b4269d312fd8f53a0

make SkPath::fConvexity atomic

The TSAN bots fail regularly with races on fConvexity.  Very annoying.

We used to have this very same problem with SkPath::fFirstDirection
until we made it atomic.  This does the same to fConvexity.

This makes the field as lightly atomic as possible, with all operations
using a relaxed memory order.  The value of fConvexity isn't guarding
any other non-atomic memory or implying any other writes have happened
so I don't think we need anything beyond relaxed here.

Change-Id: I0da1f892dc2b7072d692ce8b460fb1862aebef77
Reviewed-on: https://skia-review.googlesource.com/52180
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Mike Klein <mtklein@chromium.org>

src/core/SkPath.cpp

diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index c3a7c6b..a460990 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -175,10 +175,11 @@
     //fPathRef is assumed to have been set by the caller.
     fLastMoveToIndex = that.fLastMoveToIndex;
     fFillType        = that.fFillType;
-    fConvexity       = that.fConvexity;
-    // Simulate fFirstDirection  = that.fFirstDirection;
-    fFirstDirection.store(that.fFirstDirection.load());
     fIsVolatile      = that.fIsVolatile;
+
+    // Non-atomic assignment of atomic values.
+    fConvexity     .store(that.fConvexity     .load());
+    fFirstDirection.store(that.fFirstDirection.load());
 }
 
 bool operator==(const SkPath& a, const SkPath& b) {
@@ -193,12 +194,16 @@
         fPathRef.swap(that.fPathRef);
         SkTSwap<int>(fLastMoveToIndex, that.fLastMoveToIndex);
         SkTSwap<uint8_t>(fFillType, that.fFillType);
-        SkTSwap<uint8_t>(fConvexity, that.fConvexity);
-        // Simulate SkTSwap<uint8_t>(fFirstDirection, that.fFirstDirection);
-        uint8_t temp = fFirstDirection;
-        fFirstDirection.store(that.fFirstDirection.load());
-        that.fFirstDirection.store(temp);
         SkTSwap<SkBool8>(fIsVolatile, that.fIsVolatile);
+
+        // Non-atomic swaps of atomic values.
+        Convexity c = fConvexity.load();
+        fConvexity.store(that.fConvexity.load());
+        that.fConvexity.store(c);
+
+        uint8_t fd = fFirstDirection.load();
+        fFirstDirection.store(that.fFirstDirection.load());
+        that.fFirstDirection.store(fd);
     }
 }
 
@@ -1742,7 +1747,7 @@
 
         if (this != dst) {
             dst->fFillType = fFillType;
-            dst->fConvexity = fConvexity;
+            dst->fConvexity.store(fConvexity);
             dst->fIsVolatile = fIsVolatile;
         }
 
@@ -2158,7 +2163,7 @@
         return 0;
     }
 
-    fConvexity = (packed >> kConvexity_SerializationShift) & 0xFF;
+    fConvexity.store( (Convexity)((packed >> kConvexity_SerializationShift) & 0xFF) );
     fFillType = fillType;
     fIsVolatile = (packed >> kIsVolatile_SerializationShift) & 0x1;
     SkPathRef* pathRef = SkPathRef::CreateFromBuffer(&buffer);
@@ -3550,7 +3555,7 @@
     if (this->getSegmentMasks() == SkPath::kLine_SegmentMask) {
         return this->getBounds();
     }
-    
+
     SkPoint extremas[5]; // big enough to hold worst-case curve type (cubic) extremas + 1
     SkPoint pts[4];
     SkPath::RawIter iter(*this);