Skip to content

Fix TaskParameterTaskItem serialization perf #11638

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

Merged
merged 11 commits into from
Jul 2, 2025

Conversation

ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Mar 27, 2025

Fixes

Fixes several hotspots I found while profiling RAR service serialization.

Changes Made

  • Fix effectively doubling the work in the write direction, where TaskParameterTaskItem is created to wrap an existing TaskItem, but then of its metadata is parsed one again right before serialization
  • Use MSBuildNameIgnoreCaseComparer.Default (by moving it into Framework`
  • Avoid additional call to retrieve value when enumerating dictionary returned by ITaskItem2.CloneCustomMetadataEscaped(). If this is a Utilities.TaskItem coming back from a task, doing an accessor check is non-negligible overhead due to CopyOnWriteDictionary.
  • SImplify translation logic. Prefer unidirectional ITranslator APIs over manually checking each direction or reimplementing methods, and let TaskParameterTaskItem handle its own serialization.

A bunch of small improvements but they add up. Since the out of proc TaskHost is the only piece currently using this, here's some profiles off my RAR service builds.

TaskParameterTaskItem.CopyMetadataTo()

This is often hit by ReferenceTable.SetItemMetadata in RAR. This fix here was to use IMetadataContainer.ImportMetadata() bulk set, since ImmutableDictionary performs better when operations are batched.

Before:
image
After:
image

Double ITaskItem parsing

Essentially when in the write direction the old code would construct a TaskParameterTaskItem, but later cast it to ITaskItem and parse everything out again. You can see this here where CloneCustomMetadataEscaped() is hit even though by this point we've already extracted the external TaskItem to our own instance.

Before:
image
After:
image

@ccastanedaucf
Copy link
Contributor Author

lol probably broke something while pulling this out of my RAR branch, will check UT failures later today

@ccastanedaucf ccastanedaucf force-pushed the dev/chcasta/prop-perf-2 branch from 5096b1b to 57f0123 Compare March 28, 2025 01:47
@ccastanedaucf
Copy link
Contributor Author

ccastanedaucf commented Mar 28, 2025

Updated with profile traces now that I've fully isolated it from other perf stuff. Tests passing so should be good to go 👍

@SimaTian
Copy link
Member

Quite large change to parse out so I'm not 100% sure I didn't miss something, however overall I like it.
It get's rid of a bunch of code, which is nice.
It has a performance benefit which is even nicer.
It passes tests, which is good.

Since it is only being used by TaskHost(and eventually RAR caching), I'm voting for.

@surayya-MS surayya-MS assigned surayya-MS and unassigned surayya-MS May 28, 2025
@SimaTian
Copy link
Member

SimaTian commented Jun 25, 2025

Perf DDRITs have passed:
https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/646407
There is some SDK error in pipelines but from a first glance it appears to be unrelated. Can you check it though please?
https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=11829685&view=logs&j=3095faff-5e5f-54e0-ff01-49e86a1b9dd7&t=db3cb6fb-4173-593d-fda5-ee991ff8c0ad

@ccastanedaucf
Copy link
Contributor Author

FYI I removed the unescaped item spec struct since I expect memory footprint per TaskItem to be more important now w/ the task host, and there's ways to fix that one in RAR. Everything else is good!

Copy link
Member

@YuliiaKovalova YuliiaKovalova left a comment

Choose a reason for hiding this comment

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

some of the failures from the experimental run might be relevant, asked in DM to verify.

@YuliiaKovalova YuliiaKovalova merged commit 9db3657 into dotnet:main Jul 2, 2025
9 checks passed
YuliiaKovalova added a commit that referenced this pull request Jul 3, 2025
YuliiaKovalova added a commit that referenced this pull request Jul 3, 2025
* Revert "Fix TaskParameterTaskItem serialization perf (#11638)"
ccastanedaucf added a commit to ccastanedaucf/msbuild that referenced this pull request Jul 9, 2025
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.

5 participants