Move Unused Reference analysis out of proc#54935
Move Unused Reference analysis out of proc#54935JoeRobich merged 10 commits intodotnet:release/dev16.11from
Conversation
c0efea5 to
3981164
Compare
ryzngard
left a comment
There was a problem hiding this comment.
LGTM, with minor feedback
| autoReferences ??= ImmutableHashSet<string>.Empty; | ||
|
|
||
| var targetLibraryKeys = projectAssets.Targets | ||
| .ToImmutableDictionary(t => t.Key, t => t.Value.ToImmutableDictionary(l => l.Key.Split('/')[0], l => l.Key)); |
There was a problem hiding this comment.
this def needs a comment. plus, just blindly splitting/indexing scares me. how do we know this is safe?
There was a problem hiding this comment.
I'll add a comment to the effect of "Targets contain a hashmap of Libraries keyed by {LibraryName}/{LibraryVersion} we need to split these keys and create a mapping of LibraryName to the complete library key."
|
|
||
| if (referenceType == ReferenceType.Package && itemSpecification == ".NETStandard.Library") | ||
| { | ||
| // This depenedency is large and not useful in determining whether the parent reference has been used. |
There was a problem hiding this comment.
i don't really understand the comment. why is this reference not useful in determining if the parent reference was used?
There was a problem hiding this comment.
I will update the comment, but the way I am thinking about it is that the .NETStandard.Library brings in the entire BCL as dependencies. So, if I reference Newtonsoft.Json and it references .NETStandard.Library, then it isn't useful that I would consider any use of BCL assemblies as a use of Newtonsoft.Json.
|
|
||
| builtReferences.Add(reference.ItemSpecification, reference); | ||
|
|
||
| return reference; |
There was a problem hiding this comment.
this is a bit of a strange API in that it reference both the reference, but also adds it to a dictionary. i do not like that at all :D can you just return the reference and have someone higher up add to the dictionary?
There was a problem hiding this comment.
The issue with moving it is that this method is recursively invoked when building the dependencies list.
| HashSet<string> usedProjectFileNames = new(); | ||
|
|
||
| foreach (var project in projects) | ||
| var getCompilationTasks = projects.Select(project => project.GetCompilationAsync(cancellationToken)); |
There was a problem hiding this comment.
so, ideally parallize on the project, not the compilation level. Here's why, if you do it at the compilation level, then once we're done processing that compilation, we still can't let go of it due to the task holding onto it. If we only hold onto the projec,t the project can release the compilation if not needed.
There was a problem hiding this comment.
I've decided to revert this change for now and will revisit performance in the future.
|
please dont' merge prior to addressing or responding to the updated feedback. thanks! :) |
|
@CyrusNajmabadi Thanks for the feedback! I think it is ready for another round when you have time. |
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1358121 (microsoft)
The issue was caused by dependencies with deep transitive reference trees. The fix was to avoid building a list of all assemblies brought into the compilation by the dependency and to instead traverse the tree checking for various conditions.