Msbuild: Make MSBuildAllProjects magically append on import

[nguerrera]

See https://github.com/dotnet/sdk/issues/277#issuecomment-257720464

Today, every file has to add itself manually and many don't.

@rainersigwald

[danmoseley]

It should have done this from the start.

[davkean]

@rainersigwald How would you design this if this was automatic? Should it remain as a property for specific targets (such as CoreCompile)? Or just an implicit input into every target? Or some other way?

[rainersigwald]

This issue is about just augmenting the property to avoid having to append to it manually.

I don't think it should be treated as an implicit input to every target--that would be a full rebuild for every change to a property, which is correct (conservative) but inefficient. I'd rather see a more sophisticated up-to-date check that includes the consumed values like #701.

[rainersigwald]

Possible perf issue observed by @lifengl (https://github.com/Microsoft/msbuild/issues/3004#issuecomment-366406033):

In the trace, this property uses about half of the memory used by evaluation model, about 150M memory, which is quite excessive, and almost has no use in the design time.

Name                                                                                                      Inc %             Inc      Inc Ct   Exc %           Exc  Exc Ct      Fold   Fold Ct
Microsoft.Build!Microsoft.Build.Evaluation.Project                                                    28.1      339,988,704   3,931,804.750    0.0     37,928.055 862.005         0        0
+ Microsoft.Build!Microsoft.Build.Evaluation.Project+Data                                                 28.0      339,090,400   3,905,625.750    0.0    106,888.563 861.992         0        0
|+ mscorlib!List<Microsoft.Build.Evaluation.ProjectProperty>                                              15.7      190,223,296   896,655.500  0.3    3,801,356.250   1,661.694   3,780,692.250   800.686
||+ Microsoft.Build!Microsoft.Build.Evaluation.ProjectProperty+ProjectPropertyXmlBackedWithPredecessor    13.8      166,770,416   158,654.063  0.2    2,584,567.750   107,690.102         0        0
|||+ mscorlib!String (Bytes > 1K)                                                                         12.3      149,074,112   31,419.730  12.3    149,074,112 31,419.730          0        0
|||+ mscorlib!String (Bytes > 10K)                                                                         1.2       14,496,685   1,368.634    1.2     14,496,685 1,368.634           0        0
|||+ Microsoft.Build!Microsoft.Build.Construction.ProjectPropertyElement                                   0.0      543,095.375   18,021.219   0.0     23,927.461 747.733         0        0
|||+ mscorlib!String                                                                                       0.0       71,937.672     153.059    0.0     71,937.672 153.059         0        0

[davkean]

We also have this for our non-msbuild up-to-date checker: https://github.com/dotnet/project-system/issues/3744. The property is used as an input to it and we allocate 4% during solution open just splitting it up. I'd like a better way to handle this - such as just being able to get the "latest" import.

[cdmihai]

@jeffkl, what about reviving #3038 but scoping it down to only the known pattern? If users append something else to it, then the optimization gets turned off.

[rainersigwald]

IIRC a problem there was that a common target (maybe not common.targets) had the bad pattern so most existing project files wouldn't get the optimization.

[cdmihai]

so most existing project files wouldn't get the optimization.

I thought it was just c++. @jeffkl do you remember what broke the optimization? Could we fix that instead? :)

[jeffkl]

We think of MSBuildAllProjects as just imports. However, our own targets add $(MSBuildProjectFullPath).user. So having it be just imports does not work.

https://github.com/Microsoft/msbuild/blob/master/src/Tasks/Microsoft.Common.CurrentVersion.targets#L537

[cdmihai]

What if we ignore this particular breakage of the pattern? $(MSBuildProjectFullPath).user is added only when it is imported. This means that the approach in #3038 would end up including the .user file because it tracks imported projects, so it won't break this particular violation of the pattern.

[davkean]

What am I missing the .user example? It is an import - what would break by it?

[davkean]

Also, it doesn't appear that this: https://github.com/Microsoft/msbuild/pull/3038/files#diff-56a0887c6185e284af9f399c27a85c13R719 actually resolves the memory issue that @lifengl raised.

[jeffkl]

Here's the rundown of MSBuildAllProjects currently. I have a .NET Core project created with dotnet new which targets net46 and netstandard2.0.

There are three evaluations and MSBuildAllProjects is set the following number of times:

| Build Phase | Assignment Count | Final Length | Total Length |
|------------------------|------------------|--------------|--------------|
| Outer | 15 | 1,702 | 13,887 |
| Inner - net46 | 38 | 4,453 | 86,655 |
| Inner - netstandard2.0 | 35 | 4,155 | 74,515 |

This means the property is set a total of 88 times. Every reassignment is a new allocation which leads to GC pressure. Obviously the numbers will vary a little depending on the paths to the imports. MSBuild currently checks the datetime on each file to determine if the target is up-to-date and in the above example it could be looking at 38 files until it find one that is older. In the case when everything is up-to-date, its checking every single import.

With the above data, I am leaning towards deprecating MSBuildAllProjects and introducing a new property named MSBuildNewestProject which represents the path to the newest project in the import graph. Targets can then use $(MSBuildNewestProject) as an input to trigger execution if any project has changed. It feels like the intent of the MSBuildAllProjects property would live on without the overhead. Making MSBuildAllProjects magically contain all imports does not fully address the issue with it.

https://github.com/Microsoft/msbuild/pull/3593

[cdmihai]

@davkean True, this does not completely solve @lifengl's issue in https://github.com/Microsoft/msbuild/issues/3004#issue-297968315. The issue was that the opportunistic intern cache ends up capturing these large strings, as well as their entire history (every time it is appended, it gets captured in the cache). Jeff's PR eliminates the intermediary property values from getting in the cache, but does not eliminate the final value from getting there. For that, we'd need to redo our property lookup a bit and add a layer of generated properties that get instantiated only when read. Would fit as a nice extension of the property lookup mechanism mentioned in https://github.com/Microsoft/msbuild/issues/2713#issuecomment-411229035

[steffenloesch]

@jeffkl additionally projects with many custom build scripts are likely to have assignments to MSBuildAllProjects too. In my codebase, more than 10 additional assignments happen in a typical build of a single project.

[dasMulli]

Is it so bad to do it in a breaking manner (with some command-line escape hatch) that completely ignores assignments to MSBuildAllProjects and inserts MSBuildNewestProject wherever $(MSBuildAllProjects) is used by user targets?

[jeffkl]

After some discussion, we've landed on the following design:

1. Keep track of the last modified project or import
2. Prepend the path to MSBuildAllProjects
   3, Stop setting MSBuildAllProjects in the targets that ship with MSBuild
3. Go to other SDKs and stop setting MSBuildAllProjects

Eventually MSBuildAllProjects should become just the last modified import. Existing targets do not need to be updated to maximize back compat. Any SDK that needs to be backwards compatible can add a Condition when setting MSBuildAllProjects:

<PropertyGroup>
  <MSBuildAllProjects Condition=" '$(MSBuildVersion)' == '' Or '$(MSBuildVersion)' < '16.0' ">
      $(MSBuildAllProjects);$(MSBuildThisFileFullPath)
  </MSBuildAllProjects>
</PropertyGroup>

[bording]

@jeffkl Unless I'm misunderstanding your intent, wouldn't the condition in https://github.com/Microsoft/msbuild/issues/1299#issuecomment-412894038 need to be checking for < 16.0 instead? You should only need to set MSBuildAllProjects if you're on MSBuild 15 or less, right?

[jeffkl]

@bording yes, you are correct. I have updated the comment!

[davkean]

@jeffkl That's a string comparison right? MSBuild doesn't support version checks, if I remember correctly.

[davkean]

What release did this make? If I understand the change, we don't need to do anything here: https://github.com/dotnet/project-system/issues/3744 to react this right?

[danmoseley]

@davkean I think (?) it does, at least based on code like https://github.com/Microsoft/msbuild/blob/master/src/Build/Evaluation/Conditionals/GreaterThanOrEqualExpressionNode.cs#L35
https://github.com/Microsoft/msbuild/blob/master/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs#L47

[davkean]

Ah, looks like I'm conflating the old .NET Framework version checks which start with "v4.0".

[davkean]

@jeffkl What build will this appear in?

[jeffkl]

This went into master which will ship with dev16.