[release/8.0] Improve performance of UnmanagedMemoryStream #93812
Conversation
This file contains 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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
UnmanagedMemoryStream used Interlocked operations to update its state to prevent tearing of 64-bit values on 32-bit platforms. This pattern is expensive in general and it was found to be prohibitively expensive on recent hardware.. This change removes the expensive Interlocked operations and addresses the tearing issues in alternative way: - The _length field is converted to nuint that is guaranteed to be updated atomically. - Writes to _length field are volatile to guaranteed the unininitialized memory cannot be read. - The _position field remains long and it has a risk of tearing. It is not a problem since tearing of this field cannot lead to buffer overruns. Fixes #93624
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsBackport of #93766 to release/8.0-staging /cc @jkotas Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
This was referenced Oct 21, 2023
stephentoub
approved these changes
Oct 21, 2023
|
Approved over email |
jkotas
changed the title
lewing
added a commit
that referenced
this pull request
Oct 28, 2023
* [release/8.0] Stable branding for .NET 8 GA * Handle an empty bandPreleaseVersion * [release/8.0] Update dependencies from dotnet/emsdk (#93801) * Update dependencies from https://github.com/dotnet/emsdk build 2023102.2 Microsoft.NET.Workload.Emscripten.Current.Manifest-8.0.100.Transport From Version 8.0.0-rtm.23511.3 -> To Version 8.0.0-rtm.23520.2 * switch to the stable version now * Update dependencies * Also fix the trigger * pin the wbt sdk to avoid the latest analizer nonsense * Use source generation for the serializer * Try to make sourcebuild happy * Try again to make sourcebuild happy * Use reflection and suppress trim analysis instead This reverts commit 768b65b. * Fix reverting too much --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Larry Ewing <[email protected]> * [release/8.0] Update APICompat settings under source build (#93865) * Update APICompat settings under source build * Update resolveContract.targets --------- Co-authored-by: Viktor Hofer <[email protected]> * Override InformationalVersion for NativeAOT corelib too * [release/8.0] Improve performance of UnmanagedMemoryStream (#93812) * Improve performance of UnmanagedMemoryStream UnmanagedMemoryStream used Interlocked operations to update its state to prevent tearing of 64-bit values on 32-bit platforms. This pattern is expensive in general and it was found to be prohibitively expensive on recent hardware.. This change removes the expensive Interlocked operations and addresses the tearing issues in alternative way: - The _length field is converted to nuint that is guaranteed to be updated atomically. - Writes to _length field are volatile to guaranteed the unininitialized memory cannot be read. - The _position field remains long and it has a risk of tearing. It is not a problem since tearing of this field cannot lead to buffer overruns. Fixes #93624 * Add comment --------- Co-authored-by: Jan Kotas <[email protected]> * Update dependencies from https://github.com/dotnet/emsdk build 20231023.2 (#93881) Microsoft.SourceBuild.Intermediate.emsdk , Microsoft.NET.Workload.Emscripten.Current.Manifest-8.0.100 From Version 8.0.0-rtm.23520.2 -> To Version 8.0.0-rtm.23523.2 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * [release/8.0] Update dependencies from dnceng/internal/dotnet-optimization (#93827) * Update dependencies from https://dev.azure.com/dnceng/internal/_git/dotnet-optimization build 20231021.3 optimization.linux-arm64.MIBC.Runtime , optimization.linux-x64.MIBC.Runtime , optimization.windows_nt-arm64.MIBC.Runtime , optimization.windows_nt-x64.MIBC.Runtime , optimization.windows_nt-x86.MIBC.Runtime , optimization.PGO.CoreCLR From Version 1.0.0-prerelease.23519.5 -> To Version 1.0.0-prerelease.23521.3 * Update dependencies from https://dev.azure.com/dnceng/internal/_git/dotnet-optimization build 20231021.3 optimization.linux-arm64.MIBC.Runtime , optimization.linux-x64.MIBC.Runtime , optimization.windows_nt-arm64.MIBC.Runtime , optimization.windows_nt-x64.MIBC.Runtime , optimization.windows_nt-x86.MIBC.Runtime , optimization.PGO.CoreCLR From Version 1.0.0-prerelease.23519.5 -> To Version 1.0.0-prerelease.23521.3 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * [release/8.0][wasm] Fix perf pipeline runs (#93888) * Remove --experimental-wasm-eh argument from the wasm_args used for wasm performance runs. (#93357) (cherry picked from commit a770017) * performance-setup.sh: Use `release/8.0` as the default channel * performance-setup.ps1: use release/8.0 as the default channel * Fix passing wasmArgs for bdn --------- Co-authored-by: Parker Bibus <[email protected]> * [release/8.0] Honor JsonSerializerOptions.PropertyNameCaseInsensitive in property name conflict resolution. (#93935) * Honor JsonSerializerOptions.PropertyNameCaseInsensitive in property name conflict resolution. * Update src/libraries/System.Text.Json/tests/Common/PropertyNameTests.cs Co-authored-by: Jeff Handley <[email protected]> * Address feedback --------- Co-authored-by: Eirik Tsarpalis <[email protected]> Co-authored-by: Jeff Handley <[email protected]> * [8.0] Update MsQuic (#93979) * Try pinning the installer version to a 8.01xx sdk * Target net8.0 in SatelliteAssemblyFromProjectRef --------- Co-authored-by: Carlos Sánchez López <[email protected]> Co-authored-by: Larry Ewing <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Viktor Hofer <[email protected]> Co-authored-by: Alexander Köplinger <[email protected]> Co-authored-by: Jan Kotas <[email protected]> Co-authored-by: Ankit Jain <[email protected]> Co-authored-by: Parker Bibus <[email protected]> Co-authored-by: Eirik Tsarpalis <[email protected]> Co-authored-by: Jeff Handley <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Backport of #93766 to release/8.0-staging
/cc @jkotas
Customer Impact
Customer observed very poor performance of UnmanagedMemoryStream on recent Intel hardware in 32-bit processes.
The fix replaces expensive Interlocked operations with an alternative scheme based on Volatile. Up to 50x improvement in UnmanagedMemoryStream microbenchmarks on the affected configuration.
Testing
Verified no functional regression and improved performance of UnmanagedMemoryStream. Microbenchmark submitted to dotnet/performance repo.
Risk
Low
IMPORTANT: If this backport is for a servicing release, please verify that:
The PR target branch is
release/X.0-staging, notrelease/X.0.If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.