Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Use List<string> for excludes #6598

Merged
merged 2 commits into from
Jun 25, 2021
Merged

Use List<string> for excludes #6598

merged 2 commits into from
Jun 25, 2021

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Jun 19, 2021 •
edited
Loading

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
rainersigwald
rainersigwald approved these changes Jun 21, 2021
View reviewed changes
Copy link
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.

@sharwell
Use List<T> to avoid array copies
7e89aeb 
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
@sharwell sharwell mentioned this pull request Aug 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Forgind Forgind Forgind approved these changes

@ladipro ladipro ladipro approved these changes

@benvillalobos benvillalobos benvillalobos approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

Assignees
No one assigned
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sharwell @rainersigwald @benvillalobos @ladipro @Forgind @AR-May