Use fast modulo in HashSet #2143
Conversation
Code changes for fastmodulo
src/libraries/System.Collections/src/System/Collections/Generic/HashSet.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Collections/src/System/Collections/Generic/HashSet.cs
Show resolved
Hide resolved
src/libraries/System.Collections/src/System/Collections/Generic/HashSet.cs
Outdated
Show resolved
Hide resolved
| @@ -1979,6 +2004,14 @@ private int InternalGetHashCode(int hashCode) | |||
| return hashCode & Lower31BitMask; | |||
| } | |||
|
|
|||
| #if BIT64 | |||
| private int FastMod(int value, int divisor) | |||
There was a problem hiding this comment.
I suggest instead to extract ref int GetBucket(uint hashCode) same as in Dictionary. That puts it in one place, including the #if BIT64 stuff. With [MethodImpl(MethodImplOptions.AggressiveInlining)] on it, as for Dictionary, it will inline and be just as efficient.
|
I think i'd feel more comfortable pushing these changes if there were was better test coverage. A lot of the bugs I found were because of tests outside of System.Collections. |
| @@ -1979,6 +2004,14 @@ private int InternalGetHashCode(int hashCode) | |||
| return hashCode & Lower31BitMask; | |||
| } | |||
|
|
|||
| #if BIT64 | |||
There was a problem hiding this comment.
I believe that BIT64 works in CoreLib only. Libraries do not have infrastructure for bitness-specific ifdefs.
There was a problem hiding this comment.
Oh! Nuts. You're right.
What do you think of moving HashSet to corelib? It looks like it would not bring a great deal: ISet.cs, HashsetEqualityComparer.cs, BitHelper.cs, and one string.
Or, does JIT inline across assemblies? If so could we call into corelib for GetFastModMultiplier and FastMod -- in 32 bit the multiplier would be passed redundantly and be ignored.
Or, we could abandon this, which would be a pity.
There was a problem hiding this comment.
I would be fine with moving the whole System.Collections as part of #2138
There was a problem hiding this comment.
Any reason why we shouldn't move HashSet in this PR, as part of making this change? Seems like everything would come out in the wash.
There was a problem hiding this comment.
We like to avoid no-op refactoring with actual changes.
There was a problem hiding this comment.
What I should have said is -- just move these 3 types, then follow up by making the change (or in the same PR and not squash)?
There was a problem hiding this comment.
I can start on either one. It would be better if I start on #2138 to get more familiar with the process.
There was a problem hiding this comment.
@danmosemsft I believe the tests are not covering all constructors.
That's not good. Are they easy cases to add? |
|
Based on the discussion, it sounds like we should close this until it can be re-done on to of a moved HashSet? |
|
Yes. Too bad. |
|
Thanks @eanova |
|
No problem! |
ghost
locked as resolved and limited conversation to collaborators
This PR addresses #1989
Code changes for fastmodulo version of HashSet. Required CI testing to make sure all builds are working and to check failure at:
src\libraries\System.Diagnostics.Process\tests\ProcessWaitingTests.cs(241,0)