Cysharp / ObservableCollections

High performance observable collections and synchronized views, for WPF, Blazor, Unity.
MIT License
461 stars 36 forks source link

Simplify reverse iteration in GetEnumerator() #50

Closed bizouarn closed 1 week ago

bizouarn commented 2 weeks ago

Description

Hi, This pull request simplify the iteration logic in the method GetEnumerator(). Thank you for your attention to these changes.

Best regards, Aymeric

Changes:

Simplified the iteration logic in the method enclosed by lock (SyncRoot) by consolidating two similar foreach loops into one using a ternary operator to handle reverse iteration conditionally.

Before :

if (!reverse)
{
    foreach (var item in list)
    {
        if (filter.IsMatch(item.Item1, item.Item2))
        {
            yield return item;
        }
    }
}
else
{
    foreach (var item in list.AsEnumerable().Reverse())
    {
        if (filter.IsMatch(item.Item1, item.Item2))
        {
            yield return item;
        }
    }
}

After :

foreach (var item in reverse ? list.AsEnumerable().Reverse() : list)
{
    if (filter.IsMatch(item.Item1, item.Item2))
    {
        yield return item;
    }
}

Note

if you prefer, I can also extract the ternary into a variable before the foreach loop

neuecc commented 1 week ago

When treating a List<T> as an IEnumerable<T>, there should be performance differences. Are you considering such factors?"

bizouarn commented 1 week ago

Thanks, for your feed back and your time. This lib is very usefull. Indeed I had forgotten that. The new method was less performant. I did a benchmark. (I couldn't launch it in .netcore, so I don't have the figures for .netcore, unfortunately.) It's better to stay as it is to keep the code performant and consistent.

BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4529/22H2/2022Update)
Intel Core i7-9700K CPU 3.60GHz (Coffee Lake), 1 CPU, 8 logical and 8 physical cores
.NET SDK 9.0.100-preview.4.24267.66
  [Host]   : .NET 6.0.31 (6.0.3124.26714), X64 RyuJIT AVX2
  .NET 6.0 : .NET 6.0.31 (6.0.3124.26714), X64 RyuJIT AVX2
  .NET 8.0 : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX2
Method Job Runtime N Reverse Mean Error StdDev Median
Before .NET 8.0 .NET 8.0 10000000 False 330,079.9 us 4,589.14 us 4,068.15 us 328,910.0 us
After .NET 8.0 .NET 8.0 10000000 False 387,105.9 us 28,868.79 us 85,120.27 us 346,962.8 us
Full benchmark result

| Method | Job      | Runtime  | N        | Reverse | Mean         | Error        | StdDev       | Median       |
|------- |--------- |--------- |--------- |-------- |-------------:|-------------:|-------------:|-------------:|
| Before | .NET 6.0 | .NET 6.0 | 10000    | False   |     272.8 us |      5.00 us |      4.67 us |     272.7 us |
| After  | .NET 6.0 | .NET 6.0 | 10000    | False   |     321.3 us |      6.37 us |      6.26 us |     321.4 us |
| Before | .NET 8.0 | .NET 8.0 | 10000    | False   |     291.6 us |      5.15 us |      4.82 us |     291.2 us |
| After  | .NET 8.0 | .NET 8.0 | 10000    | False   |     294.7 us |      2.64 us |      2.47 us |     295.3 us |
| Before | .NET 6.0 | .NET 6.0 | 10000    | True    |     304.0 us |      5.89 us |      8.06 us |     303.3 us |
| After  | .NET 6.0 | .NET 6.0 | 10000    | True    |     385.6 us |     11.37 us |     33.53 us |     394.5 us |
| Before | .NET 8.0 | .NET 8.0 | 10000    | True    |     336.0 us |      6.65 us |     11.82 us |     332.8 us |
| After  | .NET 8.0 | .NET 8.0 | 10000    | True    |     345.9 us |      6.70 us |     11.74 us |     343.2 us |
| Before | .NET 6.0 | .NET 6.0 | 10000000 | False   | 440,878.6 us |  7,196.14 us |  6,009.10 us | 440,754.6 us |
| After  | .NET 6.0 | .NET 6.0 | 10000000 | False   | 471,199.8 us |  7,577.14 us |  7,087.66 us | 472,882.4 us |
| Before | .NET 8.0 | .NET 8.0 | 10000000 | False   | 330,079.9 us |  4,589.14 us |  4,068.15 us | 328,910.0 us |
| After  | .NET 8.0 | .NET 8.0 | 10000000 | False   | 387,105.9 us | 28,868.79 us | 85,120.27 us | 346,962.8 us |
| Before | .NET 6.0 | .NET 6.0 | 10000000 | True    | 471,214.2 us |  9,396.19 us | 17,877.22 us | 465,806.8 us |
| After  | .NET 6.0 | .NET 6.0 | 10000000 | True    | 478,220.9 us |  6,078.30 us |  4,745.54 us | 478,277.1 us |
| Before | .NET 8.0 | .NET 8.0 | 10000000 | True    | 374,064.3 us |  6,432.80 us |  5,702.51 us | 374,230.6 us |
| After  | .NET 8.0 | .NET 8.0 | 10000000 | True    | 373,468.3 us |  7,286.43 us |  6,815.73 us | 370,748.4 us |
Benchmark code

[SimpleJob(RuntimeMoniker.Net60)]
[SimpleJob(RuntimeMoniker.Net80)]
public class EnumerateBench
{
    public List<string> List;

    [Params(10000, 10000000)] public int N;
    [Params(false, true)] public bool Reverse;

    [GlobalSetup]
    public void GlobalSetup()
    {
        List = new List();
        for (var i = 0; i < N; i++) List.Add(Guid.NewGuid().ToString());
    }

    [Benchmark]
    public void Before()
    {
        var ret = string.Join(',', BeforeGetEnumerable());
    }

    [Benchmark]
    public void After()
    {
        var ret = string.Join(',', AfterGetEnumerable());
    }

    public IEnumerable BeforeGetEnumerable()
    {
        if (!Reverse)
        {
            foreach (var item in List)
                if (IsMatch(item))
                    yield return item;
        }
        else
        {
            foreach (var item in List.AsEnumerable().Reverse())
                if (IsMatch(item))
                    yield return item;
        }
    }

    public IEnumerable AfterGetEnumerable()
    {
        foreach (var item in Reverse ? List.AsEnumerable().Reverse() : List)
            if (IsMatch(item))
                yield return item;
    }

    public bool IsMatch(string item)
    {
        return true;
    }
}

I am closing the pull request then.