[runtime] addressed review comments outlined in #1950 (Merge pull request #2332 from shawnce/SR-946_polish)

[runtime] addressed review comments outlined in #1950

Author: gparker42

include/swift/Runtime/Mutex.h

@@ -136,7 +136,7 @@ class Mutex {
   ///
   /// Precondition: Mutex not held by this thread, undefined otherwise.
   template <typename CriticalSection>
-  void lock(CriticalSection criticalSection) {
+  void withLock(CriticalSection criticalSection) {
     lock();
     criticalSection();
     unlock();
@@ -155,7 +155,7 @@ class Mutex {
   /// ...all while being correctly protected by mutex.
   ///
   /// ```
-  ///   mutex.lockOrWait(condition, [&value] {
+  ///   mutex.withLockOrWait(condition, [&value] {
   ///     if (value > 0) {
   ///       value--;
   ///       return true;
@@ -166,9 +166,9 @@ class Mutex {
   ///
   /// Precondition: Mutex not held by this thread, undefined otherwise.
   template <typename CriticalSection>
-  void lockOrWait(ConditionVariable &condition,
-                  CriticalSection criticalSection) {
-    lock([&] {
+  void withLockOrWait(ConditionVariable &condition,
+                      CriticalSection criticalSection) {
+    withLock([&] {
       while (!criticalSection()) {
         wait(condition);
       }
@@ -185,14 +185,14 @@ class Mutex {
   /// then notifies one condition waiter about this change.
   ///
   /// ```
-  ///   mutex.lockAndNotifyOne([&value] { value++; });
+  ///   mutex.withLockThenNotifyOne(condition, [&value] { value++; });
   /// ```
   ///
   /// Precondition: Mutex not held by this thread, undefined otherwise.
   template <typename CriticalSection>
-  void lockAndNotifyOne(ConditionVariable &condition,
-                        CriticalSection criticalSection) {
-    lock([&] {
+  void withLockThenNotifyOne(ConditionVariable &condition,
+                             CriticalSection criticalSection) {
+    withLock([&] {
       criticalSection();
       condition.notifyOne();
     });
@@ -208,14 +208,14 @@ class Mutex {
   /// then notifies all condition waiters about this change.
   ///
   /// ```
-  ///   mutex.lockAndNotifyAll([&value] { value++; });
+  ///   mutex.withLockThenNotifyAll(condition, [&value] { value++; });
   /// ```
   ///
   /// Precondition: Mutex not held by this thread, undefined otherwise.
   template <typename CriticalSection>
-  void lockAndNotifyAll(ConditionVariable &condition,
-                        CriticalSection criticalSection) {
-    lock([&] {
+  void withLockThenNotifyAll(ConditionVariable &condition,
+                             CriticalSection criticalSection) {
+    withLock([&] {
       criticalSection();
       condition.notifyAll();
     });
@@ -343,12 +343,12 @@ class ReadWriteLock {
   /// the read lock.
   ///
   /// ```
-  ///   rw.readLock([&value] { value = cachedValue; });
+  ///   rw.withReadLock([&value] { value = cachedValue; });
   /// ```
   ///
   /// Precondition: ReadWriteLock not held by this thread, undefined otherwise.
   template <typename CriticalSection>
-  void readLock(CriticalSection criticalSection) {
+  void withReadLock(CriticalSection criticalSection) {
     readLock();
     criticalSection();
     readUnlock();
@@ -363,61 +363,17 @@ class ReadWriteLock {
   /// the write lock.
   ///
   /// ```
-  ///   rw.writeLock([&newValue] { cachedValue = newValue });
+  ///   rw.withWriteLock([&newValue] { cachedValue = newValue });
   /// ```
   ///
   /// Precondition: ReadWriteLock not held by this thread, undefined otherwise.
   template <typename CriticalSection>
-  void writeLock(CriticalSection criticalSection) {
+  void withWriteLock(CriticalSection criticalSection) {
     writeLock();
     criticalSection();
     writeUnlock();
   }
 
-  /// Acquires read lock before calling the read critical section and if
-  /// the critical section returns `true` (done) readWriteLock returns to the
-  /// caller. If the read critical section returns `false` the read lock
-  /// will be dropped, the write lock will be acquired and then the read
-  /// critical section will be called again. Then if the critical section
-  /// returns `true` (done) readWriteLock returns to the caller. If the read
-  /// critical section returns `false` then the write critical section will
-  /// be called (with write lock still held).
-  ///
-  /// Note in all situations on return to caller the ReadWriteLock will
-  /// not be held.
-  ///
-  /// For example the following attempts to get a cachedValue if it exists
-  /// if not found it will create the needed cachedValue.
-  ///
-  /// ```
-  ///   rw.readWriteLock(
-  ///     [&value] {
-  ///       if (cachedValue) {
-  ///         value = cachedValue;
-  ///         return true;
-  ///       }
-  ///       return false;
-  ///     },
-  ///     [&value] {
-  ///       cachedValue = createValue();
-  ///       value = cachedValue;
-  ///     });
-  /// ```
-  ///
-  /// Precondition: ReadWriteLock not held by this thread, undefined otherwise.
-  template <typename ReadonlySection, typename WriteSection>
-  void readWriteLock(ReadonlySection readSection, WriteSection writeSection) {
-    bool done;
-    readLock([&done, &readSection] { done = readSection(); });
-    if (!done) {
-      writeLock([&readSection, &writeSection] {
-        if (!readSection()) {
-          writeSection();
-        }
-      });
-    }
-  }
-
 private:
   ReadWriteLockHandle Handle;
 };
@@ -485,38 +441,38 @@ class StaticMutex {
 
   /// See Mutex::lock
   template <typename CriticalSection>
-  void lock(CriticalSection criticalSection) {
+  void withLock(CriticalSection criticalSection) {
     lock();
     criticalSection();
     unlock();
   }
 
-  /// See Mutex::lockOrWait
+  /// See Mutex::withLockOrWait
   template <typename CriticalSection>
-  void lockOrWait(StaticConditionVariable &condition,
-                  CriticalSection criticalSection) {
-    lock([&] {
+  void withLockOrWait(StaticConditionVariable &condition,
+                      CriticalSection criticalSection) {
+    withLock([&] {
       while (!criticalSection()) {
         wait(condition);
       }
     });
   }
 
-  /// See Mutex::lockAndNotifyOne
+  /// See Mutex::withLockThenNotifyOne
   template <typename CriticalSection>
-  void lockAndNotifyOne(StaticConditionVariable &condition,
-                        CriticalSection criticalSection) {
-    lock([&] {
+  void withLockThenNotifyOne(StaticConditionVariable &condition,
+                             CriticalSection criticalSection) {
+    withLock([&] {
       criticalSection();
       condition.notifyOne();
     });
   }
 
-  /// See Mutex::lockAndNotifyAll
+  /// See Mutex::withLockThenNotifyAll
   template <typename CriticalSection>
-  void lockAndNotifyAll(StaticConditionVariable &condition,
-                        CriticalSection criticalSection) {
-    lock([&] {
+  void withLockThenNotifyAll(StaticConditionVariable &condition,
+                             CriticalSection criticalSection) {
+    withLock([&] {
       criticalSection();
       condition.notifyAll();
     });
@@ -566,36 +522,22 @@ class StaticReadWriteLock {
   /// See ReadWriteLock::writeUnlock
   void writeUnlock() { ReadWriteLockPlatformHelper::writeUnlock(Handle); }
 
-  /// See ReadWriteLock::readLock
+  /// See ReadWriteLock::withReadLock
   template <typename CriticalSection>
-  void readLock(CriticalSection criticalSection) {
+  void withReadLock(CriticalSection criticalSection) {
     readLock();
     criticalSection();
     readUnlock();
   }
 
-  /// See ReadWriteLock::writeLock
+  /// See ReadWriteLock::withWriteLock
   template <typename CriticalSection>
-  void writeLock(CriticalSection criticalSection) {
+  void withWriteLock(CriticalSection criticalSection) {
     writeLock();
     criticalSection();
     writeUnlock();
   }
 
-  /// See ReadWriteLock::readWriteLock
-  template <typename ReadonlySection, typename WriteSection>
-  void readWriteLock(ReadonlySection readSection, WriteSection writeSection) {
-    bool done;
-    readLock([&done, &readSection] { done = readSection(); });
-    if (!done) {
-      writeLock([&readSection, &writeSection] {
-        if (!readSection()) {
-          writeSection();
-        }
-      });
-    }
-  }
-
 private:
   ReadWriteLockHandle Handle;
 };

stdlib/public/runtime/Casting.cpp

@@ -354,30 +354,40 @@ swift_getTypeName(const Metadata *type, bool qualified) {
   Key key(type, qualified);
   auto &cache = TypeNameCache.get();
 
-  Pair pair;
-  TypeNameCacheLock.readWriteLock(
-      [&] {
-        auto found = cache.find(key);
-        if (found != cache.end()) {
-          auto result = found->second;
-          pair = Pair{result.first, result.second};
-          return true; // Cache hit, return true (e.g. done)
-        }
-        return false; // Cache missed, return false to move to write lock.
-      },
-      [&] {
-        // Build the metadata name.
-        auto name = nameForMetadata(type, qualified);
-        // Copy it to memory we can reference forever.
-        auto size = name.size();
-        auto result = (char *)malloc(size + 1);
-        memcpy(result, name.data(), size);
-        result[size] = 0;
-        cache.insert({key, {result, size}});
-        pair = Pair{result, size};
-      });
-
-  return pair;
+  // Attempt read-only lookup of cache entry.
+  {
+    StaticScopedReadLock guard(TypeNameCacheLock);
+
+    auto found = cache.find(key);
+    if (found != cache.end()) {
+      auto result = found->second;
+      return Pair{result.first, result.second};
+    }
+  }
+
+  // Read-only lookup failed to find item, we may need to create it.
+  {
+    StaticScopedWriteLock guard(TypeNameCacheLock);
+
+    // Do lookup again just to make sure it wasn't created by another
+    // thread before we acquired the write lock.
+    auto found = cache.find(key);
+    if (found != cache.end()) {
+      auto result = found->second;
+      return Pair{result.first, result.second};
+    }
+
+    // Build the metadata name.
+    auto name = nameForMetadata(type, qualified);
+    // Copy it to memory we can reference forever.
+    auto size = name.size();
+    auto result = (char *)malloc(size + 1);
+    memcpy(result, name.data(), size);
+    result[size] = 0;
+
+    cache.insert({key, {result, size}});
+    return Pair{result, size};
+  }
 }
 
 /// Report a dynamic cast failure.

stdlib/public/runtime/Errors.cpp

@@ -168,6 +168,10 @@ struct crashreporter_annotations_t gCRAnnotations
 static void
 reportOnCrash(uint32_t flags, const char *message)
 {
+  // We must use an "unsafe" mutex in this pathway since the normal "safe"
+  // mutex calls fatalError when an error is detected and fatalError ends up
+  // calling us. In other words we could get infinite recursion if the
+  // mutex errors.
   static StaticUnsafeMutex crashlogLock();
 
   crashlogLock.lock();

stdlib/public/runtime/MetadataCache.h

@@ -337,7 +337,7 @@ template <class ValueTy> class MetadataCache {
       // appear there.  Note that we have to check again immediately
       // after acquiring the lock to prevent a race.
       auto concurrency = Concurrency.get();
-      concurrency->Lock.lockOrWait(concurrency->Queue, [&value, &entry, this] {
+      concurrency->Lock.withLockOrWait(concurrency->Queue, [&, this] {
         if ((value = entry->getValue())) {
           return true; // found a value, done waiting
         }
@@ -373,9 +373,8 @@ template <class ValueTy> class MetadataCache {
 
     // Acquire the lock, set the value, and notify any waiters.
     auto concurrency = Concurrency.get();
-    concurrency->Lock.lockAndNotifyAll(concurrency->Queue, [&entry, &value] {
-      entry->setValue(value);
-    });
+    concurrency->Lock.withLockThenNotifyAll(
+        concurrency->Queue, [&entry, &value] { entry->setValue(value); });
 
     return value;
   }

stdlib/public/runtime/MetadataLookup.cpp

@@ -263,7 +263,7 @@ _typeByMangledName(const llvm::StringRef typeName) {
     return Value->getMetadata();
 
   // Check type metadata records
-  T.SectionsToScanLock.lock([&] {
+  T.SectionsToScanLock.withLock([&] {
     foundMetadata = _searchTypeMetadataRecords(T, typeName);
   });