Skip to content

Use List.ConvertAll instead of LINQ#15140

Merged
TravisEz13 merged 2 commits into
PowerShell:masterfrom
xtqqczze:remove-linq
Apr 27, 2021
Merged

Use List.ConvertAll instead of LINQ#15140
TravisEz13 merged 2 commits into
PowerShell:masterfrom
xtqqczze:remove-linq

Conversation

@xtqqczze
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze commented Apr 2, 2021

PR Summary

PR Context

https://docs.microsoft.com/dotnet/api/system.collections.generic.list-1.convertall

PR Checklist

@ghost ghost assigned TravisEz13 Apr 2, 2021
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 5, 2021
@xtqqczze xtqqczze marked this pull request as ready for review April 6, 2021 11:20
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 6, 2021
@TravisEz13 TravisEz13 added the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Apr 8, 2021
@TravisEz13
Copy link
Copy Markdown
Member

Do we have performance results before and after for this?

cc @daxian-dbw

@TravisEz13 TravisEz13 added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 8, 2021
@xtqqczze
Copy link
Copy Markdown
Contributor Author

xtqqczze commented Apr 9, 2021

@TravisEz13 In general, there's not a large performance difference between the two approaches.

Using ConvertAll does avoid an IEnumerable allocation and means we can remove the using System.Linq.

    private List<int> list = Enumerable.Repeat(10, 10).ToList();
    public List<string> Linq()
    {
        return list.Select(t => t.ToString()).ToList();
    }
    public List<string> ConvertAll()
    {
        return list.ConvertAll(t => t.ToString());
    }

https://gist.github.com/xtqqczze/22cfeb3eb34e4ca21f4af87adf4956a5

|   Method  |     Mean |   Error |   StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated | Code Size |
|---------- |---------:|--------:|---------:|-------:|------:|------:|----------:|----------:|
|      Linq | 352.1 ns | 8.89 ns | 25.93 ns | 0.1259 |     - |     - |     528 B |    1313 B |
|ConvertAll | 295.1 ns | 9.09 ns | 26.66 ns | 0.1087 |     - |     - |     456 B |     353 B |
Gist
ConvertAllBenchmark. GitHub Gist: instantly share code, notes, and snippets.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 9, 2021
@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented Apr 10, 2021

Format working group can you review the change?

@iSazonov
Copy link
Copy Markdown
Collaborator

I wonder why do you fix only one place? I found some more with .Select( pattern.

@xtqqczze
Copy link
Copy Markdown
Contributor Author

I wonder why do you fix only one place? I found some more with .Select( pattern.

The analyzer I used only found 1 instance of the pattern: list.Select(d).ToList().

All the other locations seem to be ienumerable.Select(d).ToList().

@iSazonov
Copy link
Copy Markdown
Collaborator

The analyzer I used

Ok :-)

@xtqqczze
Copy link
Copy Markdown
Contributor Author

The analyzer I used

RCS1077

@iSazonov
Copy link
Copy Markdown
Collaborator

The analyzer I used

RCS1077

It seems it does not cover array.

@ghost ghost added the Review - Needed The PR is being reviewed label Apr 24, 2021
@ghost
Copy link
Copy Markdown

ghost commented Apr 24, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@anmenaga anmenaga removed the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Apr 27, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 27, 2021
Copy link
Copy Markdown

@anmenaga anmenaga left a comment

Choose a reason for hiding this comment

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

LGTM

@anmenaga
Copy link
Copy Markdown

Maintainers reviewed and agreed to merge this.

@TravisEz13 TravisEz13 merged commit 59715d5 into PowerShell:master Apr 27, 2021
rkeithhill pushed a commit to rkeithhill/PowerShell that referenced this pull request May 3, 2021
@xtqqczze xtqqczze deleted the remove-linq branch May 9, 2021 19:43
@daxian-dbw daxian-dbw added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label May 26, 2021
@ghost
Copy link
Copy Markdown

ghost commented May 27, 2021

🎉v7.2.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log WG-Engine-Format

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants