From 7907a50817f9065bc9ff85fcb1d9c7f8773f89f9 Mon Sep 17 00:00:00 2001 From: nmzik Date: Fri, 7 Aug 2026 09:26:37 +0200 Subject: [PATCH] minor optimizations --- src/graphics/host_gpu/memoryTracker.cpp | 10 +- src/graphics/host_gpu/memoryTracker.h | 5 +- tests/MemoryTrackerTests.cpp | 147 ++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 12 deletions(-) diff --git a/src/graphics/host_gpu/memoryTracker.cpp b/src/graphics/host_gpu/memoryTracker.cpp index 441696e1b..c5d6b6b2b 100644 --- a/src/graphics/host_gpu/memoryTracker.cpp +++ b/src/graphics/host_gpu/memoryTracker.cpp @@ -79,7 +79,6 @@ RegionManager* MemoryTracker::GetOrCreateRegion(uint64_t index) { bool MemoryTracker::IsRegionCpuModified(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); return Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { std::scoped_lock lock(manager->lock); return manager->IsModified(offset, bytes); @@ -88,7 +87,6 @@ bool MemoryTracker::IsRegionCpuModified(uint64_t vaddr, uint64_t size) { bool MemoryTracker::IsRegionGpuModified(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); return Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { std::scoped_lock lock(manager->lock); return manager->IsModified(offset, bytes); @@ -97,7 +95,6 @@ bool MemoryTracker::IsRegionGpuModified(uint64_t vaddr, uint64_t size) { void MemoryTracker::MarkRegionAsCpuModified(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { std::scoped_lock lock(manager->lock); manager->ChangeState(manager->GetCpuAddr() + offset, bytes); @@ -106,7 +103,6 @@ void MemoryTracker::MarkRegionAsCpuModified(uint64_t vaddr, uint64_t size) { void MemoryTracker::MarkRegionAsGpuModified(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { std::scoped_lock lock(manager->lock); manager->ChangeState(manager->GetCpuAddr() + offset, bytes); @@ -115,14 +111,13 @@ void MemoryTracker::MarkRegionAsGpuModified(uint64_t vaddr, uint64_t size) { void MemoryTracker::UnmarkRegionAsGpuModified(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); Iterate(vaddr, size, [](RegionManager* manager, uint64_t offset, uint64_t bytes) { std::scoped_lock lock(manager->lock); manager->ChangeState(manager->GetCpuAddr() + offset, bytes); }); } -void MemoryTracker::UntrackMemoryLocked(uint64_t vaddr, uint64_t size) { +void MemoryTracker::UntrackMemoryImpl(uint64_t vaddr, uint64_t size) { std::vector managers; managers.reserve((vaddr % TRACKER_REGION_SIZE + size + TRACKER_REGION_SIZE - 1) / TRACKER_REGION_SIZE); @@ -148,8 +143,7 @@ void MemoryTracker::UntrackMemoryLocked(uint64_t vaddr, uint64_t size) { void MemoryTracker::UntrackMemory(uint64_t vaddr, uint64_t size) { CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); - UntrackMemoryLocked(vaddr, size); + UntrackMemoryImpl(vaddr, size); } } // namespace Libs::Graphics diff --git a/src/graphics/host_gpu/memoryTracker.h b/src/graphics/host_gpu/memoryTracker.h index ff4cff8e1..0394fcbcd 100644 --- a/src/graphics/host_gpu/memoryTracker.h +++ b/src/graphics/host_gpu/memoryTracker.h @@ -68,7 +68,6 @@ public: static_assert(std::is_nothrow_invocable_v); static_assert(std::is_nothrow_invocable_v); CheckNotInUploadCallback(); - std::lock_guard access(m_access_mutex); std::vector managers; Iterate(vaddr, size, [&](RegionManager* manager, uint64_t, uint64_t) { managers.push_back(manager); @@ -109,7 +108,6 @@ public: static_assert(std::is_nothrow_invocable_v); static_assert(std::is_nothrow_invocable_v); CheckNotInUploadCallback(); - std::unique_lock access(m_access_mutex); Iterate(vaddr, size, [](RegionManager*, uint64_t, uint64_t) {}); const auto* previous_upload_owner = std::exchange(s_upload_owner, this); Iterate(vaddr, size, [&](RegionManager* manager, uint64_t offset, uint64_t bytes) { @@ -173,13 +171,12 @@ private: } static void ValidateRange(uint64_t vaddr, uint64_t size); - void UntrackMemoryLocked(uint64_t vaddr, uint64_t size); + void UntrackMemoryImpl(uint64_t vaddr, uint64_t size); RegionManager* GetOrCreateRegion(uint64_t index); std::unique_ptr[]> m_regions; std::vector> m_region_storage; std::mutex m_region_mutex; - std::mutex m_access_mutex; PageManager& m_page_manager; }; diff --git a/tests/MemoryTrackerTests.cpp b/tests/MemoryTrackerTests.cpp index 0b92c017f..a34408c01 100644 --- a/tests/MemoryTrackerTests.cpp +++ b/tests/MemoryTrackerTests.cpp @@ -2,6 +2,8 @@ #include "graphics/host_gpu/memoryTracker.h" #include "graphics/host_gpu/rangeSet.h" +#include +#include #include #include #include @@ -211,6 +213,40 @@ void TestQueriesDoNotRequireMappedOwnership() { "unowned tracker range did not expose its initial CPU-dirty state"); } +void TestConcurrentRegionPublication() { + TrackerHarness harness; + auto &tracker = harness.tracker; + auto &page_manager = harness.page_manager; + const auto page_size = page_manager.GetPageSize(); + auto *memory = Allocate(page_manager, 1); + const auto address = reinterpret_cast(memory); + + std::binary_semaphore start_first{0}; + std::binary_semaphore start_second{0}; + std::atomic_uint32_t cpu_dirty_results{0}; + std::jthread first([&] { + start_first.acquire(); + if (tracker.IsRegionCpuModified(address, page_size)) { + cpu_dirty_results.fetch_add(1, std::memory_order_relaxed); + } + }); + std::jthread second([&] { + start_second.acquire(); + if (tracker.IsRegionCpuModified(address, page_size)) { + cpu_dirty_results.fetch_add(1, std::memory_order_relaxed); + } + }); + start_first.release(); + start_second.release(); + first.join(); + second.join(); + + tracker.UntrackMemory(address, page_size); + Release(page_manager, memory, page_size); + Check(cpu_dirty_results.load(std::memory_order_relaxed) == 2, + "concurrent region publication lost initial CPU ownership"); +} + void TestCpuDirtyUpload() { TrackerHarness harness; auto &tracker = harness.tracker; @@ -497,6 +533,114 @@ void TestCrossRegionUpload() { Release(page_manager, memory, region_size * 2); } +void TestUploadDoesNotSerializeDisjointRegion() { + constexpr auto region_size = Libs::Graphics::TRACKER_REGION_SIZE; + constexpr auto page_size = Libs::Graphics::TRACKER_PAGE_SIZE; + TrackerHarness harness; + auto &tracker = harness.tracker; + auto &page_manager = harness.page_manager; + auto *memory = Allocate(page_manager, region_size * 2 / page_size); + const auto allocation_base = reinterpret_cast(memory); + const auto second_region = + (allocation_base & ~(region_size - 1)) + region_size; + Check(second_region + page_size <= allocation_base + region_size * 2, + "test allocation does not span two tracker regions"); + + // Publish both managers before the concurrent section so this test measures + // tracker access serialization rather than manager allocation. + Check(tracker.IsRegionCpuModified(allocation_base, page_size) && + tracker.IsRegionCpuModified(second_region, page_size), + "disjoint upload setup did not initialize both regions"); + + std::binary_semaphore upload_entered{0}; + std::binary_semaphore finish_upload{0}; + std::binary_semaphore query_finished{0}; + std::atomic_bool query_result{false}; + std::jthread uploader([&] { + tracker.ForEachUploadRange( + allocation_base, page_size, true, [](uint64_t, uint64_t) noexcept {}, + [&]() noexcept { + upload_entered.release(); + finish_upload.acquire(); + }); + }); + upload_entered.acquire(); + std::jthread query([&] { + query_result.store(tracker.IsRegionCpuModified(second_region, page_size), + std::memory_order_relaxed); + query_finished.release(); + }); + + const bool completed_while_upload_blocked = + query_finished.try_acquire_for(std::chrono::seconds(5)); + finish_upload.release(); + uploader.join(); + query.join(); + + tracker.UnmarkRegionAsGpuModified(allocation_base, page_size); + tracker.MarkRegionAsCpuModified(allocation_base, page_size); + tracker.UntrackMemory(allocation_base, region_size * 2); + Release(page_manager, memory, region_size * 2); + Check(completed_while_upload_blocked && + query_result.load(std::memory_order_relaxed), + "upload callback serialized an unrelated tracker region"); +} + +void TestDownloadDoesNotSerializeDisjointRegion() { + constexpr auto region_size = Libs::Graphics::TRACKER_REGION_SIZE; + constexpr auto page_size = Libs::Graphics::TRACKER_PAGE_SIZE; + TrackerHarness harness; + auto &tracker = harness.tracker; + auto &page_manager = harness.page_manager; + auto *memory = Allocate(page_manager, region_size * 2 / page_size); + const auto allocation_base = reinterpret_cast(memory); + const auto second_region = + (allocation_base & ~(region_size - 1)) + region_size; + Check(second_region + page_size <= allocation_base + region_size * 2, + "test allocation does not span two tracker regions"); + + tracker.ForEachUploadRange( + allocation_base, page_size, true, [](uint64_t, uint64_t) noexcept {}, + []() noexcept {}); + tracker.ForEachUploadRange( + second_region, page_size, false, [](uint64_t, uint64_t) noexcept {}, + []() noexcept {}); + + std::binary_semaphore download_entered{0}; + std::binary_semaphore finish_download{0}; + std::binary_semaphore mutation_finished{0}; + std::jthread downloader([&] { + tracker.ForEachDownloadRange(allocation_base, page_size, + [&](uint64_t, uint64_t) noexcept { + download_entered.release(); + finish_download.acquire(); + }); + }); + download_entered.acquire(); + std::jthread mutation([&] { + tracker.MarkRegionAsGpuModified(second_region, page_size); + mutation_finished.release(); + }); + + const bool completed_while_download_blocked = + mutation_finished.try_acquire_for(std::chrono::seconds(5)); + finish_download.release(); + downloader.join(); + mutation.join(); + + const bool both_gpu_owned = + tracker.IsRegionGpuModified(allocation_base, page_size) && + tracker.IsRegionGpuModified(second_region, page_size); + tracker.UnmarkRegionAsGpuModified(allocation_base, page_size); + tracker.UnmarkRegionAsGpuModified(second_region, page_size); + tracker.MarkRegionAsCpuModified(allocation_base, page_size); + tracker.MarkRegionAsCpuModified(second_region, page_size); + tracker.UntrackMemory(allocation_base, region_size * 2); + Release(page_manager, memory, region_size * 2); + Check(completed_while_download_blocked && both_gpu_owned, + "download callback serialized an unrelated tracker region"); +} + void TestGpuUnmarkUsesRegionMask() { constexpr auto region_size = Libs::Graphics::TRACKER_REGION_SIZE; constexpr auto page_size = Libs::Graphics::TRACKER_PAGE_SIZE; @@ -683,6 +827,7 @@ int main(int argc, char **argv) { } TestRangeSet(); TestQueriesDoNotRequireMappedOwnership(); + TestConcurrentRegionPublication(); TestCpuDirtyUpload(); TestRangeInvalidation(); TestGpuReacquisitionAfterInvalidation(); @@ -690,6 +835,8 @@ int main(int argc, char **argv) { TestExactDirtyIntervalsSharingTrackerPage(); TestGpuDownloadProtectionMirrors(); TestCrossRegionUpload(); + TestUploadDoesNotSerializeDisjointRegion(); + TestDownloadDoesNotSerializeDisjointRegion(); TestGpuUnmarkUsesRegionMask(); TestFullRegionGpuUnmarkBatching(); TestFatalPaths();