Skip to content

Use List<string> for excludes#6598

Merged
AR-May merged 2 commits into
dotnet:mainfrom
sharwell:immutable-array
Jun 25, 2021
Merged

Use List<string> for excludes#6598
AR-May merged 2 commits into
dotnet:mainfrom
sharwell:immutable-array

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 19, 2021

My preference is to use ImmutableSegmentedList<string> for this (avoids the Large Object Heap and avoids the array copy in ToImmutable()), but I haven't finished implementing it yet in Microsoft.CodeAnalysis.Collections. In the mean time, I've corrected an acute performance problem by using ImmutableArray<string> List<string> instead of ImmutableList<string>.

Fixes AB#1344683

@sharwell sharwell changed the title Use ImmutableArray<string> for excludes 🐇 Use ImmutableArray<string> for excludes Jun 19, 2021
@rainersigwald rainersigwald requested a review from ladipro June 21, 2021 14:36
@rainersigwald rainersigwald changed the title 🐇 Use ImmutableArray<string> for excludes Use ImmutableArray<string> for excludes Jun 21, 2021
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I suspect there are several places in the lazy evaluator that could benefit from switching to ImmutableArray, since they're almost entirely manipulated in the Builder.

Comment thread src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs Outdated
These lists can eventually be converted to ImmutableSegmentedList<T>.
See dotnet#6601.
@sharwell sharwell changed the title Use ImmutableArray<string> for excludes Use List<string> for excludes Jun 21, 2021
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 21, 2021
@AR-May AR-May merged commit e9593e8 into dotnet:main Jun 25, 2021
@sharwell sharwell deleted the immutable-array branch June 25, 2021 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Performance merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants