Skip to content

Reduce allocations in TaskExecutionHost #11804

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

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented May 7, 2025

Fixes #

Context

Changes Made

Testing

Notes

@Copilot Copilot AI review requested due to automatic review settings May 7, 2025 22:18
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 memory allocations in TaskExecutionHost by optimizing how task item metadata is enumerated and escaped.

  • Introduces specialized logic for handling CopyOnWriteDictionary and Dictionary types in metadata enumeration.
  • Adds a new local function to yield escaped key-value pairs with minimal allocations.

I think this parenthesis was deleted by error.
If it was a different one, more thorough look will be needed
@@ -1423,9 +1424,26 @@ private void GatherTaskItemOutputs(bool outputTargetIsItem, string outputTargetN

static IEnumerable<KeyValuePair<string, string>> EnumerateMetadata(IDictionary customMetadata)
{
foreach (DictionaryEntry de in customMetadata)
Copy link
Member

@SimaTian SimaTian May 9, 2025

Choose a reason for hiding this comment

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

What is the penalty for using the interface here please? Is it boxing in this case or something else?
(e.g. from what I read, we use specialized iterations based on type, so I assume there is something to be gained, hence the boxing question)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal here is to avoid boxing the struct enumerator of these collections. In sufficiently hot paths, the allocations can really add up.

Copy link
Member

@SimaTian SimaTian left a comment

Choose a reason for hiding this comment

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

LGTM if the parenthesis fix worked.
Otherwise the tests will fail again and revisit will be needed.

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.

3 participants