Make StringImpl flags atomic and threadsafe.

This leaves just the ref_count_ variable left as thread hostile.

CL is broken out of
https://chromium-review.googlesource.com/c/chromium/src/+/1557854

See this design doc for more details on the larger motivation for
this change:
https://docs.google.com/document/d/1hp7h6zfD5S6mnMI4cHe1PpacRnb2rNpRRkn1gBBJHuQ/edit#heading=h.erv1bv18t616

Bug: 1083392
Change-Id: I9ab3b1d86c8e27d887b6dbb34ac4ae4fd720af0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2301589
Commit-Queue: Albert J. Wong <ajwong@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790158}

third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc

@@ -37,7 +37,7 @@ struct UCharBufferTranslator {
       string->AddRef();
     location = string.get();
     location->SetHash(hash);
-    location->SetIsAtomic(true);
+    location->SetIsAtomic();
   }
 };
 
@@ -118,7 +118,7 @@ struct HashAndUTF8CharactersTranslator {
     new_string->AddRef();
     location = new_string.get();
     location->SetHash(hash);
-    location->SetIsAtomic(true);
+    location->SetIsAtomic();
   }
 };
 
@@ -181,7 +181,7 @@ AtomicStringTable::~AtomicStringTable() {
   for (StringImpl* string : table_) {
     if (!string->IsStatic()) {
       DCHECK(string->IsAtomic());
-      string->SetIsAtomic(false);
+      string->UnsetIsAtomic();
     }
   }
 }
@@ -230,7 +230,7 @@ struct LCharBufferTranslator {
     string->AddRef();
     location = string.get();
     location->SetHash(hash);
-    location->SetIsAtomic(true);
+    location->SetIsAtomic();
   }
 };
 
@@ -253,7 +253,7 @@ StringImpl* AtomicStringTable::Add(StringImpl* string) {
   StringImpl* result = *table_.insert(string).stored_value;
 
   if (!result->IsAtomic())
-    result->SetIsAtomic(true);
+    result->SetIsAtomic();
 
   DCHECK(!string->IsStatic() || result->IsStatic());
   return result;

third_party/blink/renderer/platform/wtf/text/string_impl.cc

@@ -55,7 +55,7 @@ struct SameSizeAsStringImpl {
 
 ASSERT_SIZE(StringImpl, SameSizeAsStringImpl);
 
-}  // anonymous namespace
+}  // namespace
 
 void* StringImpl::operator new(size_t size) {
   DCHECK_EQ(size, sizeof(StringImpl));
@@ -78,11 +78,18 @@ void StringImpl::DestroyIfNotStatic() const {
     delete this;
 }
 
-void StringImpl::UpdateContainsOnlyASCIIOrEmpty() const {
-  contains_only_ascii_ = Is8Bit()
-                             ? CharactersAreAllASCII(Characters8(), length())
-                             : CharactersAreAllASCII(Characters16(), length());
-  needs_ascii_check_ = false;
+bool StringImpl::ContainsOnlyASCIIOrEmptySlowCase() const {
+  bool contains_only_ascii =
+      Is8Bit() ? CharactersAreAllASCII(Characters8(), length())
+               : CharactersAreAllASCII(Characters16(), length());
+  uint32_t new_flags = kAsciiCheckDone;
+  if (contains_only_ascii)
+    new_flags |= kContainsOnlyAscii;
+  const uint32_t previous_flags =
+      hash_and_flags_.fetch_or(new_flags, std::memory_order_relaxed);
+  static constexpr uint32_t mask = kAsciiCheckDone | kContainsOnlyAscii;
+  DCHECK((previous_flags & mask) == 0 || (previous_flags & mask) == new_flags);
+  return contains_only_ascii;
 }
 
 bool StringImpl::IsSafeToSendToAnotherThread() const {

third_party/blink/renderer/platform/wtf/text/string_impl.h

@@ -26,6 +26,7 @@
 
 #include <limits.h>
 #include <string.h>
+#include <atomic>
 
 #include "base/containers/span.h"
 #include "base/macros.h"
@@ -92,14 +93,9 @@ class WTF_EXPORT StringImpl {
   // ref-counted in a non-threadsafe manner.
   enum ConstructEmptyStringTag { kConstructEmptyString };
   explicit StringImpl(ConstructEmptyStringTag)
-      : ref_count_(1),
-        length_(0),
-        hash_(0),
-        contains_only_ascii_(true),
-        needs_ascii_check_(false),
-        is_atomic_(false),
-        is_8bit_(true),
-        is_static_(true) {
+      : length_(0),
+        hash_and_flags_(kAsciiCheckDone | kContainsOnlyAscii | kIs8Bit |
+                        kIsStatic) {
     // Ensure that the hash is computed so that AtomicStringHash can call
     // existingHash() with impunity. The empty string is special because it
     // is never entered into AtomicString's HashKey, but still needs to
@@ -109,53 +105,28 @@ class WTF_EXPORT StringImpl {
 
   enum ConstructEmptyString16BitTag { kConstructEmptyString16Bit };
   explicit StringImpl(ConstructEmptyString16BitTag)
-      : ref_count_(1),
-        length_(0),
-        hash_(0),
-        contains_only_ascii_(true),
-        needs_ascii_check_(false),
-        is_atomic_(false),
-        is_8bit_(false),
-        is_static_(true) {
+      : length_(0),
+        hash_and_flags_(kAsciiCheckDone | kContainsOnlyAscii | kIsStatic) {
     GetHash();
   }
 
   // FIXME: there has to be a less hacky way to do this.
   enum Force8Bit { kForce8BitConstructor };
   StringImpl(wtf_size_t length, Force8Bit)
-      : ref_count_(1),
-        length_(length),
-        hash_(0),
-        contains_only_ascii_(!length),
-        needs_ascii_check_(static_cast<bool>(length)),
-        is_atomic_(false),
-        is_8bit_(true),
-        is_static_(false) {
+      : length_(length), hash_and_flags_(LengthToAsciiFlags(length) | kIs8Bit) {
     DCHECK(length_);
   }
 
   StringImpl(wtf_size_t length)
-      : ref_count_(1),
-        length_(length),
-        hash_(0),
-        contains_only_ascii_(!length),
-        needs_ascii_check_(static_cast<bool>(length)),
-        is_atomic_(false),
-        is_8bit_(false),
-        is_static_(false) {
+      : length_(length), hash_and_flags_(LengthToAsciiFlags(length)) {
     DCHECK(length_);
   }
 
   enum StaticStringTag { kStaticString };
   StringImpl(wtf_size_t length, wtf_size_t hash, StaticStringTag)
-      : ref_count_(1),
-        length_(length),
-        hash_(hash),
-        contains_only_ascii_(!length),
-        needs_ascii_check_(static_cast<bool>(length)),
-        is_atomic_(false),
-        is_8bit_(true),
-        is_static_(true) {}
+      : length_(length),
+        hash_and_flags_(hash << kHashShift | LengthToAsciiFlags(length) |
+                        kIs8Bit | kIsStatic) {}
 
  public:
   REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE();
@@ -201,7 +172,9 @@ class WTF_EXPORT StringImpl {
                                                        UChar*& data);
 
   wtf_size_t length() const { return length_; }
-  bool Is8Bit() const { return is_8bit_; }
+  bool Is8Bit() const {
+    return hash_and_flags_.load(std::memory_order_relaxed) & kIs8Bit;
+  }
 
   ALWAYS_INLINE const LChar* Characters8() const {
     DCHECK(Is8Bit());
@@ -230,10 +203,21 @@ class WTF_EXPORT StringImpl {
     return length() * (Is8Bit() ? sizeof(LChar) : sizeof(UChar));
   }
 
-  bool IsAtomic() const { return is_atomic_; }
-  void SetIsAtomic(bool is_atomic) { is_atomic_ = is_atomic; }
+  bool IsAtomic() const {
+    return hash_and_flags_.load(std::memory_order_acquire) & kIsAtomic;
+  }
+
+  void SetIsAtomic() {
+    hash_and_flags_.fetch_or(kIsAtomic, std::memory_order_release);
+  }
+
+  void UnsetIsAtomic() {
+    hash_and_flags_.fetch_and(~kIsAtomic, std::memory_order_release);
+  }
 
-  bool IsStatic() const { return is_static_; }
+  bool IsStatic() const {
+    return hash_and_flags_.load(std::memory_order_relaxed) & kIsStatic;
+  }
 
   bool ContainsOnlyASCIIOrEmpty() const;
 
@@ -244,27 +228,26 @@ class WTF_EXPORT StringImpl {
   // access.  So, we shift left and right when setting and getting our hash
   // code.
   void SetHash(wtf_size_t hash) const {
-    DCHECK(!HasHash());
     // Multiple clients assume that StringHasher is the canonical string
     // hash function.
     DCHECK(hash == (Is8Bit() ? StringHasher::ComputeHashAndMaskTop8Bits(
                                    Characters8(), length_)
                              : StringHasher::ComputeHashAndMaskTop8Bits(
                                    Characters16(), length_)));
-    hash_ = hash;
     DCHECK(hash);  // Verify that 0 is a valid sentinel hash value.
+    SetHashRaw(hash);
   }
 
-  bool HasHash() const { return hash_ != 0; }
+  bool HasHash() const { return GetHashRaw() != 0; }
 
   wtf_size_t ExistingHash() const {
     DCHECK(HasHash());
-    return hash_;
+    return GetHashRaw();
   }
 
   wtf_size_t GetHash() const {
-    if (HasHash())
-      return ExistingHash();
+    if (wtf_size_t hash = GetHashRaw())
+      return hash;
     return HashSlowCase();
   }
 
@@ -459,6 +442,57 @@ class WTF_EXPORT StringImpl {
   static const UChar kLatin1CaseFoldTable[256];
 
  private:
+  enum Flags {
+    // These two fields are never modified for the lifetime of the StringImpl.
+    // It is therefore safe to read them with a relaxed operation.
+    kIs8Bit = 1 << 0,
+    kIsStatic = 1 << 1,
+
+    // This is the only flag that can be both set and unset. It is safe to do
+    // so because all accesses are mediated by the same atomic string table and
+    // so protected by thread locality (pre-unification) or a mutex
+    // (post-unification). Thus these accesses can also be relaxed.
+    kIsAtomic = 1 << 2,
+
+    // These two bits are set atomically together. They are initially both
+    // zero, and like the hash computation below, become non-zero only as part
+    // of a single atomic bitwise or. Thus concurrent loads will always observe
+    // either a state where the ASCII check has not been completed and both
+    // bits are zero, or a state where the state is fully populated.
+    kAsciiCheckDone = 1 << 3,
+    kContainsOnlyAscii = 1 << 4,
+
+    // The last 24 bits (past kHashShift) are reserved for the hash.
+    // These bits are all zero if the hash is uncomputed, and the hash is
+    // atomically stored with bitwise or.
+    //
+    // Therefore a relaxed read can be used, and will either observe an
+    // uncomputed hash (if the fetch_or is not yet visible on this thread)
+    // or the correct hash (if it is). It is possible for a thread to compute
+    // the hash for a second time if there is a race. This is safe, since
+    // storing the same bits again with a bitwise or is idempotent.
+  };
+
+  // Hash value is 24 bits.
+  constexpr static int kHashShift = (sizeof(unsigned) * 8) - 24;
+
+  static inline constexpr uint32_t LengthToAsciiFlags(int length) {
+    return length ? 0 : kAsciiCheckDone | kContainsOnlyAscii;
+  }
+
+  void SetHashRaw(unsigned hash_val) const {
+    // Setting the hash is idempotent so fetch_or() is sufficient. DCHECK()
+    // as a sanity check.
+    unsigned previous_value = hash_and_flags_.fetch_or(
+        hash_val << kHashShift, std::memory_order_relaxed);
+    DCHECK(((previous_value >> kHashShift) == 0) ||
+           ((previous_value >> kHashShift) == hash_val));
+  }
+
+  unsigned GetHashRaw() const {
+    return hash_and_flags_.load(std::memory_order_relaxed) >> kHashShift;
+  }
+
   template <typename CharType>
   static size_t AllocationSize(wtf_size_t length) {
     static_assert(
@@ -485,7 +519,7 @@ class WTF_EXPORT StringImpl {
   NOINLINE wtf_size_t HashSlowCase() const;
 
   void DestroyIfNotStatic() const;
-  void UpdateContainsOnlyASCIIOrEmpty() const;
+  bool ContainsOnlyASCIIOrEmptySlowCase() const;
 
 #if DCHECK_IS_ON()
   std::string AsciiForDebugging() const;
@@ -501,18 +535,12 @@ class WTF_EXPORT StringImpl {
   }
 #endif
 
- private:
 #if DCHECK_IS_ON()
   mutable ThreadRestrictionVerifier verifier_;
 #endif
-  mutable unsigned ref_count_;
+  mutable unsigned ref_count_{1};
   const unsigned length_;
-  mutable unsigned hash_ : 24;
-  mutable unsigned contains_only_ascii_ : 1;
-  mutable unsigned needs_ascii_check_ : 1;
-  unsigned is_atomic_ : 1;
-  const unsigned is_8bit_ : 1;
-  const unsigned is_static_ : 1;
+  mutable std::atomic_uint32_t hash_and_flags_;
 
   DISALLOW_COPY_AND_ASSIGN(StringImpl);
 };
@@ -554,9 +582,10 @@ inline bool Equal(const char* a, StringImpl* b) {
 WTF_EXPORT bool EqualNonNull(const StringImpl* a, const StringImpl* b);
 
 ALWAYS_INLINE bool StringImpl::ContainsOnlyASCIIOrEmpty() const {
-  if (needs_ascii_check_)
-    UpdateContainsOnlyASCIIOrEmpty();
-  return contains_only_ascii_;
+  uint32_t flags = hash_and_flags_.load(std::memory_order_relaxed);
+  if (flags & kAsciiCheckDone)
+    return flags & kContainsOnlyAscii;
+  return ContainsOnlyASCIIOrEmptySlowCase();
 }
 
 template <typename CharType>