From 60bc31391726438302b2c43db3317f80b22b681c Mon Sep 17 00:00:00 2001 From: Erik Date: Fri, 24 Jul 2026 11:47:44 +0200 Subject: [PATCH] perf(content): CellStruct clone guard, solid-color texture cache, drop double ToList (2026-07-24 audit review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small verified MeshExtractor.cs fixes from an allocation audit: - CellStruct clip-map clone guard: the CellStruct polygon-surface path cloned decoded texture data unconditionally before applying clip-map transparency, even though the comment above it says "if we got this from the cache, we need to clone it." The GfxObj path already had the correct pattern (clone only when textureDataIsCached). Wired the same textureDataIsCached flag through CellStruct's TryGet/GetOrCreate/ RetainOrUse call sites and gated the clone on it, matching GfxObj exactly — freshly-decoded arrays (the common case) now skip a redundant clone + BlockCopy. - Solid-color texture cache: isSolid surfaces (Base1Solid / NoPos- stippled polys) allocated a fresh 32x32 RGBA array (4 KiB) on every polygon, even for repeated colors. Added a MeshExtractor-scoped ConcurrentDictionary keyed by packed ARGB, bounded by distinct colors observed (a few hundred at most) rather than call volume. Traced every downstream consumer of the solid-color array (clip-map clone-guard, translucency clone-guard, GL upload, pak serialization) — none currently mutate a solid-color array in place, since isSolid is mutually exclusive with the texture-decode branch those guards live in — but wired textureDataIsCached = true at both call sites anyway so the existing clone-before-mutate machinery protects the shared array if that ever changes. - Dropped the redundant .ToList() on both _dats.ResolveId(...) calls: DatCollectionAdapter.ResolveId already returns a materialized List typed as IEnumerable; the immediate .OrderByDescending().FirstOrDefault() enumerates once, so the extra copy was pure waste. Added SolidColorTextureCacheTests (dat-gated, matching suite convention) exercising the new cache directly via internal visibility. Co-Authored-By: Claude Fable 5 (cherry picked from commit e9f9d7539966cc26343e45d653a751c5d1045810) --- src/AcDream.Content/MeshExtractor.cs | 52 ++++++++++++--- .../SolidColorTextureCacheTests.cs | 66 +++++++++++++++++++ 2 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 tests/AcDream.Content.Tests/SolidColorTextureCacheTests.cs diff --git a/src/AcDream.Content/MeshExtractor.cs b/src/AcDream.Content/MeshExtractor.cs index 594e815d..e880177c 100644 --- a/src/AcDream.Content/MeshExtractor.cs +++ b/src/AcDream.Content/MeshExtractor.cs @@ -40,6 +40,15 @@ public sealed class MeshExtractor { maximumEntries: 128); private readonly ThreadLocal _bcDecoder = new(() => new BcDecoder()); + // Solid-color surfaces (isSolid: Base1Solid surfaces / NoPos-stippled polys) + // bake a flat ARGB fill to a fixed 32x32 RGBA array. Distinct surface colors + // across an install are a few hundred at most, so this is bounded by DISTINCT + // COLORS observed rather than by call count — unlike DecodedTextureCache's + // byte-bounded LRU, which exists to cap large per-surface pixel arrays, not a + // handful of 4 KiB solid fills. ConcurrentDictionary: MeshExtractor is shared + // by up to MaxParallelLoads (4) decode workers. + private readonly System.Collections.Concurrent.ConcurrentDictionary _solidColorTextureCache = new(); + /// /// MP1a mechanical seam: receives particle-preload meshes staged /// mid-extraction (see ). The App @@ -74,7 +83,7 @@ public sealed class MeshExtractor { try { // Use the low 32 bits as the DAT file ID var datId = (uint)(id & 0xFFFFFFFFu); - var resolutions = _dats.ResolveId(datId).ToList(); + var resolutions = _dats.ResolveId(datId); var selectedResolution = resolutions.OrderByDescending(r => r.Database == _dats.Portal).FirstOrDefault(); if (selectedResolution == null) return null; @@ -191,7 +200,7 @@ public sealed class MeshExtractor { } ct.ThrowIfCancellationRequested(); - var resolutions = _dats.ResolveId(id).ToList(); + var resolutions = _dats.ResolveId(id); var selectedResolution = resolutions.OrderByDescending(r => r.Database == _dats.Portal).FirstOrDefault(); if (selectedResolution == null) return; @@ -377,9 +386,10 @@ public sealed class MeshExtractor { if (isSolid) { texWidth = texHeight = 32; - textureData = TextureHelpers.CreateSolidColorTexture(surface.ColorValue, texWidth, texHeight); + textureData = GetOrCreateSolidColorTexture(surface.ColorValue, texWidth, texHeight); textureFormat = TextureFormat.RGBA8; uploadPixelFormat = UploadPixelFormat.Rgba; + textureDataIsCached = true; } else if (_dats.Portal.TryGet(surface.OrigTextureId, out var surfaceTexture)) { var renderSurfaceId = surfaceTexture.Textures.First(); @@ -735,15 +745,17 @@ public sealed class MeshExtractor { bool isClipMap = surface.Type.HasFlag(SurfaceType.Base1ClipMap); uint paletteId = 0; bool isDxt3or5 = false; + bool textureDataIsCached = false; DatReaderWriter.Enums.PixelFormat? sourceFormat = null; var isAdditive = false; var isTransparent = false; if (isSolid) { texWidth = texHeight = 32; - textureData = TextureHelpers.CreateSolidColorTexture(surface.ColorValue, texWidth, texHeight); + textureData = GetOrCreateSolidColorTexture(surface.ColorValue, texWidth, texHeight); textureFormat = TextureFormat.RGBA8; uploadPixelFormat = UploadPixelFormat.Rgba; + textureDataIsCached = true; } else if (_dats.Portal.TryGet(surface.OrigTextureId, out var surfaceTexture)) { var renderSurfaceId = surfaceTexture.Textures.First(); @@ -773,6 +785,7 @@ public sealed class MeshExtractor { textureData = cachedData; textureFormat = TextureFormat.RGBA8; uploadPixelFormat = UploadPixelFormat.Rgba; + textureDataIsCached = true; } else { if (TextureHelpers.IsCompressedFormat(renderSurface.Format)) { @@ -783,7 +796,7 @@ public sealed class MeshExtractor { textureData = _decodedTextureCache.GetOrCreate( decodedTextureKey, () => DecodeCompressedTexture(renderSurface, texWidth, texHeight), - out _); + out textureDataIsCached); } else { textureFormat = TextureFormat.RGBA8; @@ -841,15 +854,18 @@ public sealed class MeshExtractor { textureData = _decodedTextureCache.RetainOrUse( decodedTextureKey, textureData, - out _); + out textureDataIsCached); } } if (isClipMap && textureData != null) { // If we got this from the cache, we need to clone it so we don't scale the cached raw data - var clonedData = new byte[textureData.Length]; - System.Buffer.BlockCopy(textureData, 0, clonedData, 0, textureData.Length); - textureData = clonedData; + if (textureDataIsCached) { + var clonedData = new byte[textureData.Length]; + System.Buffer.BlockCopy(textureData, 0, clonedData, 0, textureData.Length); + textureData = clonedData; + textureDataIsCached = false; + } for (int i = 0; i < textureData.Length; i += 4) { if (textureData[i] == 0 && textureData[i + 1] == 0 && textureData[i + 2] == 0) { @@ -1006,6 +1022,24 @@ public sealed class MeshExtractor { } } + /// + /// Returns the shared 32x32 RGBA fill for , decoding + /// once per distinct ARGB value. Both call sites (GfxObj and CellStruct isSolid + /// surfaces) hardcode 32x32 immediately before calling this, so keying purely + /// on the packed ARGB value cannot collide with a different-sized request. + /// The returned array is shared and must be treated as immutable by callers, + /// same contract as pixels. + /// internal (not private): exercised directly by + /// SolidColorTextureCacheTests (AcDream.Content.csproj grants + /// InternalsVisibleTo to AcDream.Content.Tests). + /// + internal byte[] GetOrCreateSolidColorTexture(DatReaderWriter.Types.ColorARGB color, int width, int height) { + uint key = ((uint)color.Alpha << 24) | ((uint)color.Red << 16) | ((uint)color.Green << 8) | color.Blue; + return _solidColorTextureCache.GetOrAdd( + key, + _ => TextureHelpers.CreateSolidColorTexture(color, width, height)); + } + private static DecodedTextureKey CreateDecodedTextureKey( uint renderSurfaceId, DatReaderWriter.Enums.PixelFormat format, diff --git a/tests/AcDream.Content.Tests/SolidColorTextureCacheTests.cs b/tests/AcDream.Content.Tests/SolidColorTextureCacheTests.cs new file mode 100644 index 00000000..b4b2ea3c --- /dev/null +++ b/tests/AcDream.Content.Tests/SolidColorTextureCacheTests.cs @@ -0,0 +1,66 @@ +using AcDream.Content; +using DatReaderWriter; +using DatReaderWriter.Options; +using DatReaderWriter.Types; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace AcDream.Content.Tests; + +/// +/// MeshExtractor.GetOrCreateSolidColorTexture (2026-07-24 audit review, Finding +/// D): solid-color surfaces bake a 32x32 RGBA fill and previously allocated a +/// fresh array on every call. These tests exercise the cache directly (internal +/// visibility via InternalsVisibleTo) rather than through the full GfxObj/ +/// CellStruct extraction pipeline, so they don't need fixture ids — only a +/// working instance, which still needs real dats to +/// construct (its ctor builds a RetailPhysicsScriptLoader from dats.Portal). +/// Skips cleanly when dats are absent, matching ContentConformanceDats convention. +/// +public sealed class SolidColorTextureCacheTests { + [Fact] + public void GetOrCreateSolidColorTexture_SameColor_ReturnsSameSharedArray() { + var datDir = ContentConformanceDats.ResolveDatDir(); + if (datDir is null) return; // dats absent (CI) — skip, matching suite convention + + using var dats = new DatCollection(datDir, DatAccessType.Read); + using var adapter = new DatCollectionAdapter(dats); + var extractor = new MeshExtractor(adapter, NullLogger.Instance, sideStagedSink: null); + + var color = new ColorARGB { Alpha = 255, Red = 10, Green = 20, Blue = 30 }; + + var first = extractor.GetOrCreateSolidColorTexture(color, 32, 32); + var second = extractor.GetOrCreateSolidColorTexture(color, 32, 32); + + // Same ARGB value must hit the cache and return the SAME array instance, + // not merely an equal one — that's the allocation this cache exists to avoid. + Assert.Same(first, second); + Assert.Equal(32 * 32 * 4, first.Length); + Assert.Equal(10, first[0]); + Assert.Equal(20, first[1]); + Assert.Equal(30, first[2]); + Assert.Equal(255, first[3]); + } + + [Fact] + public void GetOrCreateSolidColorTexture_DifferentColors_ReturnDistinctArrays() { + var datDir = ContentConformanceDats.ResolveDatDir(); + if (datDir is null) return; // dats absent (CI) — skip, matching suite convention + + using var dats = new DatCollection(datDir, DatAccessType.Read); + using var adapter = new DatCollectionAdapter(dats); + var extractor = new MeshExtractor(adapter, NullLogger.Instance, sideStagedSink: null); + + var red = new ColorARGB { Alpha = 255, Red = 255, Green = 0, Blue = 0 }; + var blue = new ColorARGB { Alpha = 255, Red = 0, Green = 0, Blue = 255 }; + + var redTexture = extractor.GetOrCreateSolidColorTexture(red, 32, 32); + var blueTexture = extractor.GetOrCreateSolidColorTexture(blue, 32, 32); + + Assert.NotSame(redTexture, blueTexture); + Assert.Equal(255, redTexture[0]); + Assert.Equal(0, redTexture[2]); + Assert.Equal(0, blueTexture[0]); + Assert.Equal(255, blueTexture[2]); + } +}