Skip to content

[Withdrawn] Pre-size the first Enumerable.Chunk buffer (no-go)#131028

Closed
artl93 wants to merge 1 commit into
dotnet:mainfrom
artl93:artl93-chunk-presize-115079
Closed

[Withdrawn] Pre-size the first Enumerable.Chunk buffer (no-go)#131028
artl93 wants to merge 1 commit into
dotnet:mainfrom
artl93:artl93-chunk-presize-115079

Conversation

@artl93

@artl93 artl93 commented Jul 19, 2026

Copy link
Copy Markdown
Member

Status: Withdrawn (no-go). Not merging. Recording the rationale for future reference.

What was proposed

Pre-size the first Enumerable.Chunk buffer for countable non-array sources using a non-enumerating count hint (TryGetNonEnumeratedCount), i.e. Math.Min(size, count) instead of growing 4→8→…→size via Array.Resize. (Array sources are already special-cased and were untouched.)

Why this is a no-go

  1. Previously declined by LINQ maintainers, and no new real-app evidence. Essentially this exact change — including the "fold the count hint into the regular path" variant used here — was considered and rejected in Optimize Enumerable.Chunk #73134 (2022). The stated bar was evidence that Chunk's performance "make[s] a material difference in any application" and "data that it's frequently used with countable sources." This PR offered only microbenchmarks, which does not clear that bar. Issue Enumerable.Chunk() could reduce memory allocation without reducing its performance #115079 has no maintainer buy-in.
  2. New correctness-adjacent perf regression. TryGetNonEnumeratedCount calls ICollection<T>.Count / non-generic ICollection.Count, which is O(n) and/or locking for sources like ConcurrentStack<T> and ConcurrentBag<T>. This turns source.Chunk(n).First() from O(1) into O(n) and adds an extra full pass even for complete enumeration of such sources — the opposite of a performance win.
  3. Wasted work at small chunk sizes. When size <= 4, the count hint can never beat the existing Math.Min(size, 4), so the Count call is pure overhead.

Outcome

No change to main. Closing as withdrawn. The lazy/uncountable guardrail and correctness of the prototype were confirmed, but the maintainer-precedent and the expensive-Count regression make this not worth pursuing without (a) fresh maintainer appetite on #115079 and (b) a design that restricts the hint to provably cheap count providers plus real-world justification.

Note

This pull request description was created with GitHub Copilot.

For countable non-array sources, size the first chunk buffer from a
non-enumerating count hint (TryGetNonEnumeratedCount) instead of starting
at 4 and growing 4->8->...->N via repeated Array.Resize. Lazy/uncountable
sources keep the small-start behavior and are never force-enumerated.

Fixes dotnet#115079

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d7060115-2ee5-46f4-b6e0-acad31d5a484
Copilot AI review requested due to automatic review settings July 19, 2026 07:09
@artl93

artl93 commented Jul 19, 2026

Copy link
Copy Markdown
Member Author

@EgorBot -linux_amd -osx_arm64

using System.Collections;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

// A non-array ICollection<T>: countable via TryGetNonEnumeratedCount, but exercises
// the EnumerableChunkIterator path rather than the array fast path.
public sealed class CountableCollection<T> : ICollection<T>
{
    private readonly T[] _items;
    public CountableCollection(T[] items) => _items = items;
    public int Count => _items.Length;
    public bool IsReadOnly => true;
    public IEnumerator<T> GetEnumerator() => ((IEnumerable<T>)_items).GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator();
    public bool Contains(T item) => Array.IndexOf(_items, item) >= 0;
    public void CopyTo(T[] a, int i) => _items.CopyTo(a, i);
    public void Add(T item) => throw new NotSupportedException();
    public void Clear() => throw new NotSupportedException();
    public bool Remove(T item) => throw new NotSupportedException();
}

// Genuinely lazy source with unknown count: TryGetNonEnumeratedCount returns false,
// so the source must never be force-enumerated for a count.
public sealed class LazyRange : IEnumerable<int>
{
    private readonly int _count;
    public LazyRange(int count) => _count = count;
    public IEnumerator<int> GetEnumerator()
    {
        for (int i = 0; i < _count; i++) yield return i;
    }
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

[MemoryDiagnoser]
public class Bench
{
    [Params(1024, 8192)]
    public int Count { get; set; }

    [Params(16, 256, 1024)]
    public int Size { get; set; }

    private int[] _array = default!;
    private List<int> _list = default!;
    private CountableCollection<int> _coll = default!;
    private LazyRange _lazy = default!;

    [GlobalSetup]
    public void Setup()
    {
        _array = Enumerable.Range(0, Count).ToArray();
        _list = _array.ToList();
        _coll = new CountableCollection<int>(_array);
        _lazy = new LazyRange(Count);
    }

    private static int Consume(IEnumerable<int[]> chunks)
    {
        int total = 0;
        foreach (int[] c in chunks) total += c.Length;
        return total;
    }

    [Benchmark] public int Array()      => Consume(((IEnumerable<int>)_array).Chunk(Size));
    [Benchmark] public int List()       => Consume(_list.Chunk(Size));
    [Benchmark] public int Collection() => Consume(_coll.Chunk(Size));
    [Benchmark] public int Lazy()       => Consume(_lazy.Chunk(Size));
}

Note

This benchmark request was created with GitHub Copilot.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates Enumerable.Chunk’s non-array path to pre-size the first chunk buffer based on a count hint, aiming to reduce intermediate Array.Resize growth while filling the first chunk.

Changes:

  • Use source.TryGetNonEnumeratedCount(out int countHint) to size the first buffer to Math.Min(size, countHint) when a positive hint is available.
  • Preserve the existing “start small” behavior (Math.Min(size, 4)) when no count hint is available.
Show a summary per file
File Description
src/libraries/System.Linq/src/System/Linq/Chunk.cs Pre-sizes the first chunk’s staging buffer using TryGetNonEnumeratedCount to avoid repeated growth reallocations in the first chunk for countable sources.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +80 to +86
// the array we'll yield. If the source's count is known up front, size the first chunk exactly
// (capped at the chunk size) so that filling it involves no growth/resizes. Otherwise start
// small to avoid significantly overallocating when the source has many fewer elements than the
// chunk size; the fill loop below still grows the array as needed.
int arraySize = source.TryGetNonEnumeratedCount(out int countHint) && countHint > 0 ?
Math.Min(size, countHint) :
Math.Min(size, 4);
@artl93
artl93 marked this pull request as draft July 19, 2026 07:15
@artl93 artl93 changed the title Pre-size the first Enumerable.Chunk buffer [Withdrawn] Pre-size the first Enumerable.Chunk buffer (no-go) Jul 19, 2026
@artl93

artl93 commented Jul 19, 2026

Copy link
Copy Markdown
Member Author

Closing as withdrawn (no-go). This revives a change LINQ maintainers already declined in #73134 (2022) absent evidence it matters in real applications, and it introduces an O(n)/locking Count regression via TryGetNonEnumeratedCount for sources like ConcurrentStack<T>/ConcurrentBag<T> (e.g. source.Chunk(n).First() goes O(1)→O(n)), plus a useless Count call when size <= 4. Not worth pursuing without fresh maintainer appetite on #115079 and a cheap-count-gated design. Rationale recorded in the PR description.

Note

This comment was created with GitHub Copilot.

@artl93 artl93 closed this Jul 19, 2026
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.

2 participants