Skip to content

Optimize Uri.GetHashCode #32713

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.

Sign up for GitHub

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
MihaZupan merged 6 commits into from
Apr 29, 2020
Merged

Optimize Uri.GetHashCode #32713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6108fd7
Optimize Uri.GetHashCode
MihaZupan Feb 23, 2020
80c1951
Don't use case-insensitive string hashing when not necessary
MihaZupan Feb 23, 2020
4f15c66
Don't expect Uri.GetHashCode to always be case-insensitive in tests
MihaZupan Feb 24, 2020
eca5fd3
Correct Uri.Equals test
MihaZupan Feb 24, 2020
684854d
Do not cache Uri GetHashCode
MihaZupan Mar 2, 2020
450be78
Simplify Uri.Equals test
MihaZupan Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 11 additions & 30 deletions src/libraries/System.Private.Uri/src/System/Uri.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ private class MoreInfo
public string? Query;
public string? Fragment;
public string? AbsoluteUri;
public int Hash;
public string? RemoteUrl;
};

Expand Down Expand Up @@ -1505,39 +1504,26 @@ public static int FromHex(char digit) =>
(uint)(digit - 'a') <= 'f' - 'a' ? digit - 'a' + 10 :
throw new ArgumentException(nameof(digit));

//
// GetHashCode
//
// Overrides default function (in Object class)
//
//
public override int GetHashCode()
{
if (IsNotAbsoluteUri)
{
return CalculateCaseInsensitiveHashCode(OriginalString);
}

// Consider moving hash code storage from m_Info.MoreInfo to m_Info
UriInfo info = EnsureUriInfo();
if ((object?)info.MoreInfo == null)
{
info.MoreInfo = new MoreInfo();
return OriginalString.GetHashCode();
}
int tempHash = info.MoreInfo.Hash;
if (tempHash == 0)
else
{
string? chkString = info.MoreInfo.RemoteUrl;
if ((object?)chkString == null)
chkString = GetParts(UriComponents.HttpRequestUrl, UriFormat.SafeUnescaped);
tempHash = CalculateCaseInsensitiveHashCode(chkString);
if (tempHash == 0)
MoreInfo moreInfo = EnsureUriInfo().MoreInfo ??= new MoreInfo();
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Feb 24, 2020

Choose a reason for hiding this comment

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

@stephentoub - how far down the rabbit hole do we want to go here with regard to null coalescing syntax? This is a clever application but I admit it might affect maintainability of code since it involves null coalescing plus a possible assignment to a field on an object (not this) that's returned as part of a function call plus a dereference of that field, all in the same line.

Copy link
Member Author

@MihaZupan MihaZupan Feb 24, 2020

Choose a reason for hiding this comment

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

I tried to not take it too far :)

Copy link
Member Author

@MihaZupan MihaZupan Mar 11, 2020

Choose a reason for hiding this comment

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

With #33042, it becomes MoreInfo moreInfo = EnsureUriInfo().MoreInfo;

string remoteUrl = moreInfo.RemoteUrl ??= GetParts(UriComponents.HttpRequestUrl, UriFormat.SafeUnescaped);

if (IsUncOrDosPath)
{
return remoteUrl.GetHashCode(StringComparison.OrdinalIgnoreCase);
}
else
{
tempHash = 0x1000000; //making it not zero still large enough to be mapped to zero by a hashtable
return remoteUrl.GetHashCode();
}
info.MoreInfo.Hash = tempHash;
}
return tempHash;
}

//
Expand Down Expand Up @@ -4943,11 +4929,6 @@ private static char[] Compress(char[] dest, ushort start, ref int destLength, Ur
return dest;
}

internal static int CalculateCaseInsensitiveHashCode(string text)
{
return text.ToLowerInvariant().GetHashCode();
}

//
// CombineUri
//
Expand Down
8 changes: 5 additions & 3 deletions src/libraries/System.Runtime/tests/System/Uri.MethodsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,15 +389,17 @@ public static IEnumerable<object[]> Equals_TestData()
public void Equals(Uri uri1, object obj, bool expected)
{
Uri uri2 = obj as Uri;

if (uri1 != null)
{
Assert.Equal(expected, uri1.Equals(obj));
if (uri2 != null)

if (uri2 != null && expected)
{
bool onlyCaseDifference = string.Equals(uri1.OriginalString, uri2.OriginalString, StringComparison.OrdinalIgnoreCase);
Assert.Equal(expected || onlyCaseDifference, uri1.GetHashCode().Equals(uri2.GetHashCode()));
Assert.Equal(uri1.GetHashCode(), uri2.GetHashCode());
}
}

if (!(obj is string))
{
Assert.Equal(expected, uri1 == uri2);
Expand Down