8000 Use `List.ConvertAll` instead of LINQ by xtqqczze · Pull Request #15140 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Use List.ConvertAll instead of LINQ #15140

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.

By 8000 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

Merged
merged 2 commits into from
Apr 27, 2021
Merged

Conversation

xtqqczze
Copy link
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
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
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
Member
TravisEz13 commented Apr 10, 2021

Format working group can you review the change?

@iSazonov
Copy link
Collaborator

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

@xtqqczze
Copy link
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
Collaborator

The analyzer I used

Ok :-)

@xtqqczze
Copy link
Contributor Author

The analyzer I used

RCS1077

@iSazonov
Copy link
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
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
@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

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
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
0