Skip to content

Conversation

@jonathanpeppers
Copy link
Member

Looking at ResolveAssemblies, I noticed a lot of LINQ usage that
could be simplified.

We were using HashSet<string> that contains a list of all
assemblies, and the following code:

if (!topLevel && assemblies.All (a => new AssemblyNameDefinition (a, null).Name != assembly.Name.Name))
    assemblies.Add (fullPath);

This was called from a recursive function and needlessly calls
new AssemblyNameDefinition ().Name for every other assembly...
Roughly a O(N^2) operation.

So I changed the HashSet<string> to be a Dictionary<string, string>, where the "key" is the assembly.Name.Name and the value
the full path to the assembly.

I also fixed up usage of assembly.MainModule.FullyQualifiedName,
which was deprecated mentioning to use FileName instead.

Lastly, I fixed up several LINQ expressions on the Outputs of
ResolveAssemblies. I rewrote them to use a single foreach loop
instead of the equivalent LINQ that would loop over the list multiple
times.

Results

The build time improvements here were "not zero", but also not huge.
Testing the tests/Xamarin.Forms-Performance-Integration project, a
build with no changes...

Before:

188 ms  ResolveAssemblies                          1 calls

After:

181 ms  ResolveAssemblies                          1 calls

I think this change is still worthwhile, since:

  • The code is a bit cleaner, less LINQ for others to profile/look at,
    and I plan to refactor this task more soon.
  • The time savings is part of the dev-loop, and even builds with no
    changes.
  • There should be overall less allocations.

Other changes:

  • Removed extraneous log messages for inputs/outputs

Looking at `ResolveAssemblies`, I noticed a lot of LINQ usage that
could be simplified.

We were using `HashSet<string>` that contains a list of all
assemblies, and the following code:

    if (!topLevel && assemblies.All (a => new AssemblyNameDefinition (a, null).Name != assembly.Name.Name))
        assemblies.Add (fullPath);

This was called from a *recursive* function and needlessly calls
`new AssemblyNameDefinition ().Name` for every other assembly...
Roughly a O(N^2) operation.

So I changed the `HashSet<string>` to be a `Dictionary<string,
string>`, where the "key" is the `assembly.Name.Name` and the value
the full path to the assembly.

I also fixed up usage of `assembly.MainModule.FullyQualifiedName`,
which was deprecated mentioning to use `FileName` instead.

Lastly, I fixed up several LINQ expressions on the `Outputs` of
`ResolveAssemblies`. I rewrote them to use a single `foreach` loop
instead of the equivalent LINQ that would loop over the list multiple
times.

~~ Results ~~

The build time improvements here were "not zero", but also not huge.
Testing the `tests/Xamarin.Forms-Performance-Integration` project, a
build with no changes...

Before:

    188 ms  ResolveAssemblies                          1 calls

After:

    181 ms  ResolveAssemblies                          1 calls

I think this change is still worthwhile, since:
- The code is a bit cleaner, less LINQ for others to profile/look at,
  and I plan to refactor this task more soon.
- The time savings is part of the dev-loop, and even builds with no
  changes.
- There should be overall less allocations.

Other changes:
- Removed extraneous log messages for inputs/outputs
@dellis1972 dellis1972 requested a review from jonpryor September 11, 2018 07:51
@dellis1972
Copy link
Contributor

Looks ok to me. @jonpryor might want to give this a review as well though :)

@jonpryor jonpryor merged commit 38bc7d0 into dotnet:master Sep 11, 2018
@jonathanpeppers jonathanpeppers deleted the resolveassemblies-linq branch September 11, 2018 13:47
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 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.

3 participants