Skip to content

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

@ghost
Copy link

ghost commented May 24, 2021

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