From 40499d5dc3ce602ba7a6d2d6909b0d94178745c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 4 Nov 2026 19:33:58 +0000 Subject: [PATCH] Add ContiguousMap.CopyKeysTo/CopyValuesTo and obsolete the copying *Span accessors [minor] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetKeysSpan and GetValuesSpan read as zero-copy views, but every call allocated a new array and copied the whole map, so an indexed loop over them was O(n²). CopyKeysTo(Span) and CopyValuesTo(Span) fill a caller-owned buffer without allocating. The *Span methods keep working, now through those, but are marked [Obsolete] (a warning, not an error) pointing at the replacements and at AsReadOnlySpan() for zero-copy access to Entry.Key/Value. The ContiguousMap span-access benchmark now measures AsReadOnlySpan, the real zero-copy path, instead of the copy. Fixes ktsu-dev/Containers#102 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW6AfToAg6F8bcZwwZ5 --- .../ContiguousMapBenchmarks.cs | 7 +- Containers.Test/ContiguousMapTests.cs | 107 ++++++++++++++++++ Containers/ContiguousMap.cs | 80 ++++++++++--- 3 files changed, 175 insertions(+), 19 deletions(-) diff --git a/Containers.Benchmarks/ContiguousMapBenchmarks.cs b/Containers.Benchmarks/ContiguousMapBenchmarks.cs index f6b8e56..d496fbe 100644 --- a/Containers.Benchmarks/ContiguousMapBenchmarks.cs +++ b/Containers.Benchmarks/ContiguousMapBenchmarks.cs @@ -213,11 +213,12 @@ public int ContiguousMap_SpanAccess() map.TryAdd(item.Key, item.Value); } - ReadOnlySpan keysSpan = map.GetKeysSpan(); + // AsReadOnlySpan is the zero-copy view; GetKeysSpan copied every key into a new array + ReadOnlySpan.Entry> entries = map.AsReadOnlySpan(); int sum = 0; - for (int i = 0; i < keysSpan.Length; i++) + for (int i = 0; i < entries.Length; i++) { - sum += keysSpan[i]; + sum += entries[i].Key; } return sum; } diff --git a/Containers.Test/ContiguousMapTests.cs b/Containers.Test/ContiguousMapTests.cs index 01acf71..6657a93 100644 --- a/Containers.Test/ContiguousMapTests.cs +++ b/Containers.Test/ContiguousMapTests.cs @@ -414,6 +414,108 @@ public void CopyTo_ValidArray_CopiesElements() Assert.AreEqual(default, array[3]); // Unchanged } + [TestMethod] + public void CopyKeysTo_FillsDestinationInInsertionOrder() + { + ContiguousMap map = new() { { 3, "three" }, { 1, "one" }, { 4, "four" }, { 2, "two" } }; + int[] destination = new int[4]; + + map.CopyKeysTo(destination); + + int[] expected = [3, 1, 4, 2]; + Assert.AreSequenceEqual(expected, destination); + } + + [TestMethod] + public void CopyValuesTo_FillsDestinationInInsertionOrder() + { + ContiguousMap map = new() { { 3, "three" }, { 1, "one" }, { 4, "four" }, { 2, "two" } }; + string[] destination = new string[4]; + + map.CopyValuesTo(destination); + + string[] expected = ["three", "one", "four", "two"]; + Assert.AreSequenceEqual(expected, destination); + } + + [TestMethod] + public void CopyKeysTo_LongerDestination_LeavesTailUntouched() + { + ContiguousMap map = new() { { 7, "seven" }, { 8, "eight" } }; + int[] destination = [-1, -1, -1, -1]; + + map.CopyKeysTo(destination); + + int[] expected = [7, 8, -1, -1]; + Assert.AreSequenceEqual(expected, destination); + } + + [TestMethod] + public void CopyKeysTo_ShorterDestination_ThrowsWithoutWriting() + { + ContiguousMap map = new() { { 7, "seven" }, { 8, "eight" } }; + int[] destination = [-1]; + + ArgumentException exception = Assert.ThrowsExactly(() => map.CopyKeysTo(destination)); + + Assert.AreEqual("destination", exception.ParamName); + Assert.AreEqual(-1, destination[0]); + } + + [TestMethod] + public void CopyValuesTo_ShorterDestination_ThrowsWithoutWriting() + { + ContiguousMap map = new() { { 7, "seven" }, { 8, "eight" } }; + string[] destination = ["untouched"]; + + ArgumentException exception = Assert.ThrowsExactly(() => map.CopyValuesTo(destination)); + + Assert.AreEqual("destination", exception.ParamName); + Assert.AreEqual("untouched", destination[0]); + } + + [TestMethod] + public void CopyKeysAndValuesTo_DoNotAllocate() + { + ContiguousMap map = []; + for (int i = 0; i < 1_000; i++) + { + map.Add(i, i * 2); + } + + int[] keys = new int[map.Count]; + int[] values = new int[map.Count]; + map.CopyKeysTo(keys); + map.CopyValuesTo(values); + + long before = GC.GetAllocatedBytesForCurrentThread(); + map.CopyKeysTo(keys); + map.CopyValuesTo(values); + long allocated = GC.GetAllocatedBytesForCurrentThread() - before; + + Assert.AreEqual(0, allocated); + Assert.AreEqual(999, keys[999]); + Assert.AreEqual(1_998, values[999]); + } + + [TestMethod] + [DataRow(nameof(ContiguousMap<,>.GetKeysSpan), nameof(ContiguousMap<,>.CopyKeysTo))] + [DataRow(nameof(ContiguousMap<,>.GetValuesSpan), nameof(ContiguousMap<,>.CopyValuesTo))] + public void CopyingSpanAccessors_AreObsoleteAndPointAtTheirReplacement(string method, string replacement) + { + // The *Span names read as zero-copy views, but every call allocates and copies the whole map + ObsoleteAttribute? obsolete = typeof(ContiguousMap).GetMethod(method)! + .GetCustomAttributes(typeof(ObsoleteAttribute), inherit: false) + .Cast() + .SingleOrDefault(); + + Assert.IsNotNull(obsolete, $"{method} should be marked [Obsolete]"); + Assert.Contains(replacement, obsolete.Message!); + Assert.Contains("AsReadOnlySpan", obsolete.Message!); + Assert.IsFalse(obsolete.IsError, "Obsoleting must not break existing callers"); + } + +#pragma warning disable CS0618 // GetKeysSpan/GetValuesSpan are obsolete but still supported [TestMethod] public void GetKeysSpan_ReturnsCorrectSpan() { @@ -442,7 +544,9 @@ public void GetKeysSpan_ReturnsCorrectSpan() Assert.Contains(3, spanKeys); Assert.Contains(4, spanKeys); } +#pragma warning restore CS0618 +#pragma warning disable CS0618 // GetKeysSpan/GetValuesSpan are obsolete but still supported [TestMethod] public void GetValuesSpan_ReturnsCorrectSpan() { @@ -471,7 +575,9 @@ public void GetValuesSpan_ReturnsCorrectSpan() Assert.Contains("three", spanValues); Assert.Contains("four", spanValues); } +#pragma warning restore CS0618 +#pragma warning disable CS0618 // GetKeysSpan/GetValuesSpan are obsolete but still supported [TestMethod] public void ContiguousMemoryLayout_OptimalForCachePerformance() { @@ -506,6 +612,7 @@ public void ContiguousMemoryLayout_OptimalForCachePerformance() } Assert.HasCount(1000, spanKeys); } +#pragma warning restore CS0618 [TestMethod] public void WorksWithCustomTypes() diff --git a/Containers/ContiguousMap.cs b/Containers/ContiguousMap.cs index be2cb67..b789b72 100644 --- a/Containers/ContiguousMap.cs +++ b/Containers/ContiguousMap.cs @@ -623,39 +623,87 @@ public ref TValue GetValueRefOrNullRef(TKey key) public ReadOnlySpan AsReadOnlySpan() => new(items, 0, Count); /// - /// Gets a read-only span representing the keys in the map. + /// Copies the keys of the map, in insertion order, into the start of . /// - /// A read-only span over the map's keys. + /// The span to fill. It must hold at least elements. + /// Thrown when is shorter than . /// - /// This method provides direct access to the keys from the contiguous memory, enabling high-performance - /// operations. The keys are extracted from the underlying Entry array. + /// O(n), with no allocation. For indexed access without any copy, use and read + /// . /// - public ReadOnlySpan GetKeysSpan() + public void CopyKeysTo(Span destination) { - TKey[] keys = new TKey[Count]; + if (destination.Length < Count) + { + throw new ArgumentException( + $"The destination span holds {destination.Length} elements, but the map has {Count}.", + nameof(destination) + ); + } + for (int i = 0; i < Count; i++) { - keys[i] = items[i].Key; + destination[i] = items[i].Key; } - return keys.AsSpan(); } /// - /// Gets a read-only span representing the values in the map. + /// Copies the values of the map, in insertion order, into the start of . /// - /// A read-only span over the map's values. + /// The span to fill. It must hold at least elements. + /// Thrown when is shorter than . /// - /// This method provides direct access to the values from the contiguous memory, enabling high-performance - /// operations. The values are extracted from the underlying Entry array. + /// O(n), with no allocation. For indexed access without any copy, use and read + /// . /// - public ReadOnlySpan GetValuesSpan() + public void CopyValuesTo(Span destination) { - TValue[] values = new TValue[Count]; + if (destination.Length < Count) + { + throw new ArgumentException( + $"The destination span holds {destination.Length} elements, but the map has {Count}.", + nameof(destination) + ); + } + for (int i = 0; i < Count; i++) { - values[i] = items[i].Value; + destination[i] = items[i].Value; } - return values.AsSpan(); + } + + /// + /// Copies the keys in the map into a new array and returns a span over it. + /// + /// A read-only span over a new array holding the map's keys, in insertion order. + /// + /// Despite the name this is not a view: every call allocates an array and copies all n keys, so calling it + /// inside an indexed loop is O(n²). Use to fill a buffer you own, or + /// for zero-copy access to . + /// + [Obsolete("GetKeysSpan allocates and copies every key on each call. Use CopyKeysTo to fill a buffer you own, or AsReadOnlySpan() for zero-copy access to Entry.Key.")] + public ReadOnlySpan GetKeysSpan() + { + TKey[] keys = new TKey[Count]; + CopyKeysTo(keys); + return keys; + } + + /// + /// Copies the values in the map into a new array and returns a span over it. + /// + /// A read-only span over a new array holding the map's values, in insertion order. + /// + /// Despite the name this is not a view: every call allocates an array and copies all n values, so calling it + /// inside an indexed loop is O(n²). Use to fill a buffer you own, or + /// for zero-copy access to . + /// + [Obsolete("GetValuesSpan allocates and copies every value on each call. Use CopyValuesTo to fill a buffer you own, or AsReadOnlySpan() for zero-copy access to Entry.Value.")] + public ReadOnlySpan GetValuesSpan() + { + TValue[] values = new TValue[Count]; + CopyValuesTo(values); + return values; } ///