Skip to content

Perf: Pool scheduler request buffers #12007

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Jun 12, 2025

Fixes

Heavy allocations due to scheduler temporary buffers.

Context

Scheduler allocates a ton of temporary lists / arrays to dump the current list of requests. This is necessary because of the semi-recursive nature of the call stack. As you see here, the largest source of allocations ResumeRequiredWork can call back into ResumeRequiredWork, which would a runtime error without the buffer as the call stack may modify the request list.

Edit: flubbed that part a bit - it's not methods calling into themselves but multiple methods which take an enumeration calling other methods which modify the target list.

image

Changes Made

This creates an instance-level ArrayPool to share buffers across methods.

Specifically this avoids ArrayPool.Shared since the implementation differences between SharedArrayPool and ConfigurableArrayPool cause the shared pool to evict the pooled arrays (my guess is that we end up exceeding the threshold due to recursive calls).

The ArrayPool is thrown out and recreated after each build via BuildManager.Reset(), so we're not just holding on to a bunch of extra memory.

After this, we end up with:

image

The runtime cost of the pool is basically trivial:

image

@Copilot Copilot AI review requested due to automatic review settings June 12, 2025 19:40
Copy link
Contributor

@Copilot 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 reduces temporary list allocations in the Scheduler by introducing an instance-level ArrayPool<SchedulableRequest> and helper methods to rent and return buffers. Key changes include:

  • Adding a private _requestBufferPool and replacing new List<…> allocations with RentPooledBuffer/ReturnPooledBuffer.
  • Implementing RentPooledBuffer and ReturnPooledBuffer methods that manually clear and return arrays.
  • Resetting the buffer pool on scheduler reset to avoid stale state.
Comments suppressed due to low confidence (2)

src/Build/BackEnd/Components/Scheduler/Scheduler.cs:2312

  • Consider adding unit tests for RentPooledBuffer and ReturnPooledBuffer to verify behavior with varying request counts and ensure buffers are cleared and returned correctly under recursion.
private SchedulableRequest[] RentPooledBuffer(IEnumerable<SchedulableRequest> requests, int count, out int numRead)

src/Build/BackEnd/Components/Scheduler/Scheduler.cs:2312

  • Add XML documentation comments for RentPooledBuffer and ReturnPooledBuffer to explain their purpose, parameters, and behavior for future maintainers.
private SchedulableRequest[] RentPooledBuffer(IEnumerable<SchedulableRequest> requests, int count, out int numRead)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant