Add explanation of why RAW_PTR_EXCLUSIONs are safe.
Also remove one of them by using unique_ptr instead, requiring minor
changes to CommandAllocator/Iterator.
Bug: 335556942
Change-Id: Iab76d12b3cb8c759ab8acf6d41ef1733339f95e4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/188040
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/common/LinkedList.h b/src/dawn/common/LinkedList.h
index ff89d47..16b9226 100644
--- a/src/dawn/common/LinkedList.h
+++ b/src/dawn/common/LinkedList.h
@@ -172,7 +172,9 @@
private:
friend class LinkedList<T>;
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
+ // RAW_PTR_EXCLUSION: Linked lists are used and iterated a lot so these pointers are very hot.
+ // All accesses to the pointers are behind "safe" APIs such that it is not possible (in
+ // single-threaded code) to use them after they are freed.
RAW_PTR_EXCLUSION LinkNode<T>* previous_ = nullptr;
RAW_PTR_EXCLUSION LinkNode<T>* next_ = nullptr;
};
@@ -235,7 +237,9 @@
LinkNode<T>* operator*() const { return current_; }
private:
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
+ // RAW_PTR_EXCLUSION: Linked lists are used and iterated a lot so these pointers are very hot.
+ // All accesses to the pointers are behind "safe" APIs such that it is not possible (in
+ // single-threaded code) to use them after they are freed.
RAW_PTR_EXCLUSION LinkNode<T>* current_ = nullptr;
RAW_PTR_EXCLUSION LinkNode<T>* next_ = nullptr;
};
diff --git a/src/dawn/common/MutexProtected.h b/src/dawn/common/MutexProtected.h
index c08e3a1..ab2e6fc 100644
--- a/src/dawn/common/MutexProtected.h
+++ b/src/dawn/common/MutexProtected.h
@@ -32,7 +32,9 @@
#include <utility>
#include "dawn/common/Mutex.h"
+#include "dawn/common/NonMovable.h"
#include "dawn/common/Ref.h"
+#include "dawn/common/StackAllocated.h"
#include "partition_alloc/pointers/raw_ptr_exclusion.h"
namespace dawn {
@@ -69,7 +71,7 @@
// Guard class is a wrapping class that gives access to a protected resource after acquiring the
// lock related to it. For the lifetime of this class, the lock is held.
template <typename T, typename Traits>
-class Guard {
+class Guard : public NonMovable, StackAllocated {
public:
// It's the programmer's burden to not save the pointer/reference and reuse it without the lock.
auto* operator->() const { return Get(); }
@@ -92,7 +94,9 @@
friend class MutexProtected<NonConstT, Guard>;
typename Traits::LockType mLock;
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
+ // RAW_PTR_EXCLUSION: This pointer is created/destroyed on each access to a MutexProtected.
+ // The pointer is always transiently used while the MutexProtected is in scope so it is
+ // unlikely to be used after it is freed.
RAW_PTR_EXCLUSION T* mObj = nullptr;
};
diff --git a/src/dawn/native/BindGroupTracker.h b/src/dawn/native/BindGroupTracker.h
index 7b95af1..318c979 100644
--- a/src/dawn/native/BindGroupTracker.h
+++ b/src/dawn/native/BindGroupTracker.h
@@ -126,7 +126,9 @@
// |mPipelineLayout| is the current pipeline layout set on the command buffer.
// |mLastAppliedPipelineLayout| is the last pipeline layout for which we applied changes
// to the bind group bindings.
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
+ // RAW_PTR_EXCLUSION: These pointers are very hot in command recording code and point at
+ // pipeline layouts referenced by the object graph of the CommandBuffer so they cannot be
+ // freed from underneath this class.
RAW_PTR_EXCLUSION PipelineLayoutBase* mPipelineLayout = nullptr;
RAW_PTR_EXCLUSION PipelineLayoutBase* mLastAppliedPipelineLayout = nullptr;
};
diff --git a/src/dawn/native/CommandAllocator.cpp b/src/dawn/native/CommandAllocator.cpp
index f5d0cda..1fe51ee 100644
--- a/src/dawn/native/CommandAllocator.cpp
+++ b/src/dawn/native/CommandAllocator.cpp
@@ -91,7 +91,7 @@
*commandId = detail::kEndOfBlock;
return false;
}
- mCurrentPtr = AlignPtr(mBlocks[mCurrentBlock].block, alignof(uint32_t));
+ mCurrentPtr = AlignPtr(mBlocks[mCurrentBlock].block.get(), alignof(uint32_t));
return NextCommandId(commandId);
}
@@ -101,12 +101,9 @@
if (mBlocks.empty()) {
// This will case the first NextCommandId call to try to move to the next block and stop
// the iteration immediately, without special casing the initialization.
- mCurrentPtr = reinterpret_cast<uint8_t*>(&mEndOfBlock);
- mBlocks.emplace_back();
- mBlocks[0].size = sizeof(mEndOfBlock);
- mBlocks[0].block = mCurrentPtr;
+ mCurrentPtr = reinterpret_cast<char*>(&mEndOfBlock);
} else {
- mCurrentPtr = AlignPtr(mBlocks[0].block, alignof(uint32_t));
+ mCurrentPtr = AlignPtr(mBlocks[0].block.get(), alignof(uint32_t));
}
}
@@ -115,17 +112,14 @@
return;
}
- mCurrentPtr = reinterpret_cast<uint8_t*>(&mEndOfBlock);
- for (BlockDef& block : mBlocks) {
- free(block.block);
- }
+ mCurrentPtr = reinterpret_cast<char*>(&mEndOfBlock);
mBlocks.clear();
Reset();
DAWN_ASSERT(IsEmpty());
}
bool CommandIterator::IsEmpty() const {
- return mBlocks[0].block == reinterpret_cast<const uint8_t*>(&mEndOfBlock);
+ return mBlocks.empty();
}
// Potential TODO(crbug.com/dawn/835):
@@ -172,15 +166,12 @@
void CommandAllocator::Reset() {
ResetPointers();
- for (BlockDef& block : mBlocks) {
- free(block.block);
- }
mBlocks.clear();
mLastAllocationSize = kDefaultBaseAllocationSize;
}
bool CommandAllocator::IsEmpty() const {
- return mCurrentPtr == reinterpret_cast<const uint8_t*>(&mPlaceholderEnum[0]);
+ return mCurrentPtr == reinterpret_cast<const char*>(&mPlaceholderSpace[0]);
}
CommandBlocks&& CommandAllocator::AcquireBlocks() {
@@ -194,9 +185,9 @@
return std::move(mBlocks);
}
-uint8_t* CommandAllocator::AllocateInNewBlock(uint32_t commandId,
- size_t commandSize,
- size_t commandAlignment) {
+char* CommandAllocator::AllocateInNewBlock(uint32_t commandId,
+ size_t commandSize,
+ size_t commandAlignment) {
// When there is not enough space, we signal the kEndOfBlock, so that the iterator knows
// to move to the next one. kEndOfBlock on the last block means the end of the commands.
uint32_t* idAlloc = reinterpret_cast<uint32_t*>(mCurrentPtr);
@@ -221,20 +212,20 @@
// Allocate blocks doubling sizes each time, to a maximum of 16k (or at least minimumSize).
mLastAllocationSize = std::max(minimumSize, std::min(mLastAllocationSize * 2, size_t(16384)));
- uint8_t* block = static_cast<uint8_t*>(malloc(mLastAllocationSize));
+ auto block = std::unique_ptr<char[]>(new (std::nothrow) char[mLastAllocationSize]);
if (DAWN_UNLIKELY(block == nullptr)) {
return false;
}
- mBlocks.push_back({mLastAllocationSize, block});
- mCurrentPtr = AlignPtr(block, alignof(uint32_t));
- mEndPtr = block + mLastAllocationSize;
+ mCurrentPtr = AlignPtr(block.get(), alignof(uint32_t));
+ mEndPtr = block.get() + mLastAllocationSize;
+ mBlocks.push_back({mLastAllocationSize, std::move(block)});
return true;
}
void CommandAllocator::ResetPointers() {
- mCurrentPtr = reinterpret_cast<uint8_t*>(&mPlaceholderEnum[0]);
- mEndPtr = reinterpret_cast<uint8_t*>(&mPlaceholderEnum[1]);
+ mCurrentPtr = reinterpret_cast<char*>(&mPlaceholderSpace[0]);
+ mEndPtr = reinterpret_cast<char*>(&mPlaceholderSpace[1]);
}
} // namespace dawn::native
diff --git a/src/dawn/native/CommandAllocator.h b/src/dawn/native/CommandAllocator.h
index 1d9ddf1..9ad0db7 100644
--- a/src/dawn/native/CommandAllocator.h
+++ b/src/dawn/native/CommandAllocator.h
@@ -31,6 +31,7 @@
#include <cstddef>
#include <cstdint>
#include <limits>
+#include <memory>
#include <vector>
#include "dawn/common/Assert.h"
@@ -71,8 +72,7 @@
// and CommandIterator
struct BlockDef {
size_t size;
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
- RAW_PTR_EXCLUSION uint8_t* block = nullptr;
+ std::unique_ptr<char[]> block;
};
using CommandBlocks = std::vector<BlockDef>;
@@ -121,9 +121,10 @@
bool IsEmpty() const;
DAWN_FORCE_INLINE bool NextCommandId(uint32_t* commandId) {
- uint8_t* idPtr = AlignPtr(mCurrentPtr, alignof(uint32_t));
- DAWN_ASSERT(idPtr + sizeof(uint32_t) <=
- mBlocks[mCurrentBlock].block + mBlocks[mCurrentBlock].size);
+ char* idPtr = AlignPtr(mCurrentPtr, alignof(uint32_t));
+ DAWN_ASSERT(idPtr == reinterpret_cast<char*>(&mEndOfBlock) ||
+ idPtr + sizeof(uint32_t) <=
+ mBlocks[mCurrentBlock].block.get() + mBlocks[mCurrentBlock].size);
uint32_t id = *reinterpret_cast<uint32_t*>(idPtr);
@@ -138,9 +139,9 @@
bool NextCommandIdInNewBlock(uint32_t* commandId);
DAWN_FORCE_INLINE void* NextCommand(size_t commandSize, size_t commandAlignment) {
- uint8_t* commandPtr = AlignPtr(mCurrentPtr, commandAlignment);
+ char* commandPtr = AlignPtr(mCurrentPtr, commandAlignment);
DAWN_ASSERT(commandPtr + sizeof(commandSize) <=
- mBlocks[mCurrentBlock].block + mBlocks[mCurrentBlock].size);
+ mBlocks[mCurrentBlock].block.get() + mBlocks[mCurrentBlock].size);
mCurrentPtr = commandPtr + commandSize;
return commandPtr;
@@ -156,8 +157,9 @@
}
CommandBlocks mBlocks;
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
- RAW_PTR_EXCLUSION uint8_t* mCurrentPtr = nullptr;
+ // RAW_PTR_EXCLUSION: This is an extremely hot pointer during command iteration, but always
+ // points to at least a valid uint32_t, either inside a block, or at mEndOfBlock.
+ RAW_PTR_EXCLUSION char* mCurrentPtr = nullptr;
size_t mCurrentBlock = 0;
// Used to avoid a special case for empty iterators.
uint32_t mEndOfBlock = detail::kEndOfBlock;
@@ -220,9 +222,9 @@
friend CommandIterator;
CommandBlocks&& AcquireBlocks();
- DAWN_FORCE_INLINE uint8_t* Allocate(uint32_t commandId,
- size_t commandSize,
- size_t commandAlignment) {
+ DAWN_FORCE_INLINE char* Allocate(uint32_t commandId,
+ size_t commandSize,
+ size_t commandAlignment) {
DAWN_ASSERT(mCurrentPtr != nullptr);
DAWN_ASSERT(mEndPtr != nullptr);
DAWN_ASSERT(commandId != detail::kEndOfBlock);
@@ -250,7 +252,7 @@
uint32_t* idAlloc = reinterpret_cast<uint32_t*>(mCurrentPtr);
*idAlloc = commandId;
- uint8_t* commandAlloc = AlignPtr(mCurrentPtr + sizeof(uint32_t), commandAlignment);
+ char* commandAlloc = AlignPtr(mCurrentPtr + sizeof(uint32_t), commandAlignment);
mCurrentPtr = AlignPtr(commandAlloc + commandSize, alignof(uint32_t));
return commandAlloc;
@@ -258,9 +260,9 @@
return AllocateInNewBlock(commandId, commandSize, commandAlignment);
}
- uint8_t* AllocateInNewBlock(uint32_t commandId, size_t commandSize, size_t commandAlignment);
+ char* AllocateInNewBlock(uint32_t commandId, size_t commandSize, size_t commandAlignment);
- DAWN_FORCE_INLINE uint8_t* AllocateData(size_t commandSize, size_t commandAlignment) {
+ DAWN_FORCE_INLINE char* AllocateData(size_t commandSize, size_t commandAlignment) {
return Allocate(detail::kAdditionalData, commandSize, commandAlignment);
}
@@ -274,14 +276,15 @@
// Data used for the block range at initialization so that the first call to Allocate sees
// there is not enough space and calls GetNewBlock. This avoids having to special case the
// initialization in Allocate.
- uint32_t mPlaceholderEnum[1] = {0};
+ uint32_t mPlaceholderSpace[1] = {0};
// Pointers to the current range of allocation in the block. Guaranteed to allow for at
// least one uint32_t if not nullptr, so that the special kEndOfBlock command id can always
// be written. Nullptr iff the blocks were moved out.
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
- RAW_PTR_EXCLUSION uint8_t* mCurrentPtr = nullptr;
- RAW_PTR_EXCLUSION uint8_t* mEndPtr = nullptr;
+ // RAW_PTR_EXCLUSION: These are extremely hot pointers during command allocation, but always
+ // set to a valid slice (either the placeholder space, or a real allocated block).
+ RAW_PTR_EXCLUSION char* mCurrentPtr = nullptr;
+ RAW_PTR_EXCLUSION char* mEndPtr = nullptr;
};
} // namespace dawn::native
diff --git a/src/dawn/native/CommandBufferStateTracker.h b/src/dawn/native/CommandBufferStateTracker.h
index 78232b4..8476184 100644
--- a/src/dawn/native/CommandBufferStateTracker.h
+++ b/src/dawn/native/CommandBufferStateTracker.h
@@ -95,10 +95,6 @@
ValidationAspects mAspects;
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
- RAW_PTR_EXCLUSION PerBindGroup<BindGroupBase*> mBindgroups = {};
- PerBindGroup<std::vector<uint32_t>> mDynamicOffsets = {};
-
VertexBufferMask mVertexBuffersUsed;
PerVertexBuffer<uint64_t> mVertexBufferSizes = {};
@@ -107,7 +103,12 @@
uint64_t mIndexBufferSize = 0;
uint64_t mIndexBufferOffset = 0;
- // RAW_PTR_EXCLUSION: Performance reasons (based on analysis of MotionMark).
+ // RAW_PTR_EXCLUSION: These pointers are very hot in command recording code and point at
+ // various objects referenced by the object graph of the CommandBuffer so they cannot be
+ // freed from underneath this class.
+ RAW_PTR_EXCLUSION PerBindGroup<BindGroupBase*> mBindgroups = {};
+ PerBindGroup<std::vector<uint32_t>> mDynamicOffsets = {};
+
RAW_PTR_EXCLUSION PipelineLayoutBase* mLastPipelineLayout = nullptr;
RAW_PTR_EXCLUSION PipelineBase* mLastPipeline = nullptr;
RAW_PTR_EXCLUSION const RequiredBufferSizes* mMinBufferSizes = nullptr;
