From 8dec163fd84d4a8f6ff57dc4bc0ec4d1e9be7220 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 27 Jul 2026 23:00:28 +0200 Subject: [PATCH] fix(render): map the GL frame ring instead of glBufferSubData Campaign V's V4c and V4d were reverted because the connected world went blank roughly one launch in three, with no GL error anywhere and every added CPU-GPU sync point suppressing it. The plan's section 5.5 records the best-supported cause and makes this change binding before either slice may re-land: the frame ring performed 10-40 partial glBufferSubData updates per frame into a buffer object that already-submitted same-frame draws were still reading, and the offline path that passed every gate issues only 2-4. That is the offline versus connected axis, stated exactly. A partial glBufferSubData into an in-use buffer does not have one defined implementation. The driver may stall, may rename the whole data store and copy the untouched remainder forward, or may route the write through an internal staging copy, and which one it picks is a heuristic fed by the update pattern. glMapBufferRange with GL_MAP_WRITE_BIT, GL_MAP_UNSYNCHRONIZED_BIT and GL_MAP_INVALIDATE_RANGE_BIT removes the guess. The three bits say "I am writing this range", "I am overwriting all of it", and "nothing in flight reads it" - which is the ring's actual invariant rather than something the driver has to infer. GlGpuBuffer.WriteRangeUnsynchronized is that write, and the ring no longer calls Upload at all. Upload itself stays, synchronized, for the writers whose ordering really is the driver's job: the mesh arena and texture staging. The unsynchronized bit is an assertion, so the two invariants behind it are now enforced rather than merely true. Across frames it belongs to GpuFrameFlightController, which waits on a slot's fence in BeginFrame before GlRingBufferState.Reset rewinds that slot. Within a frame it belongs to the allocation cursor, which only moves forward, so each flush covers bytes strictly above every byte already flushed. GlRingBufferState now carries the flushed high-water mark explicitly and refuses a write below it, so a future change that reused ring bytes mid-frame fails loudly here instead of producing an undefined read on the GPU. MarkDirty is internal for the same reason AlignUp already was: the guard is unreachable through Allocate by construction, and proving it fires needs a direct call. The texture handle table moved too, because it is the only other buffer this backend rewrites while the frame's own draws are in flight, and leaving one partial glBufferSubData in the pre-draw flush would have left a live instance of the same mechanism sitting inside the very function this change exists to fix. It cannot use the ring's single merged span: two registrations in one frame can land on slots 5 and 50 with forty-four live slots between them, and a mapped invalidating write over that whole span would let the driver discard live bindless handles a submitted draw is reading. GlDirtySlotRuns therefore drains the table one run of consecutive dirty slots at a time. Every slot in a run is safe on its own terms: RegisterTexture writes a slot fresh from the allocator that no batch has ever indexed, and ReleaseTextureSlot's zeroing write already runs inside a retirement callback, after the fence covering every frame that could still reference it. Nothing about renderer-visible behaviour changes. No renderer, no shader and no CPU data layout is touched; only how the same bytes reach the same buffers. SupportsPersistentlyMappedRings stays false, since a map-per-flush is not a persistent mapping - its comment was rewritten because it claimed the backend never writes into mapped memory, which is no longer true. Gates. Release build green. App tests 3,862 passed / 3 skipped, against a 3,846 / 3 baseline measured on this tree plus the 16 tests added here (one full-suite baseline run failed WorldRenderFrameBuilder's runtime-root-source test, which passes alone and passed on the rerun - a pre-existing ordering flake, not a regression). Offline pixel gate against 61f3c5d8: 30 differing pixels of 563,200 compared, a fraction of 5.33e-05, nineteen times under the 0.001 threshold. Four captures were taken to bound the noise rather than assume it: two same-commit control pairs differ by 15 and 12 pixels, and the three cross-capture pairs by 30, 27 and 30, with comparable maximum channel deltas throughout. The difference is capture noise in the animated surfaces, not a rendering change. This commit is the precondition, not the re-land. V4c follows as a revert-of-its-revert on top of this ring, gated by the repeat-run connected gate at ten of ten rendered. No divergence-register row: this changes no retail-facing behaviour. Co-Authored-By: Claude Fable 5 --- .../Rendering/Gpu/Gl/GlDirtySlotRuns.cs | 108 ++++++++++ .../Rendering/Gpu/Gl/GlGpuBuffer.cs | 93 +++++++- .../Rendering/Gpu/Gl/GlGpuDevice.cs | 69 +++--- .../Rendering/Gpu/Gl/GlRingBufferState.cs | 73 +++++-- .../Rendering/Gpu/Gl/GlDirtySlotRunsTests.cs | 198 ++++++++++++++++++ .../Gpu/Gl/GlRingBufferStateTests.cs | 70 +++++++ 6 files changed, 566 insertions(+), 45 deletions(-) create mode 100644 src/AcDream.App/Rendering/Gpu/Gl/GlDirtySlotRuns.cs create mode 100644 tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlDirtySlotRunsTests.cs diff --git a/src/AcDream.App/Rendering/Gpu/Gl/GlDirtySlotRuns.cs b/src/AcDream.App/Rendering/Gpu/Gl/GlDirtySlotRuns.cs new file mode 100644 index 00000000..bc0766a4 --- /dev/null +++ b/src/AcDream.App/Rendering/Gpu/Gl/GlDirtySlotRuns.cs @@ -0,0 +1,108 @@ +namespace AcDream.App.Rendering.Gpu.Gl; + +/// +/// Tracks which texture-table slots changed since the last flush and gives them +/// back as maximal runs of consecutive dirty slots. +/// +/// Why runs, and not one merged range. The ring can flush a single +/// span covering everything written since the last draw, because every byte in +/// that span was allocated this frame and nothing in flight reads it. The +/// texture table cannot: it is a long-lived array of bindless handles, and two +/// registrations in one frame can land on slots 5 and 50 with forty-four live +/// slots in between. Those live slots are read by draws already submitted this +/// frame, so a single mapped write over [5, 51) — mapped with +/// GL_MAP_INVALIDATE_RANGE_BIT, which lets the driver discard the whole +/// range's contents while it is mapped, and GL_MAP_UNSYNCHRONIZED_BIT, +/// which stops it from waiting first — would expose those draws to torn +/// handles. Splitting on the gaps keeps every mapped range made only of slots +/// that no submitted draw can be reading. +/// +/// Why a dirty slot is safe to write unsynchronized. Two producers +/// touch a slot: GlGpuDevice.RegisterTexture, which writes a slot fresh +/// from and therefore one no batch has ever +/// indexed; and ReleaseTextureSlot's zeroing write, which runs inside a +/// retirement callback, i.e. after the fence covering every frame that could +/// still have referenced it. Both are already past the point where the GPU can +/// read the old value — which is exactly the assertion +/// GL_MAP_UNSYNCHRONIZED_BIT makes. +/// +/// GL-free by design, like and +/// , so the bookkeeping is unit tested +/// without a context. Enumeration is allocation-free: the tracker keeps the +/// min/max window that has been dirtied so a clean table costs one comparison, +/// and a dirty one scans only that window. +/// +internal sealed class GlDirtySlotRuns +{ + private readonly bool[] _dirty; + private int _windowStart = -1; + private int _windowEnd = -1; + + public GlDirtySlotRuns(uint capacity) + { + ArgumentOutOfRangeException.ThrowIfZero(capacity); + _dirty = new bool[capacity]; + } + + public uint Capacity => (uint)_dirty.Length; + + public bool HasDirtySlots => _windowStart >= 0; + + public void Mark(uint slot) + { + if (slot >= (uint)_dirty.Length) + { + throw new ArgumentOutOfRangeException( + nameof(slot), + slot, + $"The tracked table holds {_dirty.Length} slots."); + } + + _dirty[slot] = true; + _windowStart = _windowStart < 0 ? (int)slot : Math.Min(_windowStart, (int)slot); + _windowEnd = Math.Max(_windowEnd, (int)slot + 1); + } + + /// + /// Takes the lowest remaining run of consecutive dirty slots, clearing it, + /// and reports its first slot and length. Returns false once none + /// remain, so a caller drains with a while loop. + /// + public bool TryTakeNextRun(out uint firstSlot, out uint slotCount) + { + firstSlot = 0; + slotCount = 0; + if (_windowStart < 0) + return false; + + int index = _windowStart; + while (index < _windowEnd && !_dirty[index]) + index++; + if (index >= _windowEnd) + { + CloseWindow(); + return false; + } + + int start = index; + while (index < _windowEnd && _dirty[index]) + { + _dirty[index] = false; + index++; + } + + firstSlot = (uint)start; + slotCount = (uint)(index - start); + if (index >= _windowEnd) + CloseWindow(); + else + _windowStart = index; + return true; + } + + private void CloseWindow() + { + _windowStart = -1; + _windowEnd = -1; + } +} diff --git a/src/AcDream.App/Rendering/Gpu/Gl/GlGpuBuffer.cs b/src/AcDream.App/Rendering/Gpu/Gl/GlGpuBuffer.cs index e37a8628..63ce8249 100644 --- a/src/AcDream.App/Rendering/Gpu/Gl/GlGpuBuffer.cs +++ b/src/AcDream.App/Rendering/Gpu/Gl/GlGpuBuffer.cs @@ -15,9 +15,13 @@ namespace AcDream.App.Rendering.Gpu.Gl; /// surface without forking per usage. /// /// Allocated once at the description's size via glBufferData with null -/// data; every write after that is glBufferSubData — no persistent -/// mapping, matching the campaign's "GL backend is deliberately behaviour- -/// preserving" rule for this slice. +/// data. There are two write paths after that, and which one applies is a +/// property of the buffer's role rather than of the buffer object: +/// is an ordinary synchronized glBufferSubData, used +/// for one-off and streaming writes the driver must order for us (the mesh +/// arena, texture staging); maps the +/// range and is used only by the per-frame upload ring, whose non-overlap +/// invariant the caller can state. Neither path holds a persistent mapping. /// /// The glBufferData usage hint follows /// : @@ -136,6 +140,89 @@ internal sealed class GlGpuBuffer : IGpuBuffer _gl.BindBuffer(GLEnum.CopyWriteBuffer, 0); } + /// + /// Writes at through + /// glMapBufferRange with + /// GL_MAP_WRITE_BIT | GL_MAP_UNSYNCHRONIZED_BIT | GL_MAP_INVALIDATE_RANGE_BIT + /// — the canonical GL upload-ring idiom, and the only write path the + /// per-frame ring uses. + /// + /// Why not . A partial glBufferSubData + /// into a buffer object that already-submitted draws are still reading leaves + /// the driver to guess how to keep both truths alive: it can stall, it can + /// rename the whole data store and copy the untouched remainder forward, or + /// it can route the write through an internal staging copy. Which one it + /// picks is a heuristic, and the heuristic is fed by the update pattern — + /// Campaign V's V4c revert (plan §5.5) saw a world that rendered blank + /// roughly one launch in three, with no GL error at any point, on the + /// connected path that issues 10–40 such partial updates per frame, and zero + /// times on the offline path that issues 2–4. Mapping the range instead + /// removes the guess: the three bits together say "I am writing this range, + /// I am overwriting all of it, and nothing in flight reads it," which is + /// exactly the ring's actual invariant. + /// + /// The precondition is the caller's to prove. + /// GL_MAP_UNSYNCHRONIZED_BIT means the driver inserts no wait, so the + /// caller asserts that no GL command already submitted and not yet complete + /// reads any byte of [offsetBytes, offsetBytes + data.Length). The ring + /// has that invariant on both axes: within a frame the allocation cursor only + /// moves forward and refuses a write below the + /// flushed high-water mark, and across frames a slot is only rewritten after + /// GpuFrameFlightController.BeginFrame has waited on the fence for the + /// last frame that used it. GL_MAP_INVALIDATE_RANGE_BIT is likewise + /// only sound because every byte of the mapped range is then written. + /// + /// A false return from glUnmapBuffer means the data store + /// was lost while mapped (a GPU reset or display-mode change), so the write + /// did not land. That throws rather than being retried: the frame's uploads + /// are already partly gone, and silently continuing would draw from a + /// half-written ring. + /// + internal unsafe void WriteRangeUnsynchronized(long offsetBytes, ReadOnlySpan data, string context) + { + ThrowIfDisposed(); + if (offsetBytes < 0 || offsetBytes + data.Length > SizeBytes) + { + throw new ArgumentOutOfRangeException( + nameof(offsetBytes), + $"Mapped write of {data.Length} bytes at offset {offsetBytes} exceeds buffer '{Name}' ({SizeBytes} bytes)."); + } + if (data.IsEmpty) + return; + + _gl.BindBuffer(GLEnum.CopyWriteBuffer, _name); + void* mapped = _gl.MapBufferRange( + GLEnum.CopyWriteBuffer, + (nint)offsetBytes, + (nuint)data.Length, + MapBufferAccessMask.WriteBit + | MapBufferAccessMask.UnsynchronizedBit + | MapBufferAccessMask.InvalidateRangeBit); + if (mapped is null) + { + // A null return always sets a GL error, so surface that first — it + // names the actual rejection instead of the symptom. + GLHelpers.ThrowOnResourceError( + _gl, + $"map {data.Length} bytes of buffer '{Name}' at offset {offsetBytes} ({context})"); + throw new InvalidOperationException( + $"glMapBufferRange returned no pointer for {data.Length} bytes of buffer '{Name}' " + + $"at offset {offsetBytes} ({context})."); + } + + data.CopyTo(new Span(mapped, data.Length)); + + bool unmapped = _gl.UnmapBuffer(GLEnum.CopyWriteBuffer); + GLHelpers.ThrowOnResourceError(_gl, $"unmap buffer '{Name}' after writing {context}"); + _gl.BindBuffer(GLEnum.CopyWriteBuffer, 0); + if (!unmapped) + { + throw new InvalidOperationException( + $"Buffer '{Name}' lost its data store while {data.Length} bytes at offset " + + $"{offsetBytes} ({context}) were mapped; the write did not land."); + } + } + public void CopyTo(IGpuBuffer destination, long sourceOffsetBytes, long destinationOffsetBytes, long byteCount) { ThrowIfDisposed(); diff --git a/src/AcDream.App/Rendering/Gpu/Gl/GlGpuDevice.cs b/src/AcDream.App/Rendering/Gpu/Gl/GlGpuDevice.cs index dd329df8..aeeafba4 100644 --- a/src/AcDream.App/Rendering/Gpu/Gl/GlGpuDevice.cs +++ b/src/AcDream.App/Rendering/Gpu/Gl/GlGpuDevice.cs @@ -33,10 +33,21 @@ namespace AcDream.App.Rendering.Gpu.Gl; /// piece that enforces this. /// /// Flush discipline. Ring bytes and the texture-handle table are -/// both flushed with one glBufferSubData each, immediately before -/// every Draw/DrawIndexed/MultiDrawIndexedIndirect — see +/// both flushed immediately before every +/// Draw/DrawIndexed/MultiDrawIndexedIndirect — see /// — never at bind time, so a renderer that -/// writes after binding still uploads correctly. +/// writes after binding still uploads correctly. Both flushes go through +/// , i.e. +/// glMapBufferRange with +/// GL_MAP_WRITE_BIT | GL_MAP_UNSYNCHRONIZED_BIT | GL_MAP_INVALIDATE_RANGE_BIT, +/// rather than glBufferSubData: these are the only two buffers this +/// backend rewrites while the frame's own draws are still in flight, and that +/// is precisely the case where a partial glBufferSubData leaves the +/// result to a driver heuristic. See that method's remarks and campaign plan +/// §5.5 for the failure it produced. The two flushes differ in shape because +/// their non-overlap proofs differ: the ring writes one span (its cursor only +/// moves forward within a frame), the table writes one span per run of +/// consecutive dirty slots (see ). /// internal sealed class GlGpuDevice : IGpuDevice { @@ -64,8 +75,8 @@ internal sealed class GlGpuDevice : IGpuDevice new(GpuBindingModel.TextureTableCapacity); private readonly ulong[] _textureHandleTable = new ulong[GpuBindingModel.TextureTableCapacity]; private readonly GlGpuBuffer _textureTableBuffer; - private int _textureTableDirtyStart = -1; - private int _textureTableDirtyEnd = -1; + private readonly GlDirtySlotRuns _textureTableDirtySlots = + new(GpuBindingModel.TextureTableCapacity); private readonly GlRenderStateCache _renderState = new(); private readonly GlGpuPushConstantBinder _pushConstants; @@ -260,28 +271,37 @@ internal sealed class GlGpuDevice : IGpuDevice } /// - /// Uploads this slot's dirty ring bytes and the texture table's dirty - /// range (if any) with one glBufferSubData each, then clears both - /// watermarks. Called immediately before every draw — never at bind time, - /// because a renderer may still write into a ring allocation after - /// binding it. + /// Writes this slot's dirty ring bytes and every dirty run of the texture + /// table into their GL buffers, then clears both watermarks. Called + /// immediately before every draw — never at bind time, because a renderer + /// may still write into a ring allocation after binding it. + /// + /// Both writes are mapped and unsynchronized (see the class remarks and + /// ). The ring's dirty + /// span may include alignment padding between allocations; those bytes are + /// below the cursor, were allocated this frame, and are read by nothing, so + /// covering them in one map is sound and saves a call. The table has no + /// equivalent licence, which is why it drains run by run. /// internal void FlushBeforeDraw(int slotIndex) { (int start, int length) = _ringStates[slotIndex].TakeDirtyRange(); if (length > 0) - _ringBuffers[slotIndex].Upload(start, _ringStaging[slotIndex].AsSpan(start, length)); - - if (_textureTableDirtyStart >= 0) { - int tableStart = _textureTableDirtyStart; - int tableEnd = _textureTableDirtyEnd; - _textureTableDirtyStart = -1; - _textureTableDirtyEnd = -1; + _ringBuffers[slotIndex].WriteRangeUnsynchronized( + start, + _ringStaging[slotIndex].AsSpan(start, length), + $"ring slot {slotIndex}"); + } + while (_textureTableDirtySlots.TryTakeNextRun(out uint firstSlot, out uint slotCount)) + { ReadOnlySpan bytes = MemoryMarshal.AsBytes( - _textureHandleTable.AsSpan(tableStart, tableEnd - tableStart)); - _textureTableBuffer.Upload((long)tableStart * sizeof(ulong), bytes); + _textureHandleTable.AsSpan((int)firstSlot, (int)slotCount)); + _textureTableBuffer.WriteRangeUnsynchronized( + (long)firstSlot * sizeof(ulong), + bytes, + $"texture table slots {firstSlot}..{firstSlot + slotCount - 1}"); } } @@ -531,10 +551,7 @@ internal sealed class GlGpuDevice : IGpuDevice private void WriteHandle(uint slot, ulong handle) { _textureHandleTable[slot] = handle; - _textureTableDirtyStart = _textureTableDirtyStart < 0 - ? (int)slot - : Math.Min(_textureTableDirtyStart, (int)slot); - _textureTableDirtyEnd = Math.Max(_textureTableDirtyEnd, (int)slot + 1); + _textureTableDirtySlots.Mark(slot); } private GpuCapabilityRecord CaptureCapabilities() @@ -568,9 +585,9 @@ internal sealed class GlGpuDevice : IGpuDevice SupportsDrawParameters = _gl.IsExtensionPresent("GL_ARB_shader_draw_parameters"), SupportsTextureCompressionBc = _gl.IsExtensionPresent("GL_EXT_texture_compression_s3tc"), SupportsTimestampQueries = timerQuery, - // The GL backend deliberately never writes into mapped memory — - // it keeps BufferSubData uploads so every renderer port slice's - // pixel gate is a strict identity check. Only Vulkan reports true. + // The ring maps and unmaps per flush; it never holds a persistent + // mapping a renderer could write into between frames, which is what + // this capability advertises. Only Vulkan reports true. SupportsPersistentlyMappedRings = false, }; } diff --git a/src/AcDream.App/Rendering/Gpu/Gl/GlRingBufferState.cs b/src/AcDream.App/Rendering/Gpu/Gl/GlRingBufferState.cs index 1c7caca6..1771013d 100644 --- a/src/AcDream.App/Rendering/Gpu/Gl/GlRingBufferState.cs +++ b/src/AcDream.App/Rendering/Gpu/Gl/GlRingBufferState.cs @@ -2,22 +2,38 @@ namespace AcDream.App.Rendering.Gpu.Gl; /// /// Pure bookkeeping for one flight slot's upload ring: an allocation cursor -/// that only grows across a frame, plus a separate "dirty" watermark that -/// tracks the byte range written since the last flush. +/// that only grows across a frame, a "dirty" watermark tracking the byte range +/// written since the last flush, and the flushed high-water mark that makes the +/// ring's write path safe to issue unsynchronized. /// /// This is deliberately GL-free so it can be unit tested without a live -/// context. owns one instance per flight slot and -/// pairs it with a managed staging byte[] and a real GL buffer; the -/// staging array receives every write, -/// and immediately before each Draw/DrawIndexed/ -/// MultiDrawIndexedIndirect the device uploads exactly the dirty range -/// via one glBufferSubData call and calls -/// to reset the watermark. The allocation cursor itself only resets at -/// (once per BeginFrame) — writes made after a -/// flush simply extend the dirty range again, to be picked up by the next -/// flush. This is what makes "flush before every draw, not at bind time" -/// correct: a renderer that writes after binding still gets uploaded before -/// its draw call runs. +/// context. 's device owns one instance per flight slot +/// and pairs it with a managed staging byte[] and a real GL buffer; the +/// staging array receives every write, and +/// immediately before each Draw/DrawIndexed/ +/// MultiDrawIndexedIndirect the device writes exactly the dirty range +/// into the GL buffer through +/// and calls +/// to reset the watermark. The allocation cursor +/// itself only resets at (once per BeginFrame) — +/// writes made after a flush simply extend the dirty range again, to be picked +/// up by the next flush. This is what makes "flush before every draw, not at +/// bind time" correct: a renderer that writes after binding still gets uploaded +/// before its draw call runs. +/// +/// The forward-only invariant is load-bearing, not incidental. The +/// device's per-flush write is mapped with GL_MAP_UNSYNCHRONIZED_BIT, +/// which means the driver inserts no wait and the caller is asserting that no +/// submitted-and-unfinished draw reads the range. Within a frame that holds +/// because only ever hands out bytes at or above the +/// cursor, so each flush covers a range strictly above every range already +/// flushed. Rather than leave that as an emergent property of the arithmetic, +/// refuses a write below : +/// a future change that reused ring bytes mid-frame would fail loudly here +/// instead of producing an undefined read on the GPU. Across frames the +/// invariant belongs to GpuFrameFlightController, which waits on a +/// slot's fence in BeginFrame before this state is . +/// /// internal sealed class GlRingBufferState { @@ -25,6 +41,7 @@ internal sealed class GlRingBufferState private uint _cursor; private int _dirtyStart = -1; private int _dirtyEnd = -1; + private int _flushedEnd; public GlRingBufferState(int capacityBytes) { @@ -37,6 +54,12 @@ internal sealed class GlRingBufferState /// Bytes handed out since the last . public uint AllocatedBytes => _cursor; + /// + /// The exclusive end of the bytes already handed to the GPU this frame. + /// Every subsequent write must start at or above it. + /// + public int FlushedEndBytes => _flushedEnd; + /// /// Reserves bytes aligned to /// , returning the aligned offset. @@ -69,11 +92,13 @@ internal sealed class GlRingBufferState _cursor = 0; _dirtyStart = -1; _dirtyEnd = -1; + _flushedEnd = 0; } /// /// Returns the byte range written since the last flush (start, length), - /// or (0, 0) when nothing is dirty, and clears the watermark. + /// or (0, 0) when nothing is dirty, and clears the watermark while + /// advancing over it. /// public (int Start, int Length) TakeDirtyRange() { @@ -81,6 +106,7 @@ internal sealed class GlRingBufferState return (0, 0); (int start, int length) = (_dirtyStart, _dirtyEnd - _dirtyStart); + _flushedEnd = Math.Max(_flushedEnd, _dirtyEnd); _dirtyStart = -1; _dirtyEnd = -1; return (start, length); @@ -88,8 +114,23 @@ internal sealed class GlRingBufferState public bool HasDirtyBytes => _dirtyStart >= 0; - private void MarkDirty(int start, int end) + /// + /// Records that [start, end) was written. Internal rather than + /// private for the same reason is: the forward-only + /// guard below is unreachable through by + /// construction, so proving it fires at all needs a direct call. + /// + internal void MarkDirty(int start, int end) { + if (start < _flushedEnd) + { + throw new InvalidOperationException( + $"Ring write [{start}, {end}) reaches below the {_flushedEnd} bytes already " + + "uploaded this frame. Ring uploads are issued with GL_MAP_UNSYNCHRONIZED_BIT, " + + "so rewriting bytes a submitted draw may still be reading is undefined — the " + + "allocation cursor must only move forward until Reset."); + } + _dirtyStart = _dirtyStart < 0 ? start : Math.Min(_dirtyStart, start); _dirtyEnd = Math.Max(_dirtyEnd, end); } diff --git a/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlDirtySlotRunsTests.cs b/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlDirtySlotRunsTests.cs new file mode 100644 index 00000000..1e67e2da --- /dev/null +++ b/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlDirtySlotRunsTests.cs @@ -0,0 +1,198 @@ +using AcDream.App.Rendering.Gpu.Gl; + +namespace AcDream.App.Tests.Rendering.Gpu.Gl; + +/// +/// The texture table's flush is mapped with GL_MAP_UNSYNCHRONIZED_BIT and +/// GL_MAP_INVALIDATE_RANGE_BIT, so a run this tracker reports must contain +/// nothing but slots that were actually written. A run that swallowed a clean +/// slot in between would let the driver discard a live bindless handle while a +/// submitted draw was reading it — invisible in any pixel gate, and exactly the +/// class of fault the ring rewrite exists to remove. These tests pin that. +/// +public sealed class GlDirtySlotRunsTests +{ + [Fact] + public void NothingMarkedYieldsNoRuns() + { + var runs = new GlDirtySlotRuns(16); + + Assert.False(runs.HasDirtySlots); + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void OneMarkedSlotIsOneRunOfOne() + { + var runs = new GlDirtySlotRuns(16); + runs.Mark(7); + + Assert.True(runs.HasDirtySlots); + Assert.True(runs.TryTakeNextRun(out uint first, out uint count)); + Assert.Equal(7u, first); + Assert.Equal(1u, count); + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void ConsecutiveSlotsMergeIntoOneRun() + { + var runs = new GlDirtySlotRuns(16); + runs.Mark(4); + runs.Mark(5); + runs.Mark(6); + + Assert.True(runs.TryTakeNextRun(out uint first, out uint count)); + Assert.Equal(4u, first); + Assert.Equal(3u, count); + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void AGapBetweenMarkedSlotsSplitsTheRuns() + { + var runs = new GlDirtySlotRuns(64); + runs.Mark(5); + runs.Mark(50); + + Assert.True(runs.TryTakeNextRun(out uint firstStart, out uint firstCount)); + Assert.Equal(5u, firstStart); + Assert.Equal(1u, firstCount); + + Assert.True(runs.TryTakeNextRun(out uint secondStart, out uint secondCount)); + Assert.Equal(50u, secondStart); + Assert.Equal(1u, secondCount); + + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void RunsComeBackInAscendingSlotOrderRegardlessOfMarkOrder() + { + var runs = new GlDirtySlotRuns(64); + runs.Mark(40); + runs.Mark(1); + runs.Mark(41); + runs.Mark(20); + runs.Mark(0); + + Assert.True(runs.TryTakeNextRun(out uint start, out uint count)); + Assert.Equal(0u, start); + Assert.Equal(2u, count); + + Assert.True(runs.TryTakeNextRun(out start, out count)); + Assert.Equal(20u, start); + Assert.Equal(1u, count); + + Assert.True(runs.TryTakeNextRun(out start, out count)); + Assert.Equal(40u, start); + Assert.Equal(2u, count); + + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void MarkingTheSameSlotTwiceStillYieldsOneRun() + { + var runs = new GlDirtySlotRuns(16); + runs.Mark(3); + runs.Mark(3); + + Assert.True(runs.TryTakeNextRun(out uint start, out uint count)); + Assert.Equal(3u, start); + Assert.Equal(1u, count); + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void DrainingLeavesTheTrackerCleanForTheNextFlush() + { + var runs = new GlDirtySlotRuns(16); + runs.Mark(2); + runs.Mark(9); + while (runs.TryTakeNextRun(out _, out _)) + { + } + + Assert.False(runs.HasDirtySlots); + + runs.Mark(11); + Assert.True(runs.TryTakeNextRun(out uint start, out uint count)); + Assert.Equal(11u, start); + Assert.Equal(1u, count); + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void SlotsMarkedAfterAPartialDrainAreStillReported() + { + var runs = new GlDirtySlotRuns(32); + runs.Mark(4); + runs.Mark(20); + + Assert.True(runs.TryTakeNextRun(out uint start, out uint count)); + Assert.Equal(4u, start); + Assert.Equal(1u, count); + + // A draw between two flushes can register another texture. + runs.Mark(21); + + Assert.True(runs.TryTakeNextRun(out start, out count)); + Assert.Equal(20u, start); + Assert.Equal(2u, count); + Assert.False(runs.TryTakeNextRun(out _, out _)); + } + + [Fact] + public void TheLastSlotInTheTableIsAddressable() + { + var runs = new GlDirtySlotRuns(8); + runs.Mark(7); + + Assert.True(runs.TryTakeNextRun(out uint start, out uint count)); + Assert.Equal(7u, start); + Assert.Equal(1u, count); + } + + [Fact] + public void MarkingBeyondCapacityThrows() + { + var runs = new GlDirtySlotRuns(8); + Assert.Throws(() => runs.Mark(8)); + } + + [Fact] + public void ZeroCapacityIsRejected() + { + Assert.Throws(() => new GlDirtySlotRuns(0)); + } + + /// + /// The whole point, stated as one property: every slot a run covers was + /// marked, and every marked slot is covered exactly once. + /// + [Fact] + public void EveryReportedSlotWasMarkedAndEveryMarkedSlotIsReportedOnce() + { + const int Capacity = 200; + var random = new Random(20260727); + var expected = new HashSet(); + var runs = new GlDirtySlotRuns(Capacity); + for (int i = 0; i < 60; i++) + { + uint slot = (uint)random.Next(Capacity); + expected.Add(slot); + runs.Mark(slot); + } + + var reported = new List(); + while (runs.TryTakeNextRun(out uint start, out uint count)) + { + for (uint slot = start; slot < start + count; slot++) + reported.Add(slot); + } + + Assert.Equal(reported.Count, reported.Distinct().Count()); + Assert.Equal(expected.OrderBy(slot => slot), reported); + } +} diff --git a/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlRingBufferStateTests.cs b/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlRingBufferStateTests.cs index 163aa76a..4bf01f18 100644 --- a/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlRingBufferStateTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Gpu/Gl/GlRingBufferStateTests.cs @@ -63,6 +63,76 @@ public sealed class GlRingBufferStateTests Assert.Equal(8, length); } + /// + /// The precondition for issuing the ring's writes with + /// GL_MAP_UNSYNCHRONIZED_BIT: within one frame, no flush may cover a byte an + /// earlier flush already handed to the GPU. This walks a frame's worth of + /// mixed-alignment allocations and asserts the ranges are disjoint and + /// ascending. + /// + [Fact] + public void SuccessiveDirtyRangesWithinAFrameNeverOverlap() + { + var state = new GlRingBufferState(64 * 1024); + int previousEnd = 0; + + foreach (int size in new[] { 100, 4, 1024, 36, 7, 512, 3 }) + { + state.Allocate(size, alignmentBytes: 256); + state.Allocate(size, alignmentBytes: 4); + + (int start, int length) = state.TakeDirtyRange(); + Assert.True( + start >= previousEnd, + $"flush [{start}, {start + length}) reaches below the already-flushed {previousEnd} bytes"); + previousEnd = start + length; + Assert.Equal(previousEnd, state.FlushedEndBytes); + } + } + + [Fact] + public void AWriteBelowTheFlushedHighWaterMarkIsRefused() + { + var state = new GlRingBufferState(1024); + state.Allocate(128, alignmentBytes: 4); + state.TakeDirtyRange(); + + Assert.Equal(128, state.FlushedEndBytes); + InvalidOperationException error = Assert.Throws( + () => state.MarkDirty(64, 96)); + Assert.Contains("GL_MAP_UNSYNCHRONIZED_BIT", error.Message, StringComparison.Ordinal); + } + + [Fact] + public void FlushedHighWaterMarkOnlyAdvancesWhenSomethingWasFlushed() + { + var state = new GlRingBufferState(1024); + state.Allocate(48, alignmentBytes: 4); + state.TakeDirtyRange(); + Assert.Equal(48, state.FlushedEndBytes); + + // A draw with nothing newly written must not move the mark, or the next + // allocation's guard would compare against a number no flush produced. + state.TakeDirtyRange(); + Assert.Equal(48, state.FlushedEndBytes); + } + + [Fact] + public void ResetClearsTheFlushedHighWaterMarkSoTheSlotIsWritableAgain() + { + var state = new GlRingBufferState(1024); + state.Allocate(256, alignmentBytes: 4); + state.TakeDirtyRange(); + Assert.Equal(256, state.FlushedEndBytes); + + // BeginFrame has waited on this slot's fence by the time Reset runs, so + // the whole slot is writable from zero again. + state.Reset(); + + Assert.Equal(0, state.FlushedEndBytes); + Assert.Equal(0u, state.Allocate(64, alignmentBytes: 4)); + } + [Fact] public void ResetRewindsTheCursorAndClearsDirtyState() {