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

Add PreAllocatedOverlapped.UnsafeCreate / UnsafeAllocateNativeOverlapped #53196

Merged
merged 1 commit into from
May 25, 2021

Conversation

stephentoub
Copy link
Member

Fixes #42549

@ghost
Copy link

ghost commented May 24, 2021

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #42549

Author: stephentoub
Assignees: -
Labels:

area-System.Threading

Milestone: 6.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@GrabYourPitchforks
Copy link
Member

@stephentoub Feel free also to give feedback on the approved API shape. Anything weird? Missing? Some of the review was us trying to channel your spirit and proactively guess your feedback. :)

@stephentoub
Copy link
Member Author

Feel free also to give feedback on the approved API shape. Anything weird? Missing? Some of the review was us trying to channel your spirit and proactively guess your feedback. :)

My one piece of feedback is that I'm skeptical the UnsafeAllocateNativeOverlapped method is worthwhile. If you care about perf, you should be using PreAllocatedOverlapped... the UnsafeCreate there makes sense. But the minor savings that come from not capturing the ExecutionContext with the non-preallocated variant will pale in comparison to using the preallocated variant. Note that this PR doesn't make use of UnsafeAllocateNativeOverlapped, because our higher-performance code paths use PreAllocatedOverlapped.

@stephentoub stephentoub merged commit d533fbc into dotnet:main May 25, 2021
@stephentoub stephentoub deleted the unsafeoverlapped branch May 25, 2021 21:49
@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
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.

API Proposal: Add ThreadPoolBoundHandle.AllocateUnsafeNativeOverlapped
2 participants