Several allocation fixes in Scheduler.cs

[Erarndt]

Fixes #

Context

Changes Made

Testing

Notes

[Copilot]

Pull Request Overview

This pull request addresses allocation fixes and refactors parts of the Scheduler to improve performance and correctness during request scheduling.

• Preallocates the HashSet for idle nodes based on available capacity.
• Adjusts unscheduled request list initialization and updates loop ordering in FIFO assignment.
• Refactors and modernizes static local method usage for configuration and logging.

[Copilot]

The loop order in AssignUnscheduledRequestsFIFO has been reversed, which may affect the intended FIFO scheduling behavior. Please verify that iterating unscheduled requests first and then idle nodes produces the expected request assignment order.

Suggested change

	foreach (SchedulableRequest unscheduledRequest in _schedulingData.UnscheduledRequestsWhichCanBeScheduled)
	foreach (int nodeId in idleNodes)

[Copilot]

Using _schedulingData.UnscheduledRequestsCount as the list capacity assumes it matches the actual number of unscheduled requests. Please ensure that this value is always in sync with _schedulingData.UnscheduledRequestsWhichCanBeScheduled to avoid potential resizing during list population.

Suggested change

	List<SchedulableRequest> unscheduledRequests = new List<SchedulableRequest>(_schedulingData.UnscheduledRequestsCount);
	List<SchedulableRequest> unscheduledRequests = new List<SchedulableRequest>(_schedulingData.UnscheduledRequestsWhichCanBeScheduled.Count);

[SimaTian]

This appears to have killed all the pipelines in a weird way.
If/when the last one dies, we can try re-running to see if this reproduces consistently. If yes, then a second look is in order.
Btw. how can a pipeline run for more than a day without getting killed/timing out?

[rainersigwald]

Can you elaborate on this? Doesn't the next line realize the configs?

[rainersigwald]

Oh this is copy/pasted from the original. Not sure I see the benefit of this extraction, does it cause some subtle allocation thing?

[Erarndt]

The goal here is to avoid the allocation of a closure object, and an unfortunate consequence is that if there's a capture anywhere in the body of the method, the closure object is created. This happens regardless of if the code that would use the closure is called. For example this:

    public static Action Test(int input)
    {
        Action action;

        if (input < int.MinValue)
        {
            action = () => Console.WriteLine(input);
        }
        else
        {
            action = () => { };
        }

        return action;
    }

Compiles to this:

public static Action Test(int input)
{
	<>c__DisplayClass1_0 <>c__DisplayClass1_ = new <>c__DisplayClass1_0();
	<>c__DisplayClass1_.input = input;
	if (<>c__DisplayClass1_.input < int.MinValue)
	{
		return <>c__DisplayClass1_.<Test>b__0;
	}
	return <>c.<>9__1_1 ?? (<>c.<>9__1_1 = <>c.<>9.<Test>b__1_1);
}

c__DisplayClass1_0 is unconditionally allocated.

By pulling the offending code into a local function, we avoid the unconditional allocation:

    public static Action Test(int input)
    {
        Action action;

        if (input < int.MinValue)
        {
            action = WriteConsoleAction();
        }
        else
        {
            action = () => { };
        }

        return action;

        Action WriteConsoleAction()
        {
            return () => Console.WriteLine(input);
        }
    }

Instead, we only allocate when we call the local function:

public static Action Test(int input)
{
	if (input < int.MinValue)
	{
		return WriteConsoleAction(input);
	}
	return <>c.<>9__1_0 ?? (<>c.<>9__1_0 = <>c.<>9.<Test>b__1_0);
	static Action WriteConsoleAction(int input)
	{
		<>c__DisplayClass1_0 <>c__DisplayClass1_ = new <>c__DisplayClass1_0();
		<>c__DisplayClass1_.input = input;
		return <>c__DisplayClass1_.<Test>b__2;
	}
}

Creating the Action requires capturing some state objects, and we can avoid this by moving to a static local function. I can add comments or clarify this somehow. What would help?

[Erarndt]

One note, this statement return <>c.<>9__1_0 ?? (<>c.<>9__1_0 = <>c.<>9.<Test>b__1_0); assigns the non-capturing delegate to a static field that gets reused. No additional allocations here.

[rainersigwald]

if there's a capture anywhere in the body of the method, the closure object is created.

TIL. This makes sense then. I'd remove the comment since it's meaningless in this context--the idea was "don't create the context if you won't need it" but if this method is called you need it.

[rainersigwald]

What's the motivation for the switch? Document in a comment.

[Erarndt]

Enumerating over UnscheduledRequestsWhichCanBeScheduled is more expensive than enumerating over idleNodes (HashSet<int>). To enumerate UnscheduledRequestsWhichCanBeScheduled, we allocate a new enumerator each time (from the yield return) and the linked list enumeration is slightly slower too. I can add a comment to clarify if we want to keep this change.