[Mlir-commits] [mlir] 58acda1 - Revert "[mlir] Add a utility class, ThreadLocalCache, for storing non static thread local objects."
Mehdi Amini
llvmlistbot at llvm.org
Fri Aug 7 22:33:22 PDT 2020
Author: Mehdi Amini
Date: 2020-08-08T05:31:25Z
New Revision: 58acda1c16a9359c0b8c37e17a2df58c56250ed0
URL: https://github.com/llvm/llvm-project/commit/58acda1c16a9359c0b8c37e17a2df58c56250ed0
DIFF: https://github.com/llvm/llvm-project/commit/58acda1c16a9359c0b8c37e17a2df58c56250ed0.diff
LOG: Revert "[mlir] Add a utility class, ThreadLocalCache, for storing non static thread local objects."
This reverts commit 9f24640b7e6e61b0f293c724155a90a5e446dd7a.
We hit some dead-locks on thread exit in some configurations: TLS exit handler is taking a lock.
Temporarily reverting this change as we're debugging what is going on.
Added:
Modified:
mlir/lib/IR/MLIRContext.cpp
mlir/lib/Support/StorageUniquer.cpp
mlir/test/EDSC/builder-api-test.cpp
Removed:
mlir/include/mlir/Support/ThreadLocalCache.h
################################################################################
diff --git a/mlir/include/mlir/Support/ThreadLocalCache.h b/mlir/include/mlir/Support/ThreadLocalCache.h
deleted file mode 100644
index 3b5d6f0f424f..000000000000
--- a/mlir/include/mlir/Support/ThreadLocalCache.h
+++ /dev/null
@@ -1,117 +0,0 @@
-//===- ThreadLocalCache.h - ThreadLocalCache class --------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file contains a definition of the ThreadLocalCache class. This class
-// provides support for defining thread local objects with non-static duration.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef MLIR_SUPPORT_THREADLOCALCACHE_H
-#define MLIR_SUPPORT_THREADLOCALCACHE_H
-
-#include "mlir/Support/LLVM.h"
-#include "llvm/ADT/DenseMap.h"
-#include "llvm/Support/ManagedStatic.h"
-#include "llvm/Support/Mutex.h"
-#include "llvm/Support/ThreadLocal.h"
-
-namespace mlir {
-/// This class provides support for defining a thread local object with non
-/// static storage duration. This is very useful for situations in which a data
-/// cache has very large lock contention.
-template <typename ValueT>
-class ThreadLocalCache {
- /// The type used for the static thread_local cache. This is a map between an
- /// instance of the non-static cache and a weak reference to an instance of
- /// ValueT. We use a weak reference here so that the object can be destroyed
- /// without needing to lock access to the cache itself.
- struct CacheType : public llvm::SmallDenseMap<ThreadLocalCache<ValueT> *,
- std::weak_ptr<ValueT>> {
- ~CacheType() {
- // Remove the values of this cache that haven't already expired.
- for (auto &it : *this)
- if (std::shared_ptr<ValueT> value = it.second.lock())
- it.first->remove(value.get());
- }
-
- /// Clear out any unused entries within the map. This method is not
- /// thread-safe, and should only be called by the same thread as the cache.
- void clearExpiredEntries() {
- for (auto it = this->begin(), e = this->end(); it != e;) {
- auto curIt = it++;
- if (curIt->second.expired())
- this->erase(curIt);
- }
- }
- };
-
-public:
- ThreadLocalCache() = default;
- ~ThreadLocalCache() {
- // No cleanup is necessary here as the shared_pointer memory will go out of
- // scope and invalidate the weak pointers held by the thread_local caches.
- }
-
- /// Return an instance of the value type for the current thread.
- ValueT &get() {
- // Check for an already existing instance for this thread.
- CacheType &staticCache = getStaticCache();
- std::weak_ptr<ValueT> &threadInstance = staticCache[this];
- if (std::shared_ptr<ValueT> value = threadInstance.lock())
- return *value;
-
- // Otherwise, create a new instance for this thread.
- llvm::sys::SmartScopedLock<true> threadInstanceLock(instanceMutex);
- instances.push_back(std::make_shared<ValueT>());
- std::shared_ptr<ValueT> &instance = instances.back();
- threadInstance = instance;
-
- // Before returning the new instance, take the chance to clear out any used
- // entries in the static map. The cache is only cleared within the same
- // thread to remove the need to lock the cache itself.
- staticCache.clearExpiredEntries();
- return *instance;
- }
- ValueT &operator*() { return get(); }
- ValueT *operator->() { return &get(); }
-
-private:
- ThreadLocalCache(ThreadLocalCache &&) = delete;
- ThreadLocalCache(const ThreadLocalCache &) = delete;
- ThreadLocalCache &operator=(const ThreadLocalCache &) = delete;
-
- /// Return the static thread local instance of the cache type.
- static CacheType &getStaticCache() {
- static LLVM_THREAD_LOCAL CacheType cache;
- return cache;
- }
-
- /// Remove the given value entry. This is generally called when a thread local
- /// cache is destructing.
- void remove(ValueT *value) {
- // Erase the found value directly, because it is guaranteed to be in the
- // list.
- llvm::sys::SmartScopedLock<true> threadInstanceLock(instanceMutex);
- auto it = llvm::find_if(instances, [&](std::shared_ptr<ValueT> &instance) {
- return instance.get() == value;
- });
- assert(it != instances.end() && "expected value to exist in cache");
- instances.erase(it);
- }
-
- /// Owning pointers to all of the values that have been constructed for this
- /// object in the static cache.
- SmallVector<std::shared_ptr<ValueT>, 1> instances;
-
- /// A mutex used when a new thread instance has been added to the cache for
- /// this object.
- llvm::sys::SmartMutex<true> instanceMutex;
-};
-} // end namespace mlir
-
-#endif // MLIR_SUPPORT_THREADLOCALCACHE_H
diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index ace355319659..ff6c28bb7c4d 100644
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -24,7 +24,6 @@
#include "mlir/IR/Location.h"
#include "mlir/IR/Module.h"
#include "mlir/IR/Types.h"
-#include "mlir/Support/ThreadLocalCache.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/SetVector.h"
@@ -281,12 +280,8 @@ class MLIRContextImpl {
/// operations.
llvm::StringMap<AbstractOperation> registeredOperations;
- /// Identifers are uniqued by string value and use the internal string set for
- /// storage.
+ /// These are identifiers uniqued into this MLIRContext.
llvm::StringSet<llvm::BumpPtrAllocator &> identifiers;
- /// A thread local cache of identifiers to reduce lock contention.
- ThreadLocalCache<llvm::StringMap<llvm::StringMapEntry<llvm::NoneType> *>>
- localIdentifierCache;
/// An allocator used for AbstractAttribute and AbstractType objects.
llvm::BumpPtrAllocator abstractDialectSymbolAllocator;
@@ -634,37 +629,27 @@ const AbstractType &AbstractType::lookup(TypeID typeID, MLIRContext *context) {
/// Return an identifier for the specified string.
Identifier Identifier::get(StringRef str, MLIRContext *context) {
- // Check invariants after seeing if we already have something in the
- // identifier table - if we already had it in the table, then it already
- // passed invariant checks.
- assert(!str.empty() && "Cannot create an empty identifier");
- assert(str.find('\0') == StringRef::npos &&
- "Cannot create an identifier with a nul character");
-
auto &impl = context->getImpl();
- if (!context->isMultithreadingEnabled())
- return Identifier(&*impl.identifiers.insert(str).first);
-
- // Check for an existing instance in the local cache.
- auto *&localEntry = (*impl.localIdentifierCache)[str];
- if (localEntry)
- return Identifier(localEntry);
// Check for an existing identifier in read-only mode.
if (context->isMultithreadingEnabled()) {
llvm::sys::SmartScopedReader<true> contextLock(impl.identifierMutex);
auto it = impl.identifiers.find(str);
- if (it != impl.identifiers.end()) {
- localEntry = &*it;
- return Identifier(localEntry);
- }
+ if (it != impl.identifiers.end())
+ return Identifier(&*it);
}
+ // Check invariants after seeing if we already have something in the
+ // identifier table - if we already had it in the table, then it already
+ // passed invariant checks.
+ assert(!str.empty() && "Cannot create an empty identifier");
+ assert(str.find('\0') == StringRef::npos &&
+ "Cannot create an identifier with a nul character");
+
// Acquire a writer-lock so that we can safely create the new instance.
- llvm::sys::SmartScopedWriter<true> contextLock(impl.identifierMutex);
+ ScopedWriterLock contextLock(impl.identifierMutex, impl.threadingIsEnabled);
auto it = impl.identifiers.insert(str).first;
- localEntry = &*it;
- return Identifier(localEntry);
+ return Identifier(&*it);
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Support/StorageUniquer.cpp b/mlir/lib/Support/StorageUniquer.cpp
index 3fb6d3733b23..49e7272091fb 100644
--- a/mlir/lib/Support/StorageUniquer.cpp
+++ b/mlir/lib/Support/StorageUniquer.cpp
@@ -9,7 +9,6 @@
#include "mlir/Support/StorageUniquer.h"
#include "mlir/Support/LLVM.h"
-#include "mlir/Support/ThreadLocalCache.h"
#include "mlir/Support/TypeID.h"
#include "llvm/Support/RWMutex.h"
@@ -40,8 +39,6 @@ struct InstSpecificUniquer {
/// A utility wrapper object representing a hashed storage object. This class
/// contains a storage object and an existing computed hash value.
struct HashedStorage {
- HashedStorage(unsigned hashValue = 0, BaseStorage *storage = nullptr)
- : hashValue(hashValue), storage(storage) {}
unsigned hashValue;
BaseStorage *storage;
};
@@ -49,10 +46,10 @@ struct InstSpecificUniquer {
/// Storage info for derived TypeStorage objects.
struct StorageKeyInfo : DenseMapInfo<HashedStorage> {
static HashedStorage getEmptyKey() {
- return HashedStorage(0, DenseMapInfo<BaseStorage *>::getEmptyKey());
+ return HashedStorage{0, DenseMapInfo<BaseStorage *>::getEmptyKey()};
}
static HashedStorage getTombstoneKey() {
- return HashedStorage(0, DenseMapInfo<BaseStorage *>::getTombstoneKey());
+ return HashedStorage{0, DenseMapInfo<BaseStorage *>::getTombstoneKey()};
}
static unsigned getHashValue(const HashedStorage &key) {
@@ -105,34 +102,25 @@ struct StorageUniquerImpl {
if (!threadingIsEnabled)
return getOrCreateUnsafe(storageUniquer, kind, lookupKey, ctorFn);
- // Check for a instance of this object in the local cache.
- auto localIt = complexStorageLocalCache->insert_as(
- InstSpecificUniquer::HashedStorage(lookupKey.hashValue), lookupKey);
- BaseStorage *&localInst = localIt.first->storage;
- if (localInst)
- return localInst;
-
// Check for an existing instance in read-only mode.
{
llvm::sys::SmartScopedReader<true> typeLock(storageUniquer.mutex);
auto it = storageUniquer.complexInstances.find_as(lookupKey);
if (it != storageUniquer.complexInstances.end())
- return localInst = it->storage;
+ return it->storage;
}
// Acquire a writer-lock so that we can safely create the new type instance.
llvm::sys::SmartScopedWriter<true> typeLock(storageUniquer.mutex);
- return localInst =
- getOrCreateUnsafe(storageUniquer, kind, lookupKey, ctorFn);
+ return getOrCreateUnsafe(storageUniquer, kind, lookupKey, ctorFn);
}
/// Get or create an instance of a complex derived type in an thread-unsafe
/// fashion.
BaseStorage *
getOrCreateUnsafe(InstSpecificUniquer &storageUniquer, unsigned kind,
- InstSpecificUniquer::LookupKey &key,
+ InstSpecificUniquer::LookupKey &lookupKey,
function_ref<BaseStorage *(StorageAllocator &)> ctorFn) {
- auto existing =
- storageUniquer.complexInstances.insert_as({key.hashValue}, key);
+ auto existing = storageUniquer.complexInstances.insert_as({}, lookupKey);
if (!existing.second)
return existing.first->storage;
@@ -140,7 +128,9 @@ struct StorageUniquerImpl {
// instance.
BaseStorage *storage =
initializeStorage(kind, storageUniquer.allocator, ctorFn);
- return existing.first->storage = storage;
+ *existing.first =
+ InstSpecificUniquer::HashedStorage{lookupKey.hashValue, storage};
+ return storage;
}
/// Get or create an instance of a simple derived type.
@@ -152,11 +142,6 @@ struct StorageUniquerImpl {
if (!threadingIsEnabled)
return getOrCreateUnsafe(storageUniquer, kind, ctorFn);
- // Check for a instance of this object in the local cache.
- BaseStorage *&localInst = (*simpleStorageLocalCache)[kind];
- if (localInst)
- return localInst;
-
// Check for an existing instance in read-only mode.
{
llvm::sys::SmartScopedReader<true> typeLock(storageUniquer.mutex);
@@ -167,7 +152,7 @@ struct StorageUniquerImpl {
// Acquire a writer-lock so that we can safely create the new type instance.
llvm::sys::SmartScopedWriter<true> typeLock(storageUniquer.mutex);
- return localInst = getOrCreateUnsafe(storageUniquer, kind, ctorFn);
+ return getOrCreateUnsafe(storageUniquer, kind, ctorFn);
}
/// Get or create an instance of a simple derived type in an thread-unsafe
/// fashion.
@@ -230,12 +215,6 @@ struct StorageUniquerImpl {
/// Map of type ids to the storage uniquer to use for registered objects.
DenseMap<TypeID, std::unique_ptr<InstSpecificUniquer>> instUniquers;
- /// A thread local cache for simple and complex storage objects. This helps to
- /// reduce the lock contention when an object already existing in the cache.
- ThreadLocalCache<DenseMap<unsigned, BaseStorage *>> simpleStorageLocalCache;
- ThreadLocalCache<InstSpecificUniquer::StorageTypeSet>
- complexStorageLocalCache;
-
/// Flag specifying if multi-threading is enabled within the uniquer.
bool threadingIsEnabled = true;
};
diff --git a/mlir/test/EDSC/builder-api-test.cpp b/mlir/test/EDSC/builder-api-test.cpp
index b620062e2238..3fcfcf24ef8f 100644
--- a/mlir/test/EDSC/builder-api-test.cpp
+++ b/mlir/test/EDSC/builder-api-test.cpp
@@ -6,7 +6,7 @@
//
//===----------------------------------------------------------------------===//
-// RUN: mlir-edsc-builder-api-test
+// RUN: mlir-edsc-builder-api-test | FileCheck %s
#include "mlir/Dialect/Affine/EDSC/Intrinsics.h"
#include "mlir/Dialect/Linalg/EDSC/Builders.h"
More information about the Mlir-commits
mailing list