Skip to content

Commit

Permalink
Merge branch 'main' into pf/vk-update-command-buffer-count
Browse files Browse the repository at this point in the history
  • Loading branch information
@poweifeng
poweifeng authored Nov 14, 2024
2 parents 4f1b8bf + 05ffc76 commit 2ed1f2d
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 105 deletions.
2 changes: 1 addition & 1 deletion filament/backend/src/metal/MetalHandles.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ struct MetalTimerQuery : public HwTimerQuery {

struct Status {
std::atomic<bool> available {false};
uint64_t elapsed {0}; // only valid if available is true
std::atomic<uint64_t> elapsed {0}; // only valid if available is true
};

std::shared_ptr<Status> status;
Expand Down
140 changes: 67 additions & 73 deletions filament/backend/src/vulkan/VulkanCommands.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,56 +58,32 @@ VkCommandBuffer createCommandBuffer(VkDevice device, VkCommandPool pool) {

#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)
void VulkanGroupMarkers::push(std::string const& marker, Timestamp start) noexcept {
mMarkers.push_back(marker);

#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
mTimestamps.push_back(start.time_since_epoch().count() > 0.0
? start
: std::chrono::high_resolution_clock::now());
#endif
mMarkers.push_back({marker,
start.time_since_epoch().count() > 0.0
? start
: std::chrono::high_resolution_clock::now()});
}

std::pair<std::string, Timestamp> VulkanGroupMarkers::pop() noexcept {
auto const marker = mMarkers.back();
auto ret = mMarkers.back();
mMarkers.pop_back();

#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
auto const timestamp = mTimestamps.back();
mTimestamps.pop_back();
return std::make_pair(marker, timestamp);
#else
return std::make_pair(marker, Timestamp{});
#endif
return ret;
}

std::pair<std::string, Timestamp> VulkanGroupMarkers::pop_bottom() noexcept {
auto const marker = mMarkers.front();
auto ret = mMarkers.front();
mMarkers.pop_front();

#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
auto const timestamp = mTimestamps.front();
mTimestamps.pop_front();
return std::make_pair(marker, timestamp);
#else
return std::make_pair(marker, Timestamp{});
#endif
return ret;
}

std::pair<std::string, Timestamp> VulkanGroupMarkers::top() const {
std::pair<std::string, Timestamp> const& VulkanGroupMarkers::top() const {
assert_invariant(!empty());
auto const marker = mMarkers.back();
#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
auto const topTimestamp = mTimestamps.front();
return std::make_pair(marker, topTimestamp);
#else
return std::make_pair(marker, Timestamp{});
#endif
return mMarkers.back();
}

bool VulkanGroupMarkers::empty() const noexcept {
return mMarkers.empty();
}

#endif // FVK_DEBUG_GROUP_MARKERS

VulkanCommandBuffer::VulkanCommandBuffer(VulkanContext* context, VkDevice device, VkQueue queue,
Expand Down Expand Up @@ -302,6 +278,19 @@ VulkanCommandBuffer& CommandBufferPool::getRecording() {

auto& recording = *mBuffers[mRecording];
recording.begin();

#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)
if (mGroupMarkers) {
std::unique_ptr<VulkanGroupMarkers> markers = std::make_unique<VulkanGroupMarkers>();
while (!mGroupMarkers->empty()) {
auto [marker, timestamp] = mGroupMarkers->pop_bottom();
recording.pushMarker(marker.c_str());
markers->push(marker, timestamp);
}
std::swap(mGroupMarkers, markers);
}
#endif

return recording;
}

Expand Down Expand Up @@ -356,17 +345,38 @@ void CommandBufferPool::waitFor(VkSemaphore previousAction) {
recording->insertWait(previousAction);
}

void CommandBufferPool::pushMarker(char const* marker) {
#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)
std::string CommandBufferPool::topMarker() const {
if (!mGroupMarkers || mGroupMarkers->empty()) {
return "";
}
return std::get<0>(mGroupMarkers->top());
}

void CommandBufferPool::pushMarker(char const* marker, VulkanGroupMarkers::Timestamp timestamp) {
if (!mGroupMarkers) {
mGroupMarkers = std::make_unique<VulkanGroupMarkers>();
}
mGroupMarkers->push(marker, timestamp);
getRecording().pushMarker(marker);
}

void CommandBufferPool::popMarker() {
getRecording().popMarker();
std::pair<std::string, VulkanGroupMarkers::Timestamp> CommandBufferPool::popMarker() {
assert_invariant(mGroupMarkers && !mGroupMarkers->empty());
auto ret = mGroupMarkers->pop();

// Note that if we're popping a marker while not recording, we would just pop the conceptual
// stack of marker (i.e. mGroupMarkers) and not carry out the pop on the command buffer.
if (isRecording()) {
getRecording().popMarker();
}
return ret;
}

void CommandBufferPool::insertEvent(char const* marker) {
getRecording().insertEvent(marker);
}
#endif // FVK_DEBUG_GROUP_MARKERS

VulkanCommands::VulkanCommands(VkDevice device, VkQueue queue, uint32_t queueFamilyIndex,
VkQueue protectedQueue, uint32_t protectedQueueFamilyIndex, VulkanContext* context)
Expand Down Expand Up @@ -470,47 +480,31 @@ void VulkanCommands::updateFences() {
#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)

void VulkanCommands::pushGroupMarker(char const* str, VulkanGroupMarkers::Timestamp timestamp) {
#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
// If the timestamp is not 0, then we are carrying over a marker across buffer submits.
// If it is 0, then this is a normal marker push and we should just print debug line as usual.
if (timestamp.time_since_epoch().count() == 0.0) {
FVK_LOGD << "----> " << str << utils::io::endl;
}
#endif
if (!mGroupMarkers) {
mGroupMarkers = std::make_unique<VulkanGroupMarkers>();
}
mGroupMarkers->push(str, timestamp);

mPool->pushMarker(str);
mPool->pushMarker(str, timestamp);
if (mProtectedPool) {
mProtectedPool->pushMarker(str);
mProtectedPool->pushMarker(str, timestamp);
}
#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
FVK_LOGD << "----> " << str << utils::io::endl;
#endif
}

void VulkanCommands::popGroupMarker() {
assert_invariant(mGroupMarkers);

if (!mGroupMarkers->empty()) {
VkCommandBuffer const cmdbuffer = get().buffer();
#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
auto const [marker, startTime] = mGroupMarkers->pop();
auto const endTime = std::chrono::high_resolution_clock::now();
std::chrono::duration<double> diff = endTime - startTime;
FVK_LOGD << "<---- " << marker << " elapsed: " << (diff.count() * 1000) << " ms"
<< utils::io::endl;
auto ret = mPool->popMarker();
auto const& marker = ret.first;
auto const& startTime = ret.second;
auto const endTime = std::chrono::high_resolution_clock::now();
std::chrono::duration<double> diff = endTime - startTime;
FVK_LOGD << "<---- " << marker << " elapsed: " << (diff.count() * 1000) << " ms"
<< utils::io::endl;
#else
mGroupMarkers->pop();
#endif
mPool->popMarker();
if (mProtectedPool) {
mProtectedPool->popMarker();
}
} else if (mCarriedOverMarkers && !mCarriedOverMarkers->empty()) {
// It could be that pop is called between flush() and get() (new command buffer), in which
// case the marker is in "carried over" state, we'd just remove that. Since the
// mCarriedOverMarkers is in the opposite order, we pop the bottom instead of the top.
mCarriedOverMarkers->pop_bottom();
mPool->popMarker();
#endif // FVK_DEBUG_PRINT_GROUP_MARKERS

if (mProtectedPool) {
mProtectedPool->popMarker();
}
}

Expand All @@ -522,10 +516,10 @@ void VulkanCommands::insertEventMarker(char const* str, uint32_t len) {
}

std::string VulkanCommands::getTopGroupMarker() const {
if (!mGroupMarkers || mGroupMarkers->empty()) {
return "";
if (mProtectedPool) {
return mProtectedPool->topMarker();
}
return std::get<0>(mGroupMarkers->top());
return mPool->topMarker();
}
#endif // FVK_DEBUG_GROUP_MARKERS

Expand Down
26 changes: 12 additions & 14 deletions filament/backend/src/vulkan/VulkanCommands.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,11 @@ class VulkanGroupMarkers {
void push(std::string const& marker, Timestamp start = {}) noexcept;
std::pair<std::string, Timestamp> pop() noexcept;
std::pair<std::string, Timestamp> pop_bottom() noexcept;
std::pair<std::string, Timestamp> top() const;
std::pair<std::string, Timestamp> const& top() const;
bool empty() const noexcept;

private:
std::list<std::string> mMarkers;
#if FVK_ENABLED(FVK_DEBUG_PRINT_GROUP_MARKERS)
std::list<Timestamp> mTimestamps;
#endif
std::list<std::pair<std::string, Timestamp>> mMarkers;
};

#endif // FVK_DEBUG_GROUP_MARKERS
Expand Down Expand Up @@ -123,7 +120,7 @@ struct VulkanCommandBuffer {
VkSemaphore mSubmission;
VkFence mFence;
std::shared_ptr<VulkanCmdFence> mFenceStatus;
std::vector<fvkmemory::resource_ptr<Resource>> mResources;
std::vector<fvkmemory::resource_ptr<Resource>> mResources;
};

struct CommandBufferPool {
Expand All @@ -140,12 +137,14 @@ struct CommandBufferPool {
void update();
VkSemaphore flush();
void wait();

void waitFor(VkSemaphore previousAction);

void pushMarker(char const* marker);
void popMarker();
#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)
std::string topMarker() const;
void pushMarker(char const* marker, VulkanGroupMarkers::Timestamp timestamp);
std::pair<std::string, VulkanGroupMarkers::Timestamp> popMarker();
void insertEvent(char const* marker);
#endif

inline bool isRecording() const { return mRecording != INVALID; }

Expand All @@ -164,6 +163,10 @@ struct CommandBufferPool {
ActiveBuffers mSubmitted;
std::vector<std::unique_ptr<VulkanCommandBuffer>> mBuffers;
int8_t mRecording;

#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)
std::unique_ptr<VulkanGroupMarkers> mGroupMarkers;
#endif
};

// Manages a set of command buffers and semaphores, exposing an API that is significantly simpler
Expand Down Expand Up @@ -250,11 +253,6 @@ class VulkanCommands {

VkSemaphore mInjectedDependency = VK_NULL_HANDLE;
VkSemaphore mLastSubmit = VK_NULL_HANDLE;

#if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS)
std::unique_ptr<VulkanGroupMarkers> mGroupMarkers;
std::unique_ptr<VulkanGroupMarkers> mCarriedOverMarkers;
#endif
};

} // namespace filament::backend
Expand Down
36 changes: 20 additions & 16 deletions libs/utils/include/utils/Allocator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,18 +278,23 @@ class AtomicFreeList {
void* pop() noexcept {
Node* const pStorage = mStorage;

HeadPtr currentHead = mHead.load();
HeadPtr currentHead = mHead.load(std::memory_order_relaxed);
while (currentHead.offset >= 0) {
// The value of "pNext" we load here might already contain application data if another
// thread raced ahead of us. But in that case, the computed "newHead" will be discarded
// since compare_exchange_weak fails. Then this thread will loop with the updated
// since compare_exchange_weak() fails. Then this thread will loop with the updated
// value of currentHead, and try again.
Node* const pNext = pStorage[currentHead.offset].next.load(std::memory_order_relaxed);
// TSAN complains if we don't use a local variable here.
Node const node = pStorage[currentHead.offset];
Node const* const pNext = node.next;
const HeadPtr newHead{ pNext ? int32_t(pNext - pStorage) : -1, currentHead.tag + 1 };
// In the rare case that the other thread that raced ahead of us already returned the
// same mHead we just loaded, but it now has a different "next" value, the tag field will not
// match, and compare_exchange_weak will fail and prevent that particular race condition.
if (mHead.compare_exchange_weak(currentHead, newHead)) {
// In the rare case that the other thread that raced ahead of us already returned the
// same mHead we just loaded, but it now has a different "next" value, the tag field
// will not match, and compare_exchange_weak() will fail and prevent that particular
// race condition.
// acquire: no read/write can be reordered before this
if (mHead.compare_exchange_weak(currentHead, newHead,
std::memory_order_acquire, std::memory_order_relaxed)) {
// This assert needs to occur after we have validated that there was no race condition
// Otherwise, next might already contain application data, if another thread
// raced ahead of us after we loaded mHead, but before we loaded mHead->next.
Expand All @@ -306,24 +311,23 @@ class AtomicFreeList {
Node* const storage = mStorage;
assert_invariant(p && p >= storage);
Node* const node = static_cast<Node*>(p);
HeadPtr currentHead = mHead.load();
HeadPtr currentHead = mHead.load(std::memory_order_relaxed);
HeadPtr newHead = { int32_t(node - storage), currentHead.tag + 1 };
do {
newHead.tag = currentHead.tag + 1;
Node* const n = (currentHead.offset >= 0) ? (storage + currentHead.offset) : nullptr;
node->next.store(n, std::memory_order_relaxed);
} while(!mHead.compare_exchange_weak(currentHead, newHead));
Node* const pNext = (currentHead.offset >= 0) ? (storage + currentHead.offset) : nullptr;
node->next = pNext; // could be a race with pop, corrected by CAS
} while(!mHead.compare_exchange_weak(currentHead, newHead,
std::memory_order_release, std::memory_order_relaxed));
// release: no read/write can be reordered after this
}

void* getFirst() noexcept {
return mStorage + mHead.load(std::memory_order_relaxed).offset;
}

struct Node {
// This should be a regular (non-atomic) pointer, but this causes TSAN to complain
// about a data-race that exists but is benin. We always use this atomic<> in
// relaxed mode.
// The data race TSAN complains about is when a pop() is interrupted by a
// There is a benign data race when a pop() is interrupted by a
// pop() + push() just after mHead->next is read -- it appears as though it is written
// without synchronization (by the push), however in that case, the pop's CAS will fail
// and things will auto-correct.
Expand All @@ -346,7 +350,7 @@ class AtomicFreeList {
// |
// CAS, tag++
//
std::atomic<Node*> next;
Node* next = nullptr;
};

private:
Expand Down
2 changes: 1 addition & 1 deletion libs/utils/include/utils/JobSystem.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ class JobSystem {
Condition mWaiterCondition;

std::atomic<int32_t> mActiveJobs = { 0 };
utils::Arena<utils::ThreadSafeObjectPoolAllocator<Job>, LockingPolicy::NoLock> mJobPool;
utils::Arena<utils::ObjectPoolAllocator<Job>, LockingPolicy::Mutex> mJobPool;

template <typename T>
using aligned_vector = std::vector<T, utils::STLAlignedAllocator<T>>;
Expand Down

0 comments on commit 2ed1f2d

Please sign in to comment.