Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[XABT] Use SortedSets in JCWGenerator.EnsureIdenticalCollections for better performance. #9208

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 16, 2024

JCWGenerator.EnsureIdenticalCollections ensures that every architecture being built contains the same Java derived types. Because this includes Mono.Android.dll, even the dotnet new android template has ~8500 Java types found. The current implementation uses nested loops and is O(N²). By first sorting the items into SortedSet types and then using SetEquals we can significantly speed up this operation.

Fresh dotnet build of Android template (defaults to 4 architectures), EnsureIdenticalCollections takes:

main 3081 ms
PR 29 ms

@jpobst jpobst marked this pull request as ready for review August 16, 2024 01:37
@jpobst jpobst requested review from jonpryor and grendello August 16, 2024 01:37
Comment on lines +248 to +249
var templateSet = new SortedSet<string> (templateState.AllJavaTypes.Select (t => t.FullName), StringComparer.Ordinal);
var typesSet = new SortedSet<string> (state.AllJavaTypes.Select (t => t.FullName), StringComparer.Ordinal);
Copy link
Member

Choose a reason for hiding this comment

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

One trick, I've been doing to ensure there are no closures:

Suggested change
var templateSet = new SortedSet<string> (templateState.AllJavaTypes.Select (t => t.FullName), StringComparer.Ordinal);
var typesSet = new SortedSet<string> (state.AllJavaTypes.Select (t => t.FullName), StringComparer.Ordinal);
var templateSet = new SortedSet<string> (templateState.AllJavaTypes.Select (static t => t.FullName), StringComparer.Ordinal);
var typesSet = new SortedSet<string> (state.AllJavaTypes.Select (static t => t.FullName), StringComparer.Ordinal);

This wouldn't compile if you captured a variable on accident.

These are so short, though, it's fine as-is.

@jonathanpeppers
Copy link
Member

I'm going to merge as-is, to better test build performance in 9.

@jonathanpeppers jonathanpeppers merged commit 339c6a1 into main Aug 16, 2024
58 checks passed
@jonathanpeppers jonathanpeppers deleted the jcw-set-performance branch August 16, 2024 15:20
jpobst added a commit that referenced this pull request Sep 4, 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.
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants