Remove usages of Tuples#5270
Conversation
Remove allocation of Tuple and an array of lambdas.
The only potential risk is removing the "where V : class" generic constraint from CopyOnWriteDictionary. It is not clear to me why it needed to be a reference type.
|
FYI @svetkereMS this commit simplifies the logic in ComputeProvenanceResult. A Tuple was allocated unnecessarily and an array of lambdas with closures was also allocated unnecessarily. Hope I got the logic right. |
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
Looks mostly reasonable. Some of the files you touched could use some serious cleaning efforts.
| } | ||
|
|
||
| var matchOccurrences = ItemMatchesInItemSpecString(itemToMatch, itemSpec, elementLocation, itemElement.ContainingProject.DirectoryPath, _data.Expander, out Provenance provenance); | ||
| Tuple<Provenance, int> result = matchOccurrences > 0 ? Tuple.Create(provenance, matchOccurrences) : null; |
There was a problem hiding this comment.
Grrr...this was my part of my pet project for when I don't know what I should be working on. (It is related to an SLA bug I was assigned.)
I cleaned it up more. See here. I don't think you broke anything, though.
There was a problem hiding this comment.
let me know if you'd like me to take this commit out to not cause you conflicts
There was a problem hiding this comment.
Don't worry about it. I don't have a timeline for when my commits might turn into a PR, and this is definitely a huge improvement over what was there. The merge conflict (probably) won't be too serious.
| } | ||
|
|
||
| internal delegate Tuple<string, string, string> FixupParts( | ||
| internal delegate (string fixedDirectoryPart, string recursiveDirectoryPart, string fileNamePart) FixupParts( |
| /// defines the trigger condition. The second column defines the fallback .net and VS versions. | ||
| /// </remarks> | ||
| private static readonly Tuple<Version, Version>[,] s_explicitFallbackRulesForPathToDotNetFrameworkSdkTools = | ||
| private static readonly (Version, Version)[,] s_explicitFallbackRulesForPathToDotNetFrameworkSdkTools = |
There was a problem hiding this comment.
This only seems to be used in one for loop in this class. It would be messier, but do you think it would be reasonable to move this there so we don't always allocate it? I imagine that whole code path isn't used too often. At minimum, it could be cleaned up by replacing Item1, Item2 with better names.
There was a problem hiding this comment.
We can clean up ad infinitum, so one step at a time ;) I deliberately refrained from further cleanup, to make this conceptually easier to review. One semantic change per commit, one refactoring pass per PR ;)
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
LGTM. @cdmihai might also be interested.
| { | ||
| var newParts = fixupParts(fixedDirectoryPart, wildcardDirectoryPart, filenamePart); | ||
|
|
||
| // todo use named tuples when they'll be available |
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
Thanks for answering my perf questions!
4 participants
We allocate a lot of Tuples.
Hopefully switching to ValueTuples will simplify the code and reduce allocations.
One concern is that CopyOnWriteDictionary had the generic constraint on V to be a reference type. I don't know why that is, if there's an underlying reason then we need to think what to do there.
Also it feels like (string, ElementLocation) deserves to be its own named type. But we can leave that for later.