GS: a source comes out of a pool and owns no allocations

Three trips through the allocator per source, all inside a draw: the
object itself (GSAlignedClass's new, 1.4 KB), the 2 KB per-page valid
bitmap on the first write to it, and a 48-byte block for three
rectangles, also on the first write. A game that makes and drops sources
every frame - a channel shuffle does - paid all three every time, and
the cost is not just the allocator's: it is the frame-to-frame variation
of it, and it is what puts a heap block header immediately before a
source, where the crash under investigation reads one that has been
overwritten.

None of the three is needed.

The valid bitmap and the three rectangles are part of the object now,
with m_valid_init saying whether the bitmap has been cleared yet - which
is what the pointer being null used to say. The object comes from a pool
of SOURCE_POOL_SIZE slots, one allocation made when the first source is
asked for: Source::operator new takes the next free slot index and
operator delete puts it back, a couple of loads and a store either way,
with no allocator and no variation. More live sources than the pool
holds fall back to the allocator, so the number is a budget rather than
a limit and a game that exceeds it still runs.

A source is 3472 bytes, so the pool is 1.28 MB, made once and never
grown. That is the whole of the increase, and it replaces allocations
that were happening per source per draw. The destructor now frees
nothing of its own.

tests/gspool covers the slot arithmetic: every slot handed out once,
each 32-byte aligned, none overlapping another (written full and read
back), returned and reused across passes, and the fallback past the
pool's size. Plain and under ASan/UBSan.

This does not explain the crash. It does take the source's own
allocation out of the path that crashes, and out of every draw.

GSTextureCache.cpp, GSRendererHW.cpp, GSHwHack.cpp,
GSTextureReplacements.cpp and GSState.cpp compile with the Makefile's
flags. Not run.

pcsx2/GS/Renderers/HW/GSTextureCache.cpp

@@ -3165,9 +3165,9 @@ void GSTextureCache::InvalidateVideoMem(const GSOffset& off, const GSVector4i& r
 					}
 					else
 					{
-						u32* RESTRICT valid = s->m_valid.get();
+						u32* RESTRICT valid = s->m_valid;
 
-						if (valid && !s->CanPreload())
+						if (s->m_valid_init && !s->CanPreload())
 						{
 							// Invalidate data of input texture
 							if (s->m_repeating)
@@ -5469,6 +5469,64 @@ bool GSTextureCache::Surface::Overlaps(u32 bp, u32 bw, u32 psm, const GSVector4i
 
 // GSTextureCache::Source
 
+/* --- the source pool ---------------------------------------------------
+ * One allocation, made the first time a source is asked for, holding
+ * SOURCE_POOL_SIZE slots of whatever a Source is; a free list of slot
+ * indices on top. Taking and returning one is a couple of loads and a
+ * store, with no allocator, no lock and no per-draw cost that varies.
+ * Sources past the pool's size come from the allocator as before, so a
+ * game that somehow holds more of them still runs - slower, and no
+ * worse than it was.
+ *
+ * The GS runs on one thread; none of this is shared. */
+static u8*    s_source_pool;
+static size_t s_source_stride;
+static u16    s_source_free[GSTextureCache::SOURCE_POOL_SIZE];
+static u32    s_source_free_count;
+
+void* GSTextureCache::Source::operator new(size_t size)
+{
+	if (!s_source_pool)
+	{
+		u32 i;
+		/* Slots keep the 32-byte alignment the class asks for. */
+		s_source_stride = (size + 31) & ~(size_t)31;
+		s_source_pool = (u8*)memalign_alloc(32, s_source_stride * SOURCE_POOL_SIZE);
+		if (!s_source_pool)
+		{
+			s_source_stride = 0;
+			return memalign_alloc(32, size);
+		}
+		/* Handed out from the end, so the first sources of a run are the
+		 * low slots and stay together. */
+		for (i = 0; i < SOURCE_POOL_SIZE; i++)
+			s_source_free[i] = (u16)(SOURCE_POOL_SIZE - 1 - i);
+		s_source_free_count = SOURCE_POOL_SIZE;
+	}
+
+	if (s_source_free_count && size <= s_source_stride)
+		return s_source_pool + (size_t)s_source_free[--s_source_free_count] * s_source_stride;
+
+	return memalign_alloc(32, size);
+}
+
+void GSTextureCache::Source::operator delete(void* p)
+{
+	if (!p)
+		return;
+
+	if (s_source_pool && (u8*)p >= s_source_pool
+	 && (u8*)p < s_source_pool + s_source_stride * SOURCE_POOL_SIZE)
+	{
+		const size_t off = (size_t)((u8*)p - s_source_pool);
+		if (s_source_free_count < SOURCE_POOL_SIZE)
+			s_source_free[s_source_free_count++] = (u16)(off / s_source_stride);
+		return;
+	}
+
+	memalign_free(p);
+}
+
 GSTextureCache::Source::Source(const GIFRegTEX0& TEX0, const GIFRegTEXA& TEXA)
 {
 	m_TEX0 = TEX0;
@@ -5495,8 +5553,6 @@ GSTextureCache::Source::~Source()
 	}
 	m_alive = 0;
 
-	memalign_free(m_write.rect);
-
 	// Shared textures are pointers copy. Therefore no allocation
 	// to recycle.
 	if (!m_shared_texture && !m_from_hash_cache && m_texture)
@@ -5571,8 +5627,11 @@ void GSTextureCache::Source::Update(const GSVector4i& rect, int level)
 
 	u32 blocks = 0;
 
-	if (!m_valid)
-		m_valid = std::make_unique<u32[]>(GS_MAX_PAGES);
+	if (!m_valid_init)
+	{
+		memset(m_valid, 0, sizeof(m_valid));
+		m_valid_init = true;
+	}
 
 	if (m_repeating)
 	{
@@ -5646,9 +5705,6 @@ void GSTextureCache::Source::UpdateLayer(const GIFRegTEX0& TEX0, const GSVector4
 
 void GSTextureCache::Source::Write(const GSVector4i& r, int layer, const GSOffset& off)
 {
-	if (!m_write.rect)
-		m_write.rect = static_cast<GSVector4i*>(memalign_alloc(16, 3 * sizeof(GSVector4i)));
-
 	m_write.rect[m_write.count++] = r;
 
 	while (m_write.count >= 2)

pcsx2/GS/Renderers/HW/GSTextureCache.h

@@ -276,6 +276,11 @@ public:
 		bool ResizeTexture(int new_unscaled_width, int new_unscaled_height, bool recycle_old = true);
 	};
 
+	/* How many sources exist at once, and so how many the pool holds.
+	 * A frame uses a few dozen; past this the allocator is used as
+	 * before, so the number is a budget, not a limit. */
+	static constexpr u32 SOURCE_POOL_SIZE = 384;
+
 	class Source : public Surface
 	{
 	public:
@@ -290,9 +295,13 @@ public:
 		u32 m_alive = ALIVE;
 
 	private:
+		/* Three rectangles, in the object. They were a 48-byte allocation
+		 * made on the first Write of every source and freed in the
+		 * destructor - two trips through the allocator per source, in a
+		 * draw, for something this size. */
 		struct
 		{
-			GSVector4i* rect;
+			alignas(16) GSVector4i rect[3];
 			u32 count;
 		} m_write = {};
 
@@ -304,7 +313,12 @@ public:
 	public:
 		HashCacheEntry* m_from_hash_cache = nullptr;
 		std::shared_ptr<Palette> m_palette_obj;
-		std::unique_ptr<u32[]> m_valid;// each u32 bits map to the 32 blocks of that page
+		/* One bit per block, 32 blocks to a page, in the object rather
+		 * than a 2 KB allocation made the first time a source is written
+		 * to and freed with it. m_valid_init says whether it has been
+		 * cleared yet, which is what the pointer being null used to say. */
+		u32 m_valid[GS_MAX_PAGES];
+		bool m_valid_init = false;
 		GSTexture* m_palette = nullptr;
 		GSVector4i m_valid_rect = {};
 		GSVector2i m_lod = {};
@@ -330,6 +344,14 @@ public:
 		GSOffset::PageLooper m_pages;
 
 	public:
+		/* Out of a pool fixed at startup, not the allocator: a source is
+		 * made and destroyed inside a draw, and a 3.5 KB allocation per
+		 * draw is both a frame-time cost and the thing that puts a heap
+		 * block header next to it to be corrupted. Falls back to the
+		 * allocator only if more than SOURCE_POOL_SIZE are live. */
+		static void* operator new(size_t size);
+		static void operator delete(void* p);
+
 		Source(const GIFRegTEX0& TEX0, const GIFRegTEXA& TEXA);
 		virtual ~Source();
 

tests/gspool/build.sh

@@ -0,0 +1,14 @@
+#!/bin/sh
+# The texture cache's source pool (GSTextureCache::Source::operator
+# new/delete): slots handed out once each, 32-byte aligned, not
+# overlapping, returned and reused, and the fallback past the pool's
+# size. The arithmetic only - the class itself needs the whole GS.
+set -e
+DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)
+ROOT=$(CDPATH= cd -- "$DIR/../.." && pwd)
+LC="$ROOT/libretro/libretro-common"
+INC="-I $ROOT -I $ROOT/pcsx2 -I $ROOT/common -I $ROOT/common/include -I $LC/include"
+
+${CXX:-c++} -std=c++17 -O2 -g -w $SANFLAGS $INC -o "$DIR/gspool_test" \
+	"$DIR/main.cpp" "$LC/memmap/memalign.c"
+"$DIR/gspool_test"

tests/gspool/main.cpp

@@ -0,0 +1,70 @@
+/* The source pool's slot arithmetic, on its own: take every slot, give
+ * them back, take them again, and overflow past the pool. */
+#include <cstdio>
+#include <cstring>
+#include <cstdlib>
+#include <set>
+#include "common/Pcsx2Defs.h"
+#include <memalign.h>
+#define POOL 384
+static unsigned char* pool; static size_t stride;
+static unsigned short freelist[POOL]; static unsigned freecount;
+static void* take(size_t size)
+{
+	if (!pool) { unsigned i; stride=(size+31)&~(size_t)31;
+		pool=(unsigned char*)memalign_alloc(32, stride*POOL);
+		for (i=0;i<POOL;i++) freelist[i]=(unsigned short)(POOL-1-i);
+		freecount=POOL; }
+	if (freecount && size<=stride) return pool + (size_t)freelist[--freecount]*stride;
+	return memalign_alloc(32, size);
+}
+static void give(void* p)
+{
+	if (!p) return;
+	if (pool && (unsigned char*)p>=pool && (unsigned char*)p<pool+stride*POOL)
+	{ size_t off=(size_t)((unsigned char*)p-pool);
+	  if (freecount<POOL) freelist[freecount++]=(unsigned short)(off/stride);
+	  return; }
+	memalign_free(p);
+}
+static int fails;
+#define CHECK(c,m) do{ if(!(c)){ printf("  FAIL: %s\n", m); fails++; } }while(0)
+int main()
+{
+	const size_t sz = 3472;
+	std::set<void*> live;
+	void* p[POOL+16];
+	int i, pass;
+	for (pass = 0; pass < 3; pass++)
+	{
+		live.clear();
+		for (i = 0; i < POOL; i++)
+		{
+			p[i] = take(sz);
+			CHECK(p[i] != NULL, "pool slot handed out");
+			CHECK(((uintptr_t)p[i] & 31) == 0, "slot is 32-byte aligned");
+			CHECK(live.insert(p[i]).second, "slot not handed out twice");
+			memset(p[i], 0xA5, sz);           /* the whole slot is ours */
+		}
+		/* Past the pool: still works, from the allocator. */
+		for (i = 0; i < 16; i++)
+		{
+			p[POOL + i] = take(sz);
+			CHECK(p[POOL + i] != NULL, "overflow allocation");
+			CHECK(live.insert(p[POOL + i]).second, "overflow is distinct");
+			memset(p[POOL + i], 0x5A, sz);
+		}
+		for (i = POOL + 15; i >= 0; i--)
+			give(p[i]);
+	}
+	/* Slots do not overlap: write a marker in each, then check them all. */
+	for (i = 0; i < POOL; i++) { p[i] = take(sz); memset(p[i], i & 0xff, sz); }
+	for (i = 0; i < POOL; i++)
+	{
+		unsigned char* q = (unsigned char*)p[i];
+		CHECK(q[0] == (i & 0xff) && q[sz - 1] == (i & 0xff), "slot contents intact");
+	}
+	for (i = 0; i < POOL; i++) give(p[i]);
+	printf(fails ? "pool: FAILED (%d)\n" : "pool: ok\n", fails);
+	return fails != 0;
+}