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

Clean up of hashset #7344

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Used by Hashtable and Dictionary's SeralizationInfo .ctor's to store the SeralizationInfo
// object until OnDeserialization is called.

using System.Threading;
using System.Runtime.CompilerServices;
using System.Runtime.Serialization;

namespace Microsoft.Build.Collections
{
internal static partial class HashHelpers
{
private static ConditionalWeakTable<object, SerializationInfo>? s_serializationInfoTable;

public static ConditionalWeakTable<object, SerializationInfo> SerializationInfoTable
Copy link
Member

Choose a reason for hiding this comment

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

I can't review the other code at the moment but if MSBuild uses BinarySerializer it will need to stop doing so per the published deprecation plan. If it does not then this code should be dead.

Copy link
Member

Choose a reason for hiding this comment

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

We've mostly moved off of it. GenerateResource might need to continue to support it even after the runtime doesn't because it's one of the options we provide. We also haven't fully fixed BinaryTranslator.TranslateDotNet, and I agree we should get back to that when we can. Canonical issue: #6215

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SerializationInfoTable is copied from CORE. It exists in core right now afaik.

Copy link
Member

@danmoseley danmoseley Jan 29, 2022

Choose a reason for hiding this comment

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

Right, for anyone using BinarySerializer. It's only needed here if MSBuild uses that on these objects. If not then the type does not need to implement ISerializable at all

{
get
{
if (s_serializationInfoTable == null)
Interlocked.CompareExchange(ref s_serializationInfoTable, new ConditionalWeakTable<object, SerializationInfo>(), null);

return s_serializationInfoTable;
}
}
}
}
46 changes: 37 additions & 9 deletions src/Build/Collections/RetrievableEntryHashSet/HashHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
#if !SILVERLIGHT
#if FEATURE_CONSTRAINED_EXECUTION
using System.Runtime.ConstrainedExecution;
Expand All @@ -16,8 +17,15 @@ namespace Microsoft.Build.Collections
/// <summary>
/// Duplicated because internal to mscorlib
/// </summary>
internal static class HashHelpers
internal static partial class HashHelpers
{
public const uint HashCollisionThreshold = 100;

// This is the maximum prime smaller than Array.MaxArrayLength
internal const int MaxPrimeArrayLength = 0x7FEFFFFD;

public const int HashPrime = 101;
Copy link
Member

Choose a reason for hiding this comment

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

Where do we use this other than making sure we don't pick an almost-multiple of it?


// Table of prime numbers to use as hash table sizes.
// The entry used for capacity is the smallest prime number in this array
// that is larger than twice the previous capacity.
Expand Down Expand Up @@ -60,23 +68,23 @@ internal static int GetPrime(int min)
{
Debug.Assert(min >= 0, "min less than zero; handle overflow checking before calling HashHelpers");

for (int i = 0; i < primes.Length; i++)
foreach (int prime in primes)
danmoseley marked this conversation as resolved.
Show resolved Hide resolved
{
int prime = primes[i];
if (prime >= min)
{
return prime;
}
}

// Outside of our predefined table. Compute the hard way.
for (int i = (min | 1); i < Int32.MaxValue; i += 2)
// Outside of our predefined table. Compute the hard way.
for (int i = (min | 1); i < int.MaxValue; i += 2)
{
if (IsPrime(i))
if (IsPrime(i) && ((i - 1) % HashPrime != 0))
{
return i;
}
}

return min;
}

Expand All @@ -92,15 +100,35 @@ internal static int ExpandPrime(int oldSize)

// Allow the hashtables to grow to maximum possible size (~2G elements) before encoutering capacity overflow.
// Note that this check works even when _items.Length overflowed thanks to the (uint) cast
if ((uint) newSize > MaxPrimeArrayLength)
if ((uint)newSize > MaxPrimeArrayLength && MaxPrimeArrayLength > oldSize)
{
Debug.Assert(MaxPrimeArrayLength == GetPrime(MaxPrimeArrayLength), "Invalid MaxPrimeArrayLength");
return MaxPrimeArrayLength;
}

return GetPrime(newSize);
}

// This is the maximum prime smaller than Array.MaxArrayLength
internal const int MaxPrimeArrayLength = 0x7FEFFFFD;
/// <summary>Returns approximate reciprocal of the divisor: ceil(2**64 / divisor).</summary>
/// <remarks>This should only be used on 64-bit.</remarks>
public static ulong GetFastModMultiplier(uint divisor) =>
ulong.MaxValue / divisor + 1;

/// <summary>Performs a mod operation using the multiplier pre-computed with <see cref="GetFastModMultiplier"/>.</summary>
/// <remarks>This should only be used on 64-bit.</remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static uint FastMod(uint value, uint divisor, ulong multiplier)
{
// We use modified Daniel Lemire's fastmod algorithm (https://github.com/dotnet/runtime/pull/406),
Copy link
Member

Choose a reason for hiding this comment

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

Might need a third party notices entry added. You could check the original PR that added this

Copy link
Member

Choose a reason for hiding this comment

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

// which allows to avoid the long multiplication if the divisor is less than 2**31.
Debug.Assert(divisor <= int.MaxValue);

// This is equivalent of (uint)Math.BigMul(multiplier * value, divisor, out _). This version
// is faster than BigMul currently because we only need the high bits.
uint highbits = (uint)(((((multiplier * value) >> 32) + 1) * divisor) >> 32);

Debug.Assert(highbits == value % divisor);
return highbits;
}
}
}
Loading