Skip to content

Use List.ConvertAll instead of LINQ - #15140

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

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

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Apr 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

PR Context

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

PR Checklist

@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 Travis Plunk (TravisEz13) added the Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers label Apr 8, 2021
@TravisEz13

Travis Plunk (TravisEz13) commented Apr 8, 2021

Copy link
Copy Markdown
Member

Do we have performance results before and after for this?

cc Dongbo Wang (@daxian-dbw)

@TravisEz13 Travis Plunk (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

xtqqczze commented Apr 9, 2021 •
edited by unfurl-links Bot
Loading

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@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

Travis Plunk (TravisEz13) commented Apr 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Format working group can you review the change?

@iSazonov

Ilya (iSazonov) commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

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

@xtqqczze

xtqqczze commented Apr 16, 2021

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

Ilya (iSazonov) commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

The analyzer I used

Ok :-)

@xtqqczze

xtqqczze commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

The analyzer I used

RCS1077

@iSazonov

Ilya (iSazonov) commented Apr 16, 2021

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

Deleted user (ghost) commented Apr 24, 2021

Copy link
Copy Markdown

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 Andrew (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
anmenaga
Andrew (anmenaga) approved these changes Apr 27, 2021
View reviewed changes

@anmenaga Andrew (anmenaga) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@anmenaga

Andrew (anmenaga) commented Apr 27, 2021

Copy link
Copy Markdown

Maintainers reviewed and agreed to merge this.

@TravisEz13
Travis Plunk (TravisEz13) merged commit 59715d5 into PowerShell:master Apr 27, 2021
Keith Hill (rkeithhill) pushed a commit to rkeithhill/PowerShell that referenced this pull request May 3, 2021
@xtqqczze @rkeithhill
Use List.ConvertAll instead of LINQ (PowerShell#15140)
812817b
@xtqqczze
xtqqczze deleted the remove-linq branch May 9, 2021 19:43
@daxian-dbw Dongbo Wang (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

Deleted user (ghost) commented May 27, 2021

Copy link
Copy Markdown

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

Handy links:

@xtqqczze xtqqczze mentioned this pull request Nov 11, 2021
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
@xtqqczze
Use List.ConvertAll instead of LINQ (PowerShell#15140)
e41d3b2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
+1 more reviewer
@anmenaga Andrew (anmenaga) anmenaga approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 Travis Plunk (TravisEz13)

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xtqqczze @TravisEz13 @iSazonov @anmenaga @daxian-dbw