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

Avoid byte[] allocation for IPAddressUtil.IsMulticast #68591

Merged
merged 1 commit into from
May 10, 2022

Conversation

Bond-009
Copy link
Contributor

Ping.RawSocket his its own inline implementaion of this, maybe this
should be consolidated somewhere?
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
L101

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 27, 2022
@ghost
Copy link

ghost commented Apr 27, 2022

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

Issue Details

Ping.RawSocket his its own inline implementaion of this, maybe this
should be consolidated somewhere?
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
L101

Author: Bond-009
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

Ping.RawSocket his its own inline implementaion of this, maybe this
should be consolidated somewhere?
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
L101
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

byte firstByte = address.GetAddressBytes()[0];
#pragma warning disable CS0618 // using Obsolete Address API because it's the more efficient option in this case
byte firstByte = (byte)address.Address;
#pragma warning restore CS0618
return firstByte >= 224 && firstByte <= 239;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return firstByte >= 224 && firstByte <= 239;
return firstByte is >= 224 and <= 239;

Once roslyn optimizes this (cf. dotnet/roslyn#60534), codegen improvement comes for free.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand these are different constructs as far as Roslyn is concerned, but it's an unfortunate pit of failure if these otherwise identical expressions end up being optimized differently. I'd really like for us to not have to change all such range checks everywhere in order for the right things to happen.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt wfurt merged commit 6c12de6 into dotnet:main May 10, 2022
@Bond-009 Bond-009 deleted the ipaddressutils branch May 10, 2022 18:12
@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 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.

6 participants