Change NetTrace format version support, eliminate pinning, and improve UnsupportedVersionException #2180
Conversation
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
…rtedVersionException - Re-enabling support for parsing format V6 and re-enabling the tests - Removed support for ancient netperf format version 1 and 2 that were only used during .NET Core 2.0 (not even 2.1). - The implementation of the Nettrace reader was doing a bunch of pinning because the TraceEvent internal model of events uses unmanaged pointers, however the pinning was creating bad GC performance. TraceEvent builds for .NET standard so we don't have guaranteed access to the pinned heap and I didn't want to start doing lots of copying and/or managing pools of pinned arrays. Instead I switched the stream reading over to using spans backed by native heap allocations and explicit block lifetime tracking. As a side benefit it also brings the V3-5 block iteration code a bit closer to the V6 block iteration and removed all the dependencies on the existing FastSerialization library. - Add MinSupportedVersion and MaxSupportedVersion to the UnsupportedVersionException
noahfalk
changed the title
- Removed some V1_V2 support code that I missed before - Added comments referencing the block header format and why we have a duplicate FastSerialization tag enum - Pruned unneeded using statements - Switched from EndOfStreamException to FormatException so that we consistently use FormatException for all format issues - Removed StreamExtensions.ReadAtLeast which was dead code - Add tests for incomplete stream handling and verifying StreamExtensions.Read is implemented as expected for different platforms
Member
Author
|
Thanks for the feedback @brianrob! Hopefully the latest commit addresses all of it. |
wiktork
reviewed
Apr 9, 2025
|
|
||
| internal FixedBuffer(int length) | ||
| { | ||
| _pointer = Marshal.AllocHGlobal(length); |
Member
There was a problem hiding this comment.
Would NativeMemory be more appropriate here?
Member
Author
There was a problem hiding this comment.
Unforetunately TraceEvent targets .NET Standard 2.0 and NativeMemory was first supported in .NET 6 so it isn't available.
brianrob
approved these changes
Apr 9, 2025
wiktork
added a commit
to dotnet/diagnostics
that referenced
this pull request
May 5, 2025
Code updates are based on microsoft/perfview#2121 Would like to incorporate microsoft/perfview#2180 into dotnet-monitor, to potentially improve memory usage. ~~Test failures are most likely due to changes from microsoft/perfview#2170 Fixed with newest version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
@wiktork - This may fix the GC issues you were seeing in dotnet-monitor
@karpinsn - Exception improvements should help with version error messages in Dev18
@mdh1418, @brianrob, @lateralusX, @beaubelgrave - aside from turning back on V6 support hopefully the behavior should be nearly identical to before despite all the code churn in there.