Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of UnmanagedMemoryStream #93766

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Oct 20, 2023

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

@ghost
Copy link

ghost commented Oct 20, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

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. This is not a problem since tearing of this field cannot lead to buffer overruns.
Author: jkotas
Assignees: jkotas
Labels:

area-System.IO

Milestone: -

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 dotnet#93624
@gautham-beeraka
Copy link

Thank you for the fix. Will the fix be part of .NET8? Will it also be ported to .NET Framework?

@jkotas
Copy link
Member Author

jkotas commented Oct 20, 2023

Will the fix be part of .NET8?

I will do a bar-check about backporting this fix to .NET 8.

Will it also be ported to .NET Framework?

I do not expect this fix to be ported to .NET Framework. Performance fixes are ported to .NET Framework only in very exceptional situations.

@Jozkee Jozkee merged commit 33ee130 into dotnet:main Oct 20, 2023
175 checks passed
@Jozkee Jozkee added this to the 9.0.0 milestone Oct 20, 2023
@jkotas jkotas deleted the unmanagedmemorystream branch October 21, 2023 05:11
jkotas added a commit to jkotas/performance that referenced this pull request Oct 21, 2023
jkotas added a commit to jkotas/performance that referenced this pull request Oct 21, 2023
@jkotas
Copy link
Member Author

jkotas commented Oct 21, 2023

/backport to release/8.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6596033333

@jkotas
Copy link
Member Author

jkotas commented Oct 23, 2023

I will do a bar-check about backporting this fix to .NET 8.

The fix was approved for .NET 8. #93812

@ghost ghost locked as resolved and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interlocked.Read from 32-bit apps are much slower on Intel Sapphire Rapids CPUs
5 participants