Fix cross-AppDomain TaskItem modifier cache regression

[DustinCampbell]

The ItemSpecModifiers.Cache struct I introduced with #13386 caused a surprising ~200 MB allocation regression in Visual Studio scenarios on .NET Framework. Essentially, when TaskItem (a MarshalByRefObject) cached modifiers in an embedded struct, there's a huge cost to copy that struct cross-AppDomain.

To fix the problem, reduce the Cache struct to just a single field to store the full path. This should effectively bring memory allocations back in line.

[Copilot]

Pull request overview

Fixes a .NET Framework Visual Studio allocation regression caused by copying an embedded modifier-cache struct across AppDomains by moving the cache onto the item objects themselves via an internal interface.

Changes:

• Replace ItemSpecModifiers.Cache struct with an IItemSpecModifierCache interface and update modifier computation to use it.
• Implement per-item modifier caching directly on TaskItem, ProjectItem, ProjectItemInstance.TaskItem, and TaskParameterTaskItem.
• Refactor ItemSpecModifiers.GetItemSpecModifier into caching vs non-caching overloads and extract defining-project modifier resolution.

Reviewed changes

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

Show a summary per file

File	Description
src/Utilities/TaskItem.cs	Moves derivable modifier caching onto TaskItem via IItemSpecModifierCache and clears cache on ItemSpec changes.
src/Shared/TaskParameter.cs	Updates TaskParameterTaskItem to use IItemSpecModifierCache for modifier caching.
src/Framework/ItemSpecModifiers.cs	Reworks modifier computation to support caching through IItemSpecModifierCache and adds a non-caching overload.
src/Framework/IItemSpecModifierCache.cs	Introduces internal interface for per-item derivable modifier caching across AppDomain boundaries.
src/Framework.UnitTests/FileUtilities_Tests.cs	Updates tests to use the new cache interface instead of the removed struct cache.
src/Build/Instance/ProjectItemInstance.cs	Implements cache interface on ProjectItemInstance.TaskItem, clears cache on spec changes, and copies cache on clone.
src/Build/Definition/ProjectItem.cs	Implements cache interface on ProjectItem and routes built-in metadata through the new cache API.
src/Build/Definition/BuiltInMetadata.cs	Switches built-in metadata APIs from ref Cache to IItemSpecModifierCache.

[JanProvaznik]

experimentally inserting to run speedometer at exp/dev/dustinca/fix-cross-appdomain-taskitem-cache branch (ETA results in 8h)
edit: oops you already did that when submitting pr and it was blocked on infra
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/725889

[DustinCampbell]

@JanProvaznik: It looks like this PR definitely fixes the regression. Here's the experimental insertion: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/726186

There seems to be some sort of regression around methods jitted, but I can't see how this PR would cause that.

[JanProvaznik]

Methods jitted is a limitation of the experimental insertions, these numbers definitely look good!

[JanProvaznik]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

Expert reviewer subagent launched for PR #13493. It is running in background (agent_id: expert-review-pr-13493) and will post its own review comments, inline annotations, and final verdict directly to the PR. No separate action needed from this orchestrator.

[github-actions]

24-Dimension Review Summary

Clean, well-scoped fix for a real ~200 MB allocation regression confirmed by VS experimental insertion.

What changed: The Cache struct is reduced from 6 string fields to just FullPath. RootDir, Filename, Extension, RelativeDir, and Directory are now recomputed on each access instead of cached.

Why it's correct: TaskItem inherits MarshalByRefObject on .NET Framework. Marshaling a 6-field struct cross-AppDomain required copying all reference fields, causing the regression. The recomputed modifiers are cheap (string slicing / Path helpers) relative to the marshaling cost. FullPath — the only expensive one (involves I/O-capable GetFullPath) — remains cached.

#	Dimension	Verdict
1	Backwards Compatibility	✅ LGTM — internal optimization only; external behavior unchanged
2	ChangeWave Discipline	✅ LGTM — no behavioral change, no ChangeWave needed
3	Performance & Allocation	✅ LGTM — trades cheap recomputation for expensive cross-AppDomain marshaling; validated by VS perf data
4	Test Coverage	✅ LGTM — existing tests cover correctness; cross-AppDomain regression is validated by VS insertion
5	Error Message Quality	✅ LGTM — N/A
6	Logging & Diagnostics	✅ LGTM — N/A
7	String Comparison	✅ LGTM — no changes
8	API Surface	✅ LGTM — Cache is internal
9	Target Authoring	✅ LGTM — N/A
10	Design	✅ LGTM — appropriate fix for the root cause
11	Cross-Platform	✅ LGTM — no platform-specific concerns
12	Code Simplification	✅ LGTM — net -25 lines, simpler struct
13	Concurrency	✅ LGTM — single-field struct; reference writes are atomic; computed value is deterministic
14	Naming	✅ LGTM — no changes
15	SDK Integration	✅ LGTM — N/A
16	Idiomatic C#	✅ LGTM
17	File I/O & Paths	✅ LGTM — no changes to path handling
18	Documentation	✅ NIT — see inline comment suggesting doc comment on Cache to prevent re-introducing regression
19	Build Infrastructure	✅ LGTM — N/A
20	Scope & PR Discipline	✅ NIT — trailing whitespace on line 1
21	Evaluation Model	✅ LGTM — no evaluation changes
22	Correctness & Edge Cases	✅ LGTM — compute methods are deterministic; same results as cached version
23	Dependency Management	✅ LGTM — N/A
24	Security	✅ LGTM — N/A

All 24 dimensions pass — two NITs (documentation + trailing whitespace), no blocking or major issues. Approving.

Generated by Expert Code Review (command) for issue #13493 · ● 2.9M

test

[JanProvaznik]

/backport to vs18.6

[github-actions]

Started backporting to vs18.6 (link to workflow run)