Remove large-memory CopyTo and parser overflow tests - #131280
Merged
tannergooding merged 2 commits intoJul 24, 2026
Merged
Conversation
The three disabled/expensive large-memory tests committed 8GB (CopyTo) or refilled 2GB five times (parser), thrashing swap on 14GB CI agents. Halve the CopyTo footprint to a single overlapping ~4GB buffer, verify only the boundaries (the tail still catches a truncated length), drop the redundant 4GB+256B case, and fill the parser buffer once while restoring only the bytes each case touches. Switch AllocationHelper from Marshal.AllocHGlobal to NativeMemory, and re-enable the two CopyTo tests by removing the dotnet#24139 ActiveIssue. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts several System.Memory.Tests [OuterLoop] scenarios to reduce peak memory and per-test execution cost while keeping coverage of large-buffer behaviors (notably >4GB CopyTo and ~2GB parser overflow).
Changes:
- Updates the large-buffer
Span<T>.CopyTo/ReadOnlySpan<T>.CopyTotests to use a single overlapping >4GB native allocation instead of two separate >4GB allocations. - Refactors the 2GiB parser overflow test to fill the 2GB buffer once and restore only the bytes touched per test case.
- Switches test native allocation helpers from
Marshal.AllocHGlobal/FreeHGlobaltoNativeMemory.Alloc/Free.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Memory/tests/Span/CopyTo.cs | Reworks >4GB Span.CopyTo test to use one overlapping native buffer and fewer assertions. |
| src/libraries/System.Memory/tests/ReadOnlySpan/CopyTo.cs | Mirrors the Span change for ReadOnlySpan.CopyTo with the same allocation strategy. |
| src/libraries/System.Memory/tests/ParsersAndFormatters/Parser/ParserTests.2gbOverflow.cs | Avoids repeated full-buffer refills by reusing a single 2GB span and restoring only mutated bytes. |
| src/libraries/System.Memory/tests/AllocationHelper.cs | Changes native allocation/free implementation to NativeMemory and keeps a global mutex for large allocations. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 3
- AllocationHelper: release the mutex on any failed allocation via finally, not just OutOfMemoryException, so a failure can't leave it held and hang later tests. - CopyTo tests: initialize only the boundary elements instead of filling the whole >4GB buffer, dropping an extra full memory pass while keeping truncation detection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
tannergooding
changed the title
Member
Author
|
@vcsjones any other feedback on this one? |
vcsjones
approved these changes
Jul 24, 2026
hez2010
pushed a commit
to hez2010/runtime
that referenced
this pull request
Jul 26, 2026
Fixes dotnet#24139. Removes the three large-memory `[OuterLoop]` tests in `System.Memory.Tests` — the two `CopyToLargeSizeTest` (`Span` + `ReadOnlySpan`) and `TestParser2GiBOverflow` — rather than trying to keep them alive. These allocated 4–16GB of memory (or repeatedly filled a 2GB buffer) and were flaky on the 14GB outerloop machines, which is what dotnet#24139 reports. After looking at what they actually cover, they aren't worth that cost: - There is no special `>2GB`/`>4GB` code path. `Span<T>.CopyTo` just funnels into the native `Memmove`, and the parser case only validates that a full 2GB buffer is consumed and succeeds/fails as expected. - `Span<T>` is itself length-limited, so there is no overflow edge case here that isn't already covered — and better bounded — by the existing bounds/overflow tests. - The behavior can be validated manually if this logic is ever touched again, which matches the conclusion reached previously for a similar 2GB test in dotnet#118400. `AllocationHelper` is left as-is; it is still used by the remaining large-allocation tests (`Overflow`, `Clear`, `BinarySearch`, `Base64`). > [!NOTE] > This PR description and the code changes were drafted with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
tannergooding
added a commit
that referenced
this pull request
Jul 27, 2026
Fixes #131390. `Microsoft.Bcl.Memory.Tests` links `System.Memory/tests/AllocationHelper.cs` and targets `net481`, where `NativeMemory` doesn't exist. #131280 switched that helper from `Marshal.AllocHGlobal` to `NativeMemory` without accounting for the shared compile, breaking `windows-x64 Release Libraries_NET481`. Rather than reverting to `Marshal.AllocHGlobal` -- which is `LocalAlloc` on Windows and would pessimize every modern .NET consumer to fix a `net481`-only compile error -- this adds a test-only `NativeMemory` polyfill under `Common/tests`, alongside the `RuntimeHelpers` polyfill already linked into the same `.NETFramework` `ItemGroup`. `System.Memory.Tests` is `$(NetCoreAppCurrent)`-only, so `Microsoft.Bcl.Memory.Tests` is the only project that picks the polyfill up. ---------- The one unavoidable change in `AllocationHelper` itself is the argument cast: ```diff - memory = (IntPtr)NativeMemory.Alloc((nuint)size); + memory = (IntPtr)NativeMemory.Alloc((nuint)(void*)size); ``` `IntPtr`/`UIntPtr` aren't the numeric types on .NETFramework, so `(nuint)size` is `CS0030` there -- that's the second error in the issue, and it's independent of the polyfill. The pointer round-trip is legal on both and compiles to nothing. ---------- Validated locally on Windows x64: | Leg | Result | | --- | --- | | `Microsoft.Bcl.Memory.Tests` `net481` innerloop | 548 passed | | `Microsoft.Bcl.Memory.Tests` `net481` outerloop | 2 passed | | `Microsoft.Bcl.Memory.Tests` netcoreapp innerloop | 548 passed | | `System.Memory.Tests` innerloop | 52,929 passed | | `System.Memory.Tests` outerloop | 31 passed | The outerloop runs cover the remaining `AllocationHelper` consumers (`Overflow`, `Clear`, `BinarySearch`, `Base64`, `Base64Url`). Stashing the fix reproduces the exact `CS0103`/`CS0030` errors from the issue. CC. @dotnet/area-system-memory > [!NOTE] > This PR description and the code changes were drafted with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #24139.
Removes the three large-memory
[OuterLoop]tests inSystem.Memory.Tests— the twoCopyToLargeSizeTest(Span+ReadOnlySpan) andTestParser2GiBOverflow— rather than trying to keep them alive.These allocated 4–16GB of memory (or repeatedly filled a 2GB buffer) and were flaky on the 14GB outerloop machines, which is what #24139 reports. After looking at what they actually cover, they aren't worth that cost:
>2GB/>4GBcode path.Span<T>.CopyTojust funnels into the nativeMemmove, and the parser case only validates that a full 2GB buffer is consumed and succeeds/fails as expected.Span<T>is itself length-limited, so there is no overflow edge case here that isn't already covered — and better bounded — by the existing bounds/overflow tests.AllocationHelperis left as-is; it is still used by the remaining large-allocation tests (Overflow,Clear,BinarySearch,Base64).Note
This PR description and the code changes were drafted with GitHub Copilot.