common: SlotVector without generations

CMakeLists.txt

@@ -451,6 +451,11 @@ add_executable(lru_cache_tests EXCLUDE_FROM_ALL
 )
 target_include_directories(lru_cache_tests PRIVATE ${inc_headers})
 
+add_executable(slot_vector_tests EXCLUDE_FROM_ALL
+	"${KYTY_TESTS_DIR}/SlotVectorTests.cpp"
+)
+target_include_directories(slot_vector_tests PRIVATE ${inc_headers})
+
 add_executable(ime_dialog_tests EXCLUDE_FROM_ALL
 	"${KYTY_TESTS_DIR}/ImeDialogTests.cpp"
 	"${KYTY_SOURCE_DIR}/libs/imeCommon.cpp"

src/common/slotVector.h

@@ -1,15 +1,15 @@
 #ifndef EMULATOR_SRC_COMMON_SLOTVECTOR_H_
 #define EMULATOR_SRC_COMMON_SLOTVECTOR_H_
 
-#include "common/assert.h"
-#include "common/abi.h"
+#include "common/common.h"
 
 #include <compare>
 #include <cstdint>
-#include <deque>
 #include <functional>
 #include <limits>
-#include <optional>
+#include <memory>
+#include <new>
+#include <numeric>
 #include <utility>
 #include <vector>
 
@@ -19,105 +19,105 @@ struct SlotId {
 	static constexpr uint32_t INVALID_INDEX = std::numeric_limits<uint32_t>::max();
 
 	constexpr SlotId() noexcept = default;
-	constexpr SlotId(uint32_t value) noexcept: index(value), generation(1) {}
-	constexpr SlotId(uint32_t value, uint32_t slot_generation) noexcept
-	    : index(value), generation(slot_generation) {}
+	constexpr SlotId(uint32_t index) noexcept: index(index) {}
 
-	[[nodiscard]] constexpr explicit operator bool() const noexcept { return index != INVALID_INDEX; }
+	[[nodiscard]] constexpr explicit operator bool() const noexcept {
+		return index != INVALID_INDEX;
+	}
 	constexpr auto operator<=>(const SlotId&) const noexcept = default;
 
 	uint32_t index = INVALID_INDEX;
-	uint32_t generation = 0;
 };
 
-// Stable-address slot storage for cache resources that are intentionally non-movable.
+// Fixed-size chunks keep the stored, non-movable resources in place. Occupancy is a byte per
+// slot, not a bitset: a deferred erase runs while fault threads read other slots.
 template <typename T>
 class SlotVector {
+	static constexpr uint32_t ChunkSize = 256;
+
 public:
 	SlotVector() = default;
 	KYTY_CLASS_NO_COPY(SlotVector);
 
+	~SlotVector() noexcept {
+		ForEach([](SlotId, T& value) { value.~T(); });
+	}
+
 	[[nodiscard]] T& operator[](SlotId id) noexcept {
-		EXIT_IF(!is_allocated(id));
-		return *m_values[id.index].value;
+		return m_chunks[id.index / ChunkSize][id.index % ChunkSize].object;
 	}
 
 	[[nodiscard]] const T& operator[](SlotId id) const noexcept {
-		EXIT_IF(!is_allocated(id));
-		return *m_values[id.index].value;
+		return m_chunks[id.index / ChunkSize][id.index % ChunkSize].object;
 	}
 
 	[[nodiscard]] T* try_get(SlotId id) noexcept {
-		return is_allocated(id) ? &*m_values[id.index].value : nullptr;
+		return is_allocated(id) ? &(*this)[id] : nullptr;
 	}
 
 	[[nodiscard]] const T* try_get(SlotId id) const noexcept {
-		return is_allocated(id) ? &*m_values[id.index].value : nullptr;
+		return is_allocated(id) ? &(*this)[id] : nullptr;
 	}
 
 	[[nodiscard]] bool is_allocated(SlotId id) const noexcept {
-		return id && id.index < m_values.size() &&
-		       m_values[id.index].generation == id.generation && m_values[id.index].value.has_value();
+		return id.index < m_stored.size() && m_stored[id.index] != 0;
 	}
 
 	template <typename... Args>
 	[[nodiscard]] SlotId insert(Args&&... args) {
-		uint32_t index = 0;
 		if (m_free_list.empty()) {
-			index = static_cast<uint32_t>(m_values.size());
-			m_values.emplace_back();
-			m_values.back().value.emplace(std::forward<Args>(args)...);
-		} else {
-			index = m_free_list.back();
-			m_free_list.pop_back();
-			EXIT_IF(m_values[index].value.has_value());
-			m_values[index].value.emplace(std::forward<Args>(args)...);
+			Reserve();
 		}
-		++m_size;
-		return SlotId {index, m_values[index].generation};
+		const uint32_t index = m_free_list.back();
+		m_free_list.pop_back();
+		new (&(*this)[index]) T(std::forward<Args>(args)...);
+		m_stored[index] = 1;
+		return SlotId {index};
 	}
 
 	void erase(SlotId id) noexcept {
-		EXIT_IF(!is_allocated(id));
-		auto& slot = m_values[id.index];
-		slot.value.reset();
-		if (++slot.generation == 0) {
-			slot.generation = 1;
-		}
+		(*this)[id].~T();
+		m_stored[id.index] = 0;
 		m_free_list.push_back(id.index);
-		--m_size;
 	}
 
-	[[nodiscard]] size_t size() const noexcept { return m_size; }
-	[[nodiscard]] size_t capacity() const noexcept { return m_values.size(); }
-
 	template <typename F>
 	void ForEach(F&& fn) {
-		for (uint32_t index = 0; index < m_values.size(); ++index) {
-			if (m_values[index].value) {
-				fn(SlotId {index, m_values[index].generation}, *m_values[index].value);
+		for (uint32_t index = 0; index < m_stored.size(); ++index) {
+			if (m_stored[index] != 0) {
+				fn(SlotId {index}, (*this)[index]);
 			}
 		}
 	}
 
 	template <typename F>
 	void ForEach(F&& fn) const {
-		for (uint32_t index = 0; index < m_values.size(); ++index) {
-			if (m_values[index].value) {
-				fn(SlotId {index, m_values[index].generation}, *m_values[index].value);
+		for (uint32_t index = 0; index < m_stored.size(); ++index) {
+			if (m_stored[index] != 0) {
+				fn(SlotId {index}, (*this)[index]);
 			}
 		}
 	}
 
 private:
-	struct Slot {
-		std::optional<T> value;
-		uint32_t         generation = 1;
+	union Slot {
+		Slot() noexcept {}
+		~Slot() noexcept {}
+
+		T object;
 	};
 
-	std::deque<Slot>             m_values;
-	std::vector<uint32_t>        m_free_list;
-	size_t                       m_size = 0;
+	void Reserve() {
+		const auto first = static_cast<uint32_t>(m_stored.size());
+		m_chunks.push_back(std::make_unique<Slot[]>(ChunkSize));
+		m_stored.resize(first + ChunkSize);
+		m_free_list.resize(ChunkSize);
+		std::iota(m_free_list.rbegin(), m_free_list.rend(), first);
+	}
+
+	std::vector<std::unique_ptr<Slot[]>> m_chunks;
+	std::vector<uint8_t>                 m_stored;
+	std::vector<uint32_t>                m_free_list;
 };
 
 } // namespace Common
@@ -125,7 +125,7 @@ private:
 template <>
 struct std::hash<Common::SlotId> {
 	[[nodiscard]] size_t operator()(Common::SlotId id) const noexcept {
-		return std::hash<uint64_t> {}((static_cast<uint64_t>(id.generation) << 32u) | id.index);
+		return std::hash<uint32_t> {}(id.index);
 	}
 };
 

src/graphics/host_gpu/renderer/image/image.h

@@ -139,7 +139,10 @@ public:
 	}
 
 	static inline std::atomic<uint64_t> heap_epoch {0};
+	static inline std::atomic<uint64_t> serials {0};
 
+	// Unique per image, unlike ImageId: freed slots are reused.
+	const uint64_t   serial = serials.fetch_add(1u, std::memory_order_relaxed) + 1u;
 	ImageInfo        info;
 	VulkanImage      backing;
 	std::vector<CachedImageView> views;

src/graphics/host_gpu/renderer/pipeline/imageHeap.cpp

@@ -350,12 +350,17 @@ void ImageHeap::Release(View& view, uint32_t entry) {
 }
 
 void ImageHeap::Unreference(Element& element) {
-	if (auto* image = m_context.GetTextureCache().m_slot_images.try_get(element.binding.image_id)) {
+	if (auto* image = ImageOf(element)) {
 		image->heap_refs.fetch_sub(1u, std::memory_order_relaxed);
 		image->tick_accessed_last = m_context.GetCommandScheduler().CurrentTick();
 	}
 }
 
+Image* ImageHeap::ImageOf(const Element& element) const {
+	auto* image = m_context.GetTextureCache().m_slot_images.try_get(element.binding.image_id);
+	return image != nullptr && image->serial == element.serial ? image : nullptr;
+}
+
 uint32_t ImageHeap::Allocate() {
 	auto& scheduler = m_context.GetCommandScheduler();
 	while (!m_retired.empty() && scheduler.IsFree(m_retired.front().first)) {
@@ -393,7 +398,7 @@ void ImageHeap::Rebind(RenderExecutor& executor) {
 			for (const auto& [value, index]: view->elements) {
 				const auto& binding = m_elements[index].binding;
 				if (!binding.image_id) continue;
-				const auto* image = textures.m_slot_images.try_get(binding.image_id);
+				const auto* image = ImageOf(m_elements[index]);
 				if (image == nullptr || !image->registered || image->depth_id ||
 				    image->binding.needs_rebind) {
 					mark(index);
@@ -434,6 +439,7 @@ void ImageHeap::Rebind(RenderExecutor& executor) {
 			if (binding.image_id) {
 				auto& image = textures.GetImage(binding.image_id);
 				image.heap_refs.fetch_add(1u, std::memory_order_relaxed);
+				m_elements[index].serial = image.serial;
 				binding.layout     = image.ReadLayout();
 				binding.image_view = textures.FindTexture(binding.image_id, binding.desc);
 				if (binding.desc.info.metadata.kind == ImageMetadataKind::Dcc) {
@@ -471,8 +477,7 @@ void ImageHeap::WriteDescriptors() {
 void ImageHeap::Commit(CommandBuffer& buffer, vk::PipelineBindPoint bind_point,
                        vk::PipelineLayout layout) {
 	KYTY_PROFILER_FUNCTION();
-	auto&      textures = m_context.GetTextureCache();
-	const auto epoch    = Image::heap_epoch.load(std::memory_order_relaxed);
+	const auto epoch = Image::heap_epoch.load(std::memory_order_relaxed);
 	for (auto* view: m_active) {
 		if (view->dirty_begin != view->dirty_end) {
 			const auto slots = std::span(view->slots).subspan(view->dirty_begin,
@@ -493,7 +498,7 @@ void ImageHeap::Commit(CommandBuffer& buffer, vk::PipelineBindPoint bind_point,
 		// this draw's discovery replaced lives until the tick ends, and its slots still select it.
 		std::erase_if(view->away, [&](uint32_t index) {
 			const auto& element = m_elements[index];
-			auto*       image   = textures.m_slot_images.try_get(element.binding.image_id);
+			auto*       image   = ImageOf(element);
 			if (element.refs == 0u || image == nullptr) return true;
 			if (image->binding.is_target || image->binding.shader_write ||
 			    image->binding.force_general) {

src/graphics/host_gpu/renderer/pipeline/imageHeap.h

@@ -57,9 +57,10 @@ private:
 	struct Element {
 		DescriptorValue value;
 		TextureBinding  binding;
-		uint32_t        kind  = 0;
-		uint32_t        refs  = 0; // Slots of the owning view that select it.
-		bool            stale = false;
+		uint32_t        kind   = 0;
+		uint32_t        refs   = 0; // Slots of the owning view that select it.
+		bool            stale  = false;
+		uint64_t        serial = 0; // Image::serial of binding.image_id.
 	};
 
 	struct Memory;
@@ -109,6 +110,7 @@ private:
 	uint32_t Intern(View& view, uint32_t kind, const DescriptorValue& value);
 	void     Release(View& view, uint32_t entry);
 	void     Unreference(Element& element);
+	Image*   ImageOf(const Element& element) const;
 	uint32_t Allocate();
 	void     Retire(View& view);
 	void     WriteDescriptors();

tests/ShaderRecompilerComputeTests.cpp

@@ -331,22 +331,6 @@ struct TextureCacheTestAccess {
     return count;
   }
 
-  static void AddPageOwner(TextureCache &cache, uint64_t address, ImageId id) {
-    std::lock_guard lock(cache.m_lock);
-    cache
-        .m_image_page_table[static_cast<size_t>(
-            address >> TextureCache::ImagePageTable::kPageBits)]
-        .push_back(id);
-  }
-
-  static bool RemovePageOwner(TextureCache &cache, uint64_t address,
-                              ImageId id) {
-    std::lock_guard lock(cache.m_lock);
-    auto *owners = cache.m_image_page_table.Find(static_cast<size_t>(
-        address >> TextureCache::ImagePageTable::kPageBits));
-    return owners != nullptr && owners->Erase(id);
-  }
-
   static void SetQueryEpoch(TextureCache &cache, uint32_t epoch) {
     std::lock_guard lock(cache.m_lock);
     cache.m_image_query_epoch = epoch;
@@ -4347,11 +4331,8 @@ public:
       constexpr uint64_t recycled_offset = index_offset + 4 * index_page;
       const auto recycled = cache.FindBuffer(base + recycled_offset + 0x100,
                                              sizeof(uint32_t));
-      Require(name, "slot generation reuse",
-              (recycled.index == index_left.index && recycled != index_left) ||
-                  (recycled.index == index_right.index &&
-                   recycled != index_right),
-              "retired Buffer slot was not reused with a new generation");
+      Require(name, "slot reuse", recycled == index_left || recycled == index_right,
+              "retired Buffer slot was not reused");
       const auto [resolved_left, resolved_left_offset] = cache.ObtainBuffer(
           index_begin + 0x100, sizeof(uint32_t), true, false, index_left);
       const auto [resolved_right, resolved_right_offset] =
@@ -5465,6 +5446,28 @@ public:
       Require(name, "unmapped descriptor", !textures.IsRegionRegistered(texture(23), 0x1000u),
               "a descriptor over unmapped memory was resolved");
 
+      // An idle view keeps the element of a freed image. Once a new image takes its slot,
+      // reactivating the view must resolve the descriptor again.
+      write(30, descriptor(30, Format::k8_8_8_8UNorm));
+      Require(name, "idle element", acquire(unfiltered), "the view was declined");
+      draw();
+      const auto erased = ImageHeapTestAccess::ImageOf(heap, idle.slots[240]);
+      textures.UnmapMemory(texture(30), 0x1000u);
+      Require(name, "replacement", acquire(sampled), "the view was declined");
+      draw();
+      write(31, descriptor(31, Format::k8_8_8_8UNorm));
+      Require(name, "slot reuse", acquire(sampled), "the view was declined");
+      draw();
+      Require(name, "reused slot",
+              erased && ImageHeapTestAccess::ImageOf(heap, filtered.slots[248]) == erased,
+              "the next image did not take the freed image's slot");
+      Require(name, "reactivated idle view", acquire(unfiltered), "the view was declined");
+      draw();
+      Require(name, "reused slot element",
+              ImageHeapTestAccess::ImageOf(heap, idle.slots[240]) ==
+                  ImageHeapTestAccess::ImageOf(heap, filtered.slots[240]),
+              "a reactivated view kept an element whose image slot holds a new image");
+
       heap.UnmapMemory(table, table_size);
       Require(name, "unmapped table",
               ImageHeapTestAccess::MemoryCount(heap) == 0u &&
@@ -6060,16 +6063,6 @@ public:
                   large_owner) == 0,
               "large image unregister left coarse memberships behind");
 
-      const ImageId stale{first.index, first.generation + 1};
-      TextureCacheTestAccess::AddPageOwner(texture_cache, base, stale);
-      const auto stale_filtered = TextureCacheTestAccess::FindImages(
-          texture_cache, base, sizeof(initial), false);
-      Require(name, "stale page owner filtering",
-              IsOnlyImage(stale_filtered, first) &&
-                  TextureCacheTestAccess::RemovePageOwner(texture_cache, base,
-                                                          stale),
-              "a stale generation escaped the direct page-owner lookup");
-
       TextureCacheTestAccess::SetQueryEpoch(texture_cache, UINT32_MAX);
       const auto wrap_results = TextureCacheTestAccess::FindImages(
           texture_cache, base, sizeof(initial), false);
@@ -17009,9 +17002,7 @@ public:
       scheduler.EndRendering();
       RenderExecutorTestAccess::ResetBindings(executor);
       Require(name, "draw-scoped target reset",
-              TextureCacheTestAccess::Owner(texture_cache,
-                                            target_subresource_id) == nullptr &&
-                  !texture_cache.GetImage(target_parent_id).binding.is_target &&
+              !texture_cache.GetImage(target_parent_id).binding.is_target &&
                   !texture_cache.GetImage(depth_id).binding.is_target,
               "RenderExecutor retained target binding state after reset");
 
@@ -48130,37 +48121,6 @@ void CheckDepthTargetFootprints() {
   std::printf("[host]    %-32s ok\n", "DepthTargetFootprints");
 }
 
-struct SlotLifetimeProbe {
-  explicit SlotLifetimeProbe(bool *destroyed) : destroyed(destroyed) {
-    if (destroyed == nullptr || *destroyed) {
-      EXIT("fence-lifetime probe has invalid construction state\n");
-    }
-  }
-  ~SlotLifetimeProbe() { *destroyed = true; }
-
-  bool *destroyed = nullptr;
-};
-
-void CheckSlotVectorLifetime() {
-  bool destroyed = false;
-  Common::SlotVector<SlotLifetimeProbe> slots;
-  const auto first = slots.insert(&destroyed);
-  Require("SlotVectorLifetime", "inserted",
-          slots.is_allocated(first) && slots.try_get(first) != nullptr &&
-              !destroyed,
-          "slot insertion did not expose its cache-owned object");
-  slots.erase(first);
-  const bool first_destroyed = destroyed;
-  destroyed = false;
-  const auto second = slots.insert(&destroyed);
-  Require("SlotVectorLifetime", "generation",
-          first_destroyed && !destroyed && first.index == second.index &&
-              first != second && !slots.is_allocated(first) &&
-              slots.is_allocated(second),
-          "reused slot accepted a stale cache resource id");
-  std::printf("[host]    %-32s ok\n", "SlotVectorLifetime");
-}
-
 void CheckEmbeddedFetchLaneSpill() {
   std::vector<u32> code;
   code.push_back(EncodeSMovB32(0, InlineU32(0)));
@@ -51297,7 +51257,6 @@ int main(int argc, char **argv) {
   CheckDepthFeedbackAspects();
   CheckDynamicRenderingState();
   CheckDepthTargetFootprints();
-  CheckSlotVectorLifetime();
 #else
   if (argc != 1) {
     std::fprintf(stderr, "unknown test selector: %s\n", argv[1]);

tests/SlotVectorTests.cpp

@@ -0,0 +1,103 @@
+#include "common/slotVector.h"
+
+#include <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <utility>
+#include <vector>
+
+namespace {
+
+void Check(bool value, const char *message) {
+  if (!value) {
+    std::fprintf(stderr, "SlotVectorTests: failed: %s\n", message);
+    std::abort();
+  }
+}
+
+// Non-movable like the cached Image/Buffer; counts live objects.
+struct Probe {
+  Probe(int *live, uint32_t value) : live(live), value(value) { ++*live; }
+  ~Probe() { --*live; }
+  KYTY_CLASS_NO_COPY(Probe);
+
+  int *live;
+  uint32_t value;
+};
+
+static_assert(sizeof(Common::SlotId) == sizeof(uint32_t));
+
+void TestInsertEraseReuse() {
+  int live = 0;
+  Common::SlotVector<Probe> slots;
+  Check(!slots.is_allocated(Common::SlotId {}) && slots.try_get(Common::SlotId {}) == nullptr &&
+            !slots.is_allocated(Common::SlotId {0}),
+        "an empty vector reported an allocated slot");
+
+  const auto a = slots.insert(&live, 0u);
+  const auto b = slots.insert(&live, 1u);
+  const auto c = slots.insert(&live, 2u);
+  Check(a.index == 0 && b.index == 1 && c.index == 2 && live == 3,
+        "fresh slots were not handed out in order");
+  Check(slots.is_allocated(b) && slots.try_get(b) == &slots[b] && slots[b].value == 1,
+        "an inserted slot did not expose its object");
+  Check(!slots.is_allocated(Common::SlotId {3}), "an unused slot reported an object");
+
+  slots.erase(b);
+  slots.erase(a);
+  Check(live == 1 && !slots.is_allocated(a) && !slots.is_allocated(b) &&
+            slots.try_get(b) == nullptr && slots.is_allocated(c),
+        "erase did not destroy and free exactly its slot");
+
+  // Freed indices are reused most recently freed first, with the same plain id.
+  const auto reused_a = slots.insert(&live, 0u);
+  const auto reused_b = slots.insert(&live, 1u);
+  const auto d = slots.insert(&live, 3u);
+  Check(reused_a == a && reused_b == b && d.index == 3 && live == 4,
+        "freed slots were not reused in LIFO order");
+  Check(slots[reused_a].value == 0 && slots[reused_b].value == 1,
+        "a reused slot did not expose its new object");
+}
+
+void TestForEachAndStability() {
+  int live = 0;
+  {
+    Common::SlotVector<Probe> slots;
+    const auto first = slots.insert(&live, 0u);
+    Probe *const first_address = &slots[first];
+
+    // Cross several chunks; earlier objects must keep their address.
+    std::vector<Common::SlotId> ids {first};
+    for (uint32_t i = 1; i < 1000; ++i) {
+      ids.push_back(slots.insert(&live, i));
+    }
+    Check(&slots[first] == first_address && live == 1000,
+          "growing the vector moved a stored object");
+
+    std::vector<uint32_t> expected;
+    for (uint32_t i = 0; i < 1000; ++i) {
+      if (i % 3 == 0) {
+        slots.erase(ids[i]);
+      } else {
+        expected.push_back(i);
+      }
+    }
+    std::vector<uint32_t> visited;
+    std::as_const(slots).ForEach([&](Common::SlotId id, const Probe &probe) {
+      Check(probe.value == id.index, "ForEach passed an id that does not match its object");
+      visited.push_back(id.index);
+    });
+    Check(visited == expected && live == static_cast<int>(expected.size()),
+          "ForEach did not visit exactly the live slots in index order");
+  }
+  Check(live == 0, "destroying the vector did not destroy every live object exactly once");
+}
+
+} // namespace
+
+int main() {
+  TestInsertEraseReuse();
+  TestForEachAndStability();
+  std::puts("SlotVectorTests: all cases passed");
+  return 0;
+}