Avoid boxing and unnecessary array allocations

[Erarndt]

Fixes #

Context

There are several delegates called during build that cause some sneaky allocations. For instance, look at the boxing of the BuildRequestEngineStatus enum:

The culprit is the ErrorUtilities.VerifyThrow() calls that take object as parameters for the formatted message. To make the call, the enum gets boxed even when the condition check returns true. Adding additional overloads that take the specific type lets multiple callers benefit, and I did some additional cleanup in the delegates to remove some allocations.

Before:

After:

The UnblockBuildRequest delegate has no allocations

Changes Made

Testing

Notes

[Copilot]

Pull Request Overview

This PR optimizes performance by reducing boxing allocations and unnecessary array allocations in the BuildRequestEngine. The main issue addressed is the automatic boxing of BuildRequestEngineStatus enum values when passed to ErrorUtilities.VerifyThrow() calls that accept object parameters.

• Introduces a BuildRequestEngineStatusBoxes class with pre-boxed enum values and an extension method to avoid repeated boxing
• Updates ErrorUtilities.VerifyThrow() calls to use the new boxing extension method
• Adds overloaded TraceEngine() methods to prevent array allocations in debugging scenarios

Reviewed Changes

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

File	Description
IBuildRequestEngine.cs	Adds BuildRequestEngineStatusBoxes class with cached boxed enum values and Box() extension method
BuildRequestEngine.cs	Updates enum boxing calls and adds overloaded TraceEngine methods to reduce allocations

[rainersigwald]

In the future, I'd prefer to see these changes separated out; I found it confusing to see the unrelated changes in the same PR.

[Erarndt]

In the future, I'd prefer to see these changes separated out; I found it confusing to see the unrelated changes in the same PR.

Curious where/how you'd split this. The paths seemed related to me, so it seemed natural to group them.

[rainersigwald]

I'd have done

1. Introduce .Box()
2. Avoid HasFlag
3. TraceEngine overloads
4. Avoid OverallResult when unnecessary

then each is tiny, scoped, and trivial to review. Here I got confused about .Box()'s relationship to the others and wasn't sure whether things were connected.

[Erarndt]

I'd have done

1. Introduce .Box()
2. Avoid HasFlag
3. TraceEngine overloads
4. Avoid OverallResult when unnecessary

then each is tiny, scoped, and trivial to review. Here I got confused about .Box()'s relationship to the others and wasn't sure whether things were connected.

Will do! Thanks for the input

[YuliiaKovalova]

Thank you, Eric!

[YuliiaKovalova]

@Erarndt please address the remaining comments and we are ready to go

[surayya-MS]

I'd have done

1. Introduce .Box()
2. Avoid HasFlag
3. TraceEngine overloads
4. Avoid OverallResult when unnecessary

then each is tiny, scoped, and trivial to review. Here I got confused about .Box()'s relationship to the others and wasn't sure whether things were connected.

Will do! Thanks for the input

@Erarndt do you plan to make changes regarding this comment?
From the code, it seems to me that all 4 are done. So, was it about providing more info about .Box()'s relationship to the others?
I think this is good to merge.

[Erarndt]

I'd have done

1. Introduce .Box()
2. Avoid HasFlag
3. TraceEngine overloads
4. Avoid OverallResult when unnecessary

then each is tiny, scoped, and trivial to review. Here I got confused about .Box()'s relationship to the others and wasn't sure whether things were connected.

Will do! Thanks for the input

@Erarndt do you plan to make changes regarding this comment? From the code, it seems to me that all 4 are done. So, was it about providing more info about .Box()'s relationship to the others? I think this is good to merge.

Unless I misunderstood, @rainersigwald requested that I split out the separate changes in the future. No changes requested for this PR.

[rainersigwald]

Correct.