GSNodeAllocator kept a free list per node type in C++, and every node
the list did not have came from operator new. The free list is now a C89
gs_node_pool in GSObjectPool.h and .c, beside the object pool, and
GSNodeAllocator is only the wrapper the standard containers need.
Take and give are static in the header and compile to the same
instructions as before: five for take, three for give, which is as
short as a free list gets. What changed is the empty list. Out of line,
the pool hands out the newest slab's never-used nodes in address order
and makes a 16 KB slab when that runs out. One heap call covers 185
nodes of the hash cache's 88 bytes instead of one per node, and no
pass links a slab onto the free list before it is used. Out of memory
it aborts, as operator new does without exceptions, so take has no
null check.
callgrind, 10,000 nodes filled, retaken 20 times, then 5 million
take/give pairs: the caller's code runs the same 44,795,372
instructions either way. Making nodes took about 2,000,000 instructions
in malloc and operator new before, and takes 151,489 in the refill
now: 49.47 million instructions in all became 47.53 million. Timed
(medians of 9 runs, 200,000 nodes):
- first fill: 13.0 ms to 8.4 ms;
- scattered retake: 450 ms to 367 ms, the nodes now packed in slabs;
- hot take/give: unchanged.
The slabs used to stay until exit. gs_node_pools_release frees those of
every pool whose nodes are all back, and the HW renderer calls it once
the texture cache is gone.
tests/gspool checks the node pools: nodes aligned, apart and over more
than one slab; a node given back is taken again; release keeps a pool's
slabs while one node is out and frees them once all are back. The
object pool's double-free block now skips address sanitizer builds,
where that pool hands out nothing and its takes could not succeed.
The log is the whole story: the same source address destroyed over and
over, "nothing freed", thousands of times a second, until the game
stopped. The marker added earlier caught the double free and refused to
free anything, which is why memory stayed at 5.3 GB this time - but
refusing quietly and being called again forever is a lockup of its own.
Three things, and the first is the one that matters.
The pool refuses the second give. It keeps a byte a slot saying whether
that slot is out with a caller, and a give for a slot already on the
free list is counted and ignored. Without it the slot goes on the list
twice, is handed to two callers, and one live object is quietly
overwritten by an unrelated one - which is every strange thing in this
arc, including the corrupted frames.
This is what paraLLEl-GS does not need and why. It owns images through a
refcounted handle - Vulkan::ImageHandle, recycle_image_handle takes it
by value - so a second free is not something a call site can express.
Sources and targets here are raw pointers, deleted by whoever remembers
to, and one of them remembered twice. Until that is a handle, the pool
is where it gets caught.
SourceMap::RemoveAt takes an already-dead source out of the structures
and leaves it alone rather than deleting it again.
And the marker says its piece once instead of every time.
While reading paraLLEl-GS for this: its recycling is a slab indexed by
size class - recycled_image_pool[levels][log2 h][log2 w], an array index
rather than a walk - images are returned to it in a batch at a safe
point rather than mid-draw, and when the total passes a high-water mark
it drops the lot and starts over. That is the shape our pool should be,
and it is the next thing.
tests/gspool covers it: a second give is counted, not repeated, and two
takes afterwards never hand out one slot. Passes plain and under
ASan/UBSan. GSTextureCache.cpp compiles with the Makefile's flags.
The pools ae056015c and 28d79767b added were written as C++ - static
arrays and an operator new per class - in a tree that is being converted
to C89. They are one file of C now:
GS/Renderers/HW/GSObjectPool.{c,h}, a gs_object_pool_t of fixed slots
with init, free, take and give, built by the Makefile and CMake as a C
translation unit and compiled clean with -std=c89 -pedantic -Wall
-Wextra. What is left in GSTextureCache.cpp is the pair of hooks each
class needs, one call each, because the classes are still C++; they go
when the classes do.
take and give are static functions in the header, not in the .c. That is
deliberate and it is what the codegen comparison is for: they are what a
draw pays, so out of line they would be a call the C++ they replaced did
not make.
Generated code, same compiler and flags (g++ -O2, x86-64), the C++
version against this one:
give 31 instructions 31 instructions identical
take 68 instructions 32 instructions C is smaller
calls 1 (the fallback) 1 (the fallback) same
take is smaller because the pool's one-time setup - the allocation and
the loop that fills the free list - is out of line in the .c instead of
inlined into every call site. The steady-state path is the same
sequence either way: test the free count, compare the size against the
stride, take the index, multiply, add. give compiles to the same
instructions, which is what "as fast as the C++" has to mean here.
tests/gspool now links GSObjectPool.c and exercises the real pool rather
than a copy of its arithmetic: every slot handed out once, 32-byte
aligned, none overlapping another, returned and reused across passes,
and NULL past the pool's size so the caller falls back. Plain and under
ASan/UBSan.
GSTextureCache.cpp compiles with the Makefile's flags, GSObjectPool.c as
C89. Not run.
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.
libretroadmin