diff options
| author | Matt Peters <[email protected]> | 2021-12-03 08:22:13 -0700 |
|---|---|---|
| committer | Matt Peters <[email protected]> | 2021-12-03 08:22:13 -0700 |
| commit | 0d8d8b09f925bd8bd0183c647e21e209ed71284b (patch) | |
| tree | 23dac3bb52fdd3311f61370702c6e0dfe6527a48 | |
| parent | Make IoBufferCore::Materialize threadsafe (diff) | |
| download | zen-0d8d8b09f925bd8bd0183c647e21e209ed71284b.tar.xz zen-0d8d8b09f925bd8bd0183c647e21e209ed71284b.zip | |
Fixes from code review
| -rw-r--r-- | zencore/include/zencore/iobuffer.h | 9 | ||||
| -rw-r--r-- | zencore/iobuffer.cpp | 12 |
2 files changed, 11 insertions, 10 deletions
diff --git a/zencore/include/zencore/iobuffer.h b/zencore/include/zencore/iobuffer.h index a39f442e3..9d18a55e9 100644 --- a/zencore/include/zencore/iobuffer.h +++ b/zencore/include/zencore/iobuffer.h @@ -112,7 +112,7 @@ public: inline void EnsureDataValid() const { - uint32_t LocalFlags = m_Flags.load(std::memory_order_acquire); + const uint32_t LocalFlags = m_Flags.load(std::memory_order_acquire); if ((LocalFlags & kIsExtended) && !(LocalFlags & kIsMaterialized)) { Materialize(); @@ -193,12 +193,11 @@ public: { ZEN_ASSERT_SLOW((uint32_t(ContentType) & kContentTypeMask) == uint32_t(ContentType)); uint32_t OldValue = m_Flags.load(std::memory_order_relaxed); - bool bSucceeded; + uint32_t NewValue; do { - uint32_t NewValue = (OldValue & ~(kContentTypeMask << kContentTypeShift)) | (uint32_t(ContentType) << kContentTypeShift); - bSucceeded = m_Flags.compare_exchange_weak(OldValue, NewValue, std::memory_order_relaxed, std::memory_order_relaxed); - } while (!bSucceeded); + NewValue = (OldValue & ~(kContentTypeMask << kContentTypeShift)) | (uint32_t(ContentType) << kContentTypeShift); + } while (!m_Flags.compare_exchange_weak(OldValue, NewValue, std::memory_order_relaxed, std::memory_order_relaxed)); } inline ZenContentType GetContentType() const diff --git a/zencore/iobuffer.cpp b/zencore/iobuffer.cpp index 9a7c6b30e..979772d5e 100644 --- a/zencore/iobuffer.cpp +++ b/zencore/iobuffer.cpp @@ -66,7 +66,7 @@ IoBufferCore::FreeBuffer() return; } - uint32_t LocalFlags = m_Flags.load(std::memory_order_relaxed); + const uint32_t LocalFlags = m_Flags.load(std::memory_order_relaxed); #if ZEN_PLATFORM_WINDOWS if (LocalFlags & kLowLevelAlloc) { @@ -171,12 +171,13 @@ IoBufferExtendedCore::IoBufferExtendedCore(void* FileHandle, uint64_t Offset, ui , m_FileHandle(FileHandle) , m_FileOffset(Offset) { - m_Flags.fetch_or(kIsOwnedByThis | kIsExtended, std::memory_order_relaxed); + uint32_t NewFlags = kIsOwnedByThis | kIsExtended; if (TransferHandleOwnership) { - m_Flags.fetch_or(kOwnsFile, std::memory_order_relaxed); + NewFlags |= kOwnsFile; } + m_Flags.fetch_or(NewFlags, std::memory_order_relaxed); } IoBufferExtendedCore::IoBufferExtendedCore(const IoBufferExtendedCore* Outer, uint64_t Offset, uint64_t Size) @@ -199,7 +200,7 @@ IoBufferExtendedCore::~IoBufferExtendedCore() #endif } - uint32_t LocalFlags = m_Flags.load(std::memory_order_relaxed); + const uint32_t LocalFlags = m_Flags.load(std::memory_order_relaxed); #if ZEN_PLATFORM_WINDOWS if (LocalFlags & kOwnsMmap) { @@ -241,7 +242,8 @@ IoBufferExtendedCore::Materialize() const RwLock::ExclusiveLockScope _(g_MappingLock); // Someone could have gotten here first - if (m_Flags.load(std::memory_order_acquire) & kIsMaterialized) + // We can use memory_order_relaxed on this load because the mutex has already provided the fence + if (m_Flags.load(std::memory_order_relaxed) & kIsMaterialized) return; uint32_t NewFlags = kIsMaterialized; |