Refresh and update RetrievableEntryHashset #8989
Conversation
| @@ -0,0 +1,29 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
from Hashset, untested here. And hopefully going away soon with your use of BinaryFormatter.
| ProjectItemDefinition newItemDefinition = new ProjectItemDefinition(Project, itemType); | ||
|
|
||
| ItemDefinitions.Add(newItemDefinition); | ||
| ItemDefinitions.AddOrReplace(newItemDefinition); |
| internal static void VerifyThrowInternalError(bool condition, string message, params object[] args) | ||
| { | ||
| if (s_throwExceptions && !condition) | ||
| if (!condition) |
There was a problem hiding this comment.
cleaned up this file
- remove the actual exception throwing from within VerifyThrowXX. this makes it more likely they can inline. "outside" the condition is hot.
- add string resource verify to just outside every condition, and only there. inside the condition, it's going to try to load it anyway.
- move s_throwExceptions inside the cold ThrowXX methods as well.
| using System.Runtime.ConstrainedExecution; | ||
| #endif | ||
| #endif | ||
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
basically verbatim from runtime copy
| @@ -1,219 +1,142 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
difficult to review (will be easier when other PR is merged)
essentially the dotnet/runtime version, with the MSBuild modifications, unused stuff removed, some style cleanup.
note -- we're probably a bit inconsistent about verifythrow vs debug.assert. it might be good at some future point to figure out what is actually exposed publicly. everything else should be debug.assert. and we can probably stub out some of the interface implementations if they're not publicly exposed.
There was a problem hiding this comment.
and maybe version can become debug only too.
|
|
||
| private int[] _buckets; | ||
| private Slot[] _slots; | ||
| private Entry[] _entries; | ||
| private ulong _fastModMultiplier; |
There was a problem hiding this comment.
adds 8 bytes for the multiplier but removed 4/8 bytes for the constrained comparer, which I think is reasonable to obtain when needed.
There was a problem hiding this comment.
and now I removed the 4 byte version.
| // 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> |
There was a problem hiding this comment.
this comment here is what I was referring to. it works on 32 bit, IIRC this is here because on 32 bit the multiply below is less efficient . @stephentoub there's no correctness issue here, right? they don't have the benefit of #if BIT64 in this project. My assumption is that the vast majority of their use is 64 bit.
There was a problem hiding this comment.
there's no correctness issue here, right
Correct
danmoseley
force-pushed
the
improvehashset
branch
from
9e5680e to
b9b7df9
Compare
|
I'm going to break this into smaller PR's. |
2 participants
(This is on top of #8986 which should be merged first to make this smaller.)
Since RetrievableEntryHashSet snapped Hashset code many years ago, numerous improvements have been made to Hashset for size and performance. This brings over those improvements. I compared side by side in a diff/merge tool, and copied pasted as appropriate, re-applying the special changes on the MSBuild side.
NOTE -- in Hashset, the "fastmod" optimization is only used in 64 bit builds. It can do this because it's in corelib, which has separate 32 and 64 bit builds. MSBuild of course does not, so I used this path in both bitnesses. It may be a slight deoptimization for lookups when running in a 32 bit process, hopefully outweighed by the improvement in 64 bit which is much more common.
This ought to be measured for performance -- is there a magic way to do that before/after?