Repository navigation
Revert "[cDAC] Avoid allocating stack contexts for output" - #137101
Merged
Merged
Conversation
This reverts commit f3a4618.
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 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
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
jkotas
requested review from
max-charlamb and
rcj1
and
a balanced review from Copilot
November 6, 2026 19:02
max-charlamb
approved these changes
Nov 6, 2026
lewing
approved these changes
Nov 6, 2026
rcj1
approved these changes
Nov 6, 2026
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the change restoring byte-array copies for cDAC stack-context output.
No open issues 🎉
What changed in this PR
Reverts #136934, restoring allocated byte-array copies for cDAC stack-context output instead of read-only spans over retained context data.
Changes:
- Restores
byte[]return types for context accessors and allocation inContextHolder.GetBytes(). - Updates stack walking, debugger operations, and legacy adapters to consume arrays directly.
- Aligns unit tests, dump tests, and the StackWalk contract documentation with the restored behavior.
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/StackWalkTests.cs | Removes redundant context conversion when seeding a walk. |
| src/native/managed/cdac/tests/UnitTests/PlatformContextTests.cs | Uses returned arrays for context snapshots and comparisons. |
| src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs | Uses the returned context array directly. |
| src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs | Removes redundant context-array conversions. |
| src/native/managed/cdac/tests/DumpTests/StackWalkDumpTests.cs | Restores array-based context checks and consumption. |
| src/native/managed/cdac/tests/DumpTests/IXCLRDataFrameDumpTests.cs | Updates context-buffer assertions for arrays. |
| src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiStackWalkDumpTests.cs | Removes conversion when selecting non-leaf contexts. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Consumes context arrays directly. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataStackWalk.cs | Restores array-based context retrieval. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataFrame.cs | Restores array copying and direct context consumption. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_1.ExceptionHandling.cs | Uses the returned array to seed parent-frame searches. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_1.cs | Restores array output and removes redundant conversions. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/IPlatformAgnosticContext.cs | Restores the byte[] return type for GetBytes(). |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/ContextHolder.cs | Restores allocation of a context-byte copy. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs | Adapts context writes and copying to array output. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IStackWalk.cs | Restores the byte[] return type for GetRawContext(). |
| docs/design/datacontracts/StackWalk.md | Documents restored byte-array context output. |
Review effort: Balanced
Member
|
/ba-g fast track build break fix |
max-charlamb
added a commit
that referenced
this pull request
Nov 9, 2026
## Summary Reapply #136934 after it was reverted by #137101. Return retained stack-context snapshots as `ReadOnlySpan<byte>` instead of allocating a new `byte[]` for every output request. This changes only the context-output APIs: - `IStackWalk.GetRawContext` returns a read-only view of the frame's retained context snapshot. - `IPlatformAgnosticContext.GetBytes` returns a read-only view over the serialized platform context. - COM and debugger consumers copy directly from the span where possible. - APIs and tests that require owned, writable, or pinned context storage materialize arrays explicitly. The span remains backed by the context snapshot owned by the stack frame handle. Advancing the walk does not mutate that retained snapshot. The original PR passed its CI, but #130367 merged after that validation and added six new `GetBytes()` callers. The combined changes compiled neither in the three production callers that invoked `.AsSpan()` on the new `ReadOnlySpan<byte>` nor in three tests that assigned the span directly to `byte[]`. This replay accounts for all six callers. ## Performance The measurements from #136934 used frozen NativeAOT readers with provider identities reversed between two passes. Each pass used 3 launches with 30 observations per case, with no outlier removal. The primary run retained 2,520 observations across 14 timing cases. | Scenario | Before | After | Change | Reversed-order changes | |---|---:|---:|---:|---:| | Enum CodeHeavy Heap | 4.665 ms | 4.554 ms | -2.4% | -0.7% / -4.0% | | Enum DeepStacks Heap | 12.585 ms | 12.182 ms | -3.2% | -5.4% / -0.8% | | Enum SmallSimple Heap | 2.787 ms | 2.738 ms | -1.8% | -4.8% / +0.9% | | Stack DeepStacks Heap Cold | 9.040 ms | 8.668 ms | -4.1% | -2.6% / -5.7% | | Stack DeepStacks Heap Warm | 8.645 ms | 8.504 ms | -1.6% | -1.5% / -1.8% | | Stack DeepStacks Mini Cold | 11.450 ms | 11.640 ms | +1.7% | +2.3% / +1.0% | | Stack DeepStacks Mini Warm | 11.013 ms | 10.913 ms | -0.9% | +3.5% / -5.4% | A separate cold-walk confirmation used two more reversed passes and 720 additional observations: | Scenario | Before | After | Change | Reversed-order changes | |---|---:|---:|---:|---:| | Stack DeepStacks Heap Cold | 8.692 ms | 8.199 ms | -5.7% | -4.2% / -7.2% | | Stack DeepStacks Mini Cold | 11.185 ms | 10.637 ms | -4.9% | -4.2% / -5.6% | The change removes one temporary 1,232-byte array for each AMD64 context output. Dump-enumeration timing does not directly exercise the migrated output APIs, and shared-host timing differences are not guaranteed gains. ## Testing - `dotnet build src\native\managed\cdac\Microsoft.Diagnostics.DataContractReader.Legacy\Microsoft.Diagnostics.DataContractReader.Legacy.csproj -c Release --no-restore -m:1 -p:BuildInParallel=false -p:RuntimeConfiguration=Release -p:LibrariesConfiguration=Release` -- passed with 0 warnings and 0 errors. - `$dotnetHost = "$PWD\.dotnet\dotnet.exe"; dotnet test src\native\managed\cdac\tests\UnitTests\Microsoft.Diagnostics.DataContractReader.Tests.csproj -c Release --no-restore -m:1 -p:NuGetConsoleProcessFileName="$dotnetHost" -p:BuildInParallel=false -p:RuntimeConfiguration=Release -p:LibrariesConfiguration=Release` -- passed, 4,044 tests. - The filtered stack-walk dump-test project compiled successfully, but no dump tests executed because all available local and .NET 10 configurations were skipped in this checkout. > [!NOTE] > This pull request description was generated with GitHub Copilot. Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6297d10c-2f94-48bb-b897-bc0b3e1a855f
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.
Reverts #136934