Skip to content

Small optimizations in System.Net.NetworkInformation #64423

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

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Jan 28, 2022

This PR does two small optimizations:

  • In the TeredoHelper class, we used to queue a callback to the thread pool by using the good old ThreadPool.QueueUserWorkItem method. I changed it by implementing the IThreadPoolWorkItem interface in TeredoHelper, and queueing it directly to the thread pool, saving an allocation, and avoiding needlessly flowing the ExecutionContext (judging from the name TeredoHelper.UnsafeNotifyStableUnicastIpAddressTable).
  • The SystemIPGlobalProperties.GetUnicastAddressesAsync was creating a TaskCompletionSource<bool>, while the newer non-generic TaskCompletionSource class is more appropriate and has a tiny bit smaller footprint. I changed it.

@ghost ghost added area-System.Net community-contribution Indicates that the PR has been added by a community member labels Jan 28, 2022
@ghost
Copy link

ghost commented Jan 28, 2022

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

Issue Details

This PR does two small optimizations:

  • In the TeredoHelper class, we used to queue a callback to the thread pool by using the good old ThreadPool.QueueUserWorkItem method. I changed it by implementing the IThreadPoolWorkItem interface in TeredoHelper, and queueing it directly to the thread pool, saving an allocation, and avoiding needlessly flowing the ExecutionContext (judging from the name TeredoHelper.UnsafeNotifyStableUnicastIpAddressTable).
  • The SystemIPGlobalProperties.GetUnicastAddressesAsync was creating a TaskCompletionSource<bool>, while the newer non-generic TaskCompletionSource class is more appropriate and has a tiny bit smaller footprint. I changed it.
Author: teo-tsirpanis
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

@stephentoub
Copy link
Member

Thanks. It's fine to change the TaskCompletionSource<T> to a TaskCompletionSource, but we should stick with QueueUserWorkItem. Saving one tiny allocation here for something that's probably only invoked once in the lifetime of the app isn't worth the complication of having yet another IThreadPoolWorkItem implementation, complicating debugging (e.g. the work item won't be inspectable by VS' implicit understanding of "normal" work items), etc.

@teo-tsirpanis
Copy link
Contributor Author

Done @stephentoub. Can you tell me more about this debugger limitation with IThreadPoolWorkItemss? I'm interested to learn.

@stephentoub
Copy link
Member

Debuggers know about the built-in work item types. They know about their layout, what their fields mean, etc. An arbitrary IThreadPoolWorkItem is an unknown entity. They can't reason about what it is, what it represents, etc.

@stephentoub stephentoub merged commit 01fabd0 into dotnet:main Jan 28, 2022
@teo-tsirpanis teo-tsirpanis deleted the patch-1 branch January 28, 2022 16:34
pranavkm pushed a commit to dotnet/aspnetcore that referenced this pull request Jan 29, 2022
# Use non-generic TaskCompletionSource

Use the non-generic `TaskCompletionSource` where appropriate.

## Description

Inspired by dotnet/runtime#64423, I had a look to see if there were any usages of `TaskCompletionSource<T>` that could just use the non generic variant instead. Searching for `bool`, `int` and `object` I found quite a few, mostly in tests, which were just being used to signal completion of arbitrary operation where the `T` value wasn't used.

This PR changes all such usages that wouldn't require use of conditional compilation to handle TFMs that don't have support for `TaskCompletionSource`.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants