Clean up of hashset #7344
Clean up of hashset #7344
Conversation
| // This is the maximum prime smaller than Array.MaxArrayLength | ||
| internal const int MaxPrimeArrayLength = 0x7FEFFFFD; | ||
|
|
||
| public const int HashPrime = 101; |
There was a problem hiding this comment.
Where do we use this other than making sure we don't pick an almost-multiple of it?
| Array.Clear(_slots, 0, _lastIndex); | ||
| #if NET6_0_OR_GREATER | ||
| Array.Clear(_buckets); |
There was a problem hiding this comment.
Although I do think this is cleaner, adding the #ifs makes it very not worth it to me.
There was a problem hiding this comment.
This might have been done because of a slight codegen improvement. (?)
There was a problem hiding this comment.
The reason I used it was because CORE used it. When I checked the source code for it, it was a lot more simple. So it should run faster.
The Helper changes are to get in line with CORE. They added the SerializationInfoTable back in 2020 to help with serialization. The changes to Hashset are to reduce the effort of a diff. |
| { | ||
| private static ConditionalWeakTable<object, SerializationInfo>? s_serializationInfoTable; | ||
|
|
||
| public static ConditionalWeakTable<object, SerializationInfo> SerializationInfoTable |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
SerializationInfoTable is copied from CORE. It exists in core right now afaik.
There was a problem hiding this comment.
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
| [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), |
There was a problem hiding this comment.
Might need a third party notices entry added. You could check the original PR that added this
There was a problem hiding this comment.
Initial work on #7340
First changes are about making it easier to diff between CORE and MSBuild.
Imported changes to HashHelpers from CORE.
Still working on merging changes from CORE to RetrievableEntryHashSet.