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

[XABT] Replace SortedSet with HashSet. #9280

Merged
merged 1 commit into from
Sep 4, 2024
Merged

[XABT] Replace SortedSet with HashSet. #9280

merged 1 commit into from
Sep 4, 2024

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Sep 3, 2024

One question that came up with #9208 was "why SortedSet instead of HashSet"?

Indeed a HashSet seems to be 10ms - 15ms faster to construct than the SortedSet while taking the same ~1ms to perform the SetEquals operation.

@jpobst jpobst marked this pull request as ready for review September 4, 2024 00:31
@jpobst jpobst requested review from jonpryor and grendello September 4, 2024 00:31
Copy link
Contributor

@dellis1972 dellis1972 left a comment

Choose a reason for hiding this comment

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

Looks ok.

I do wonder if there is a faster way to do this. We just need to know if templateState and state have identical data.
Using the current code we loop throught the lists at least 4 times

2 to create the hashsets.
The two more when we do the comparisons (luckily we use the fast path in this case because they have the same item count and the same comparer).

I wonder if it would be better to generate a hash of each set and compare that, but the items would need to be processed in order for that to work.

@grendello
Copy link
Contributor

grendello commented Sep 4, 2024

Using a running hash or checksum might not be accurate enough, as the integer types we'd use for them have limited range. However, what I think might speed things up a little bit more is not using LINQ to populate the HashSet<> instances. LINQ (even in such simple instances like here) hides a lot of overhead, a for loop might be faster.

@jpobst
Copy link
Contributor Author

jpobst commented Sep 4, 2024

For the dotnet new android case, which contains ~8500 JLO's due to Mono.Android.dll, the entire operation takes about 5 ms, so there isn't much more performance to squeeze here. 😉

If anything, we could probably omit this step for a Debug build. It is largely just ensuring that something that "can't happen" isn't happening. It's probably safe to skip this for Debug and only do it on Release builds. But that's a bigger change...

@grendello
Copy link
Contributor

For the dotnet new android case, which contains ~8500 JLO's due to Mono.Android.dll, the entire operation takes about 5 ms, so there isn't much more performance to squeeze here. 😉

If anything, we could probably omit this step for a Debug build. It is largely just ensuring that something that "can't happen" isn't happening. It's probably safe to skip this for Debug and only do it on Release builds. But that's a bigger change...

Yeah, it's a good idea to omit it for Debug, the assemblies there should be identical.

@jpobst jpobst merged commit d6213d5 into main Sep 4, 2024
56 of 58 checks passed
@jpobst jpobst deleted the hashset branch September 4, 2024 17:33
@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants