Update RetrievableEntryHashSet to match changes in HashSet

[danmoseley]

MSBuild has a "RetrievableEntryHashSet" which is like a Dictionary but the entries store both key and value so it can have the small size of a hashset. There are a few other semantic changes listed in the comment on the class.

Since RetrievableEntryHashSet snapped official Hashset code 15 years ago, numerous improvements have been made to Hashset for size and performance. This brings over those improvements.

I also removed the 'version' field from these -- it's a feature of many of our collections that help ensure the enumerator isn't invalidated during enumeration. We've considered removing it altogether. In the MSBuild use case here, I believe the value if any is not worth the 4 extra bytes on the object, as IIRC there end up being a lot of these objects. I synthesized a version from the other fields that is almost as good, instead.

This brings the "fastmod" optimization which in corelib is only used in 64 bit builds. MSBuild does not build separate 32 and 64 bit builds 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?

[rainersigwald]

This ought to be measured for performance -- is there a magic way to do that before/after?

We can trivially run the Visual Studio "Perf DDRITs", by pushing to a branch with the special name exp/*. I pushed to https://github.com/dotnet/msbuild/tree/exp/danmose-hashset, which is building as https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=8076262, which will trigger the test insertion.

@AR-May is any of your test infra ready to try this? I think it's not, quite yet.

[danmoseley]

Nice. How do I see the results?

[danmoseley]

I would imagine any changes here vanish in the noise unless perhaps it's pure incremental build of a tree with no VS time in it.

When MSBuild runs in a VS build, is it 64 bit? I guess the inproc part is 32 bit

[AR-May]

The infra is not yet ready for that indeed.

@danmoseley the results are in the VS experimental PR, I tagged you there. I indeed do not see any perf improvements, I guess due to the noise as you mentioned.

[danmoseley]

@AR-May thanks. I put in the internal ticket:

We can either (1) leave the code as is (2) take it because there's some value in aligning with Hashset (3) take it because we expect there may be marginal improvements, lost here in the noise. (4) make more limited changes (5) take it at the start of preview 9.
What do you suggest? I'd be inclined to do (5) I think, I can extract a few small low risk pieces for .NET 8 and leave the rest for .NET 9 (and we could choose to not do it then)

[danmoseley]

IIRC there end up being a lot of these objects

hmm, looking at the code as it is today, there shouldn't be a huge number of these per project. So the size is relatively uninteresting. Perf may be, but apparently it's not significant enough to show up in the perf lab.

[danmoseley]

I'm going to close this as I don't see good reason for this much churn right now. I may offer some small PR's with pieces.