Skip to content

Conversation

@0xLogicalx
Copy link
Contributor

Replaces LINQ-based task collection initialization with direct for loops in performance-critical code paths, eliminating unnecessary intermediate allocations.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes performance-critical code paths by replacing LINQ-based collection initialization with direct for loops, eliminating unnecessary intermediate allocations from Enumerable.Range().Select() chains.

Key Changes:

  • Replaced LINQ-based task collection creation with direct for loops in two high-throughput components
  • Properly handled loop variable capture in closures to avoid common pitfalls
  • Removed unused using System.Linq directive where applicable

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Nethermind/Nethermind.Trie/BatchedTrieVisitor.cs Replaced LINQ chain with direct for loop for task creation in batched trie visiting; removed unused using System.Linq directive
src/Nethermind/Nethermind.Network/PeerManager.cs Replaced LINQ chain with direct for loop for worker task creation; correctly captured loop variable to avoid closure issues

for (int idx = 0; idx < _outgoingConnectParallelism; idx++)
{
await foreach (Peer peer in taskChannel.Reader.ReadAllAsync(_cancellationTokenSource.Token))
int workerIdx = idx;
Copy link
Member

Choose a reason for hiding this comment

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

we can probably avoid closure with something like:

for (int idx = 0; idx < _outgoingConnectParallelism; idx++)
{
    tasks.Add(RunWorker(idx));
}

async Task RunWorker(int workerIdx)
{
    await foreach (Peer peer in taskChannel.Reader.ReadAllAsync(_cancellationTokenSource.Token))
    {
        try
        {
            await SetupOutgoingPeerConnection(peer);
        }
        catch (TaskCanceledException)
        {
            if (_logger.IsDebug) _logger.Debug($"Connect worker {workerIdx} cancelled");
            break;
        }
        catch (Exception e)
        {
            if (_logger.IsError) _logger.Error($"Error setting up connection to {peer}, {e}");
        }
    }
    if (_logger.IsDebug) _logger.Debug($"Connect worker {workerIdx} completed");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I switched the loop to call a local RunWorker(int workerIdx) helper so each worker receives its own index instead of capturing the loop variable. This removes the closure while keeping the behavior the same.

@LukaszRozmej
Copy link
Member

Probably whitespace is off

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.

3 participants