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.

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

use collection expression spread operator #6978

Conversation

testfirstcoder
Copy link
Contributor

  • avoid additional array allocation and conversion
  • avoid implicit checking of duplicates using Union method

@mikeminutillo
Copy link
Member

Hi @testfirstcoder ,

You raised a similar PR last week #6969

That one was closed as not deduplicating, which this one also does not do.

What issue are you trying to fix with these changes?

@testfirstcoder
Copy link
Contributor Author

testfirstcoder commented Mar 21, 2024

Hi @mikeminutillo,

it's not a particular issue fixing but rather a performance related one.

[SimpleJob(RuntimeMoniker.Net80)]
[MemoryDiagnoser]
public class Benchmark
{
    private int[] visitedNodes;

    private const int NewNode = -1;

    [Params(0, 1, 10)]
    public int N;

    public Benchmark() => visitedNodes = Enumerable.Range(start: 1, count: N).ToArray();

    [Benchmark]
    public void Union_ToArray()
    {
        int[] newVisitedNodes = visitedNodes.Union(new[] { NewNode }).ToArray();
    }

    [Benchmark]
    public void Append_ToArray()
    {
        int[] newVisitedNodes = visitedNodes.Append(NewNode).ToArray();
    }

    [Benchmark]
    public void Collection_Expression_Spread_Operator()
    {
        int[] newVisitedNodes = [.. visitedNodes, NewNode];
    }
}

void Main() => BenchmarkRunner.Run<Benchmark>();

// * Summary *

BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4170/22H2/2022Update)
Intel Core i7-10700 CPU 2.90GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.202
[Host] : .NET 8.0.3 (8.0.324.11423), X64 RyuJIT AVX2

Job=.NET 8.0 Runtime=.NET 8.0

Method N Mean Error StdDev Median Gen0 Allocated
Union_ToArray 0 83.037 ns 1.6542 ns 1.6988 ns 83.384 ns 0.0401 336 B
Append_ToArray 0 47.313 ns 0.8261 ns 0.7727 ns 47.171 ns 0.0105 88 B
Collection_Expression_Spread_Operator 0 4.394 ns 0.1457 ns 0.4296 ns 4.241 ns 0.0038 32 B
Union_ToArray 1 79.278 ns 1.4234 ns 2.9077 ns 79.304 ns 0.0401 336 B
Append_ToArray 1 49.852 ns 0.8911 ns 1.1587 ns 50.158 ns 0.0105 88 B
Collection_Expression_Spread_Operator 1 4.527 ns 0.1248 ns 0.3622 ns 4.508 ns 0.0038 32 B
Union_ToArray 10 78.397 ns 1.5664 ns 2.3446 ns 78.125 ns 0.0401 336 B
Append_ToArray 10 46.757 ns 0.9523 ns 2.0087 ns 46.258 ns 0.0105 88 B
Collection_Expression_Spread_Operator 10 4.166 ns 0.1129 ns 0.3203 ns 4.089 ns 0.0038 32 B

@testfirstcoder
Copy link
Contributor Author

testfirstcoder commented Mar 21, 2024

Is an implicit deduplication using Union necessary? Could visitedNodes have some duplicates?

There is an explicit duplication check here, isn't?

if (visitedNodes.Any(n => n == node))
{
    return true;
}

@WilliamBZA WilliamBZA merged commit fa2c2fa into Particular:master Mar 28, 2024
3 checks passed
@andreasohlund andreasohlund added this to the 9.0.0 milestone Apr 5, 2024
@bording bording removed this from the 9.0.0 milestone Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants