dotnet / dotnet/msbuild

Avoid re-evaluation when GetItemProvenance or GetAllGlobs are called

Open
#2,066 0 comments 0 reactions 0 assignees View on GitHub
Area: Performance triaged
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

[GetItemProvenance](https://github.com/Microsoft/msbuild/blob/master/src/Build/Definition/Project.cs#L1341) and [GetAllGlobs](https://github.com/Microsoft/msbuild/blob/master/src/Build/Definition/Project.cs#L1113) require the [evaluated item elements](https://github.com/Microsoft/msbuild/blob/master/src/Build/Definition/Project.cs#L3073) to work on. These represent all the item elements that the Evaluator took into consideration, and was added to support the two globbing APIs. However, recording this extra data caused a performance regression due to the new array list growth.

The "temporary" solution is to not record the item element data by default, and have the two APIs [request it on demand](https://github.com/Microsoft/msbuild/blob/master/src/Build/Definition/Project.cs#L1383) by doing another re-evaluation. API users also have the option of requesting this extra data right from the very first evaluation by [setting a flag](https://github.com/Microsoft/msbuild/blob/master/src/Build/Definition/ProjectLoadSettings.cs#L40).

This solution is not ideal because it introduces surprise re-evaluations if the API callers missed setting the flag, and because it introduces hidden dependencies throughout the code, making it harder to understand. And the perf regression is still there, the only difference is that API callers choose when to take it.

Better solutions:
- replace the growing List with a different data structure. Maybe a "block array list" which grows different from a `List`: rather than copying the small array into a larger array and disposing the old array (GC pressure, etc), keep the old array, allocate a (potentially larger) new array, and link the two. Memory pressure is still there but GC pressure should go away.
- rather than keep adding new lists in the Evaluator when some bit of code requires some new bit of data, implement a visitor over the project root elements so that API callers can do partial evaluations and just get the info they need. Tracked in #2021. This way GetItemProvenance and GetAllGlobs can just re-walk the minimal import tree to get and cache the data they need.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.