Project Cache initial implementation #5936

cdmihai · 2020-12-07T00:49:06Z

Ready for review. Start with the documentation which should describe all the big changes here.

Todos:

Unit tests
RPS run
Create issue: an async LoggingService misses events logged by Plugin.EndBuildAsync when Plugin.EndBuildAsync also throws an exception

Issues addressed in future PRs:

constrain proxy target builds to inproc nodes, as a perf optimization.
parent plugin log events under the queried project's logging context so they show nicely in the binlog
add option to query the plugin against all graph nodes without building anything (cache warmup)
pipe MSBuildFileSystem to the plugin for both graph and non graph scenarios.

Since it's a bigger PR you might consider using CodeFlow to make it easier to review, it has a chrome extension: https://www.1eswiki.com/wiki/CodeFlow_integration_with_GitHub_Pull_Requests (msft internal link)

cdmihai · 2020-12-08T02:05:29Z

src/Build/BackEnd/Components/ProjectCache/CacheResult.cs

+        CacheHit,
+        CacheMiss,
+        CacheNotApplicable,
+        CacheError


Is it worth keeping CacheError? I'm thinking of dropping it in favor of just checking PluginLoggerBase.HasLoggedErrors. If we keep both, what should the user or msbuild do if the plugin returns CacheError but no errors are logged? Seems simpler to just keep one mechanism for signaling errors.

Yeah I agree, the way its mostly done is its success unless an error was logged.

+1 for using PluginLoggerBase.HasLoggedErrors

K, I'll remove CacheError

So it turns out CacheError may have a good use: when the plugin is queried for a project but it encounters and logs one or more errors, what should the plugin return? It could either return a null CacheResult or a CacheResult of result type CacheError (returning a CacheMiss or CacheNotApplicable doesn't seem a good fit for the error case). But then there's the above ambiguity when a plugin could return CacheError without logging any errors, which I guess could be fine.

An alternative is to remove the enum entirely. On cache hits the plugin returns a CacheResult with the build results (or proxy targets and whatnot). On anything else (cache miss, cache not applicable, cache error) the plugin returns a null CacheResult and logs why it couldn't satisfy the request. Worst case we get a plugin that returns null and does not log anything.

I am inclined now to keep CacheError in order to make the return modes explicit.

Let me know what you think.

I'm slightly more inclined to return null on any failure case. The main question in my mind is whether you'd react differently to CacheMiss, CacheNotApplicable, and CacheError, but the right response, from what I can tell, is to pretend the cache doesn't exist and build normally for all three cases. May as well just have a single "build everything" return value.

Decided to just replace CacheError with a None value. Hit, Miss, and NA are domain level concepts that sort of define the meaning of a plugin so I like to have them explicit. Without them, we'd rely on arbitrary plugin implementations on how they express these 3 possibilities, versus MSBuild giving a standardized message for each type of response. Another concrete use case for having them is to enable MSBuild to report the cache hit ratio in the build summary (hits / (hits + misses), need NA to not count them as misses). That's a very useful metric for cacheability health (which, just like perf, is something that tends to degrade over time).

cdmihai · 2020-12-11T17:07:51Z

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -962,117 +1000,300 @@ internal void ExecuteSubmission(BuildSubmission submission, bool allowMainThread
            ErrorUtilities.VerifyThrowArgumentNull(submission, nameof(submission));
            ErrorUtilities.VerifyThrow(!submission.IsCompleted, "Submission already complete.");

-            lock (_syncLock)
+            if (ProjectCacheIsPresent())


Start review here. This method is the entrypoint from where the logic starts diverting when a cache is available.

src/Build/Microsoft.Build.csproj

src/Build/BackEnd/BuildManager/BuildParameters.cs

src/Build/BackEnd/BuildManager/BuildSubmission.cs

src/Build/BackEnd/Components/ProjectCache/CacheContext.cs

jeffkl · 2020-12-19T00:00:37Z

src/Build/BackEnd/Components/ProjectCache/CacheResult.cs

+        CacheHit,
+        CacheMiss,
+        CacheNotApplicable,
+        CacheError


Yeah I agree, the way its mostly done is its success unless an error was logged.

src/Build/BackEnd/Components/ProjectCache/CacheResult.cs

src/Build/BackEnd/Components/ProjectCache/ProjectCacheBase.cs

src/Build/BackEnd/Components/ProjectCache/ProjectCacheDescriptor.cs

src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs

benvillalobos

WIP review

benvillalobos · 2020-12-22T20:24:11Z

src/Build/BackEnd/Components/ProjectCache/CacheResult.cs

+        CacheHit,
+        CacheMiss,
+        CacheNotApplicable,
+        CacheError


+1 for using PluginLoggerBase.HasLoggedErrors

benvillalobos · 2020-12-22T20:39:26Z

src/Build/BackEnd/BuildManager/BuildParameters.cs

@@ -10,7 +10,9 @@
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Collections;
 using Microsoft.Build.Evaluation;
+using Microsoft.Build.Experimental.ProjectCache;


how long do we plan on keeping this in the Experimental namespace?

I'd like to keep it at least until we validate it working for two of our internal build accelerators.

src/Build/BackEnd/Components/ProjectCache/PluginLoggerBase.cs

src/Build/BackEnd/BuildManager/BuildManager.cs

benvillalobos · 2020-12-23T00:10:03Z

src/Build/BackEnd/BuildManager/BuildManager.cs

+                        })
+                    .ToArray());
+
+            var cacheItems = nodeToCacheItems.Values.SelectMany(i => i).ToHashSet();


What's the purpose of SelectMany here?

I'm not sure I follow why the conversion to an array then a hashset, can't the list of items be stored as a hashset to begin with?

It's inconsequential, I think. Each node can declare zero or more plugins, so first I construct a dictionary from a node to the collection of plugins it declares. But the contract is that there can be a single plugin (path + plugin settings), and all nodes must declare that plugin. So in order to find that single plugin I flatten (SelectMany) the collections of plugins from each node into a single set (ProjectCacheItem implements Equals and GetHashCode to make this correct). I could have skipped the dictionary by flattening everything from the start, but I also want to give a nice error message with all the nodes that may be missing the plugin. And given that this method is not on a hot path, I went with a lot of LINQ :)

I'm not sure I follow everything here. Wouldn't this be union rather than intersection, so wouldn't this find all the plugins declared by any node? How does this help you find the one from all of them? And I'm not sure how I follow how this turns into an error.

This part constructs a dictionary from every node to all the declared plugins in that node:
https://github.com/cdmihai/msbuild/blob/acb4b7e2e0ffbd4a9df13c6fba11dd6aa5f37944/src/Build/BackEnd/BuildManager/BuildManager.cs#L1866-L1882

This part flattens (via SelectMany) all the declared plugins into a single set which removes the duplicates according to the overriden equals and hashcode in ProjectCacheItem.
https://github.com/cdmihai/msbuild/blob/acb4b7e2e0ffbd4a9df13c6fba11dd6aa5f37944/src/Build/BackEnd/BuildManager/BuildManager.cs#L1884

The set should contain a single item if all nodes declare a single plugin (plugin path + plugin settings).

Error cases:

set contains more than 1 plugins (error prints all declared plugins)

set contains a single plugin but there are nodes which do not declare it (error prints all nodes that do not declare the plugin)

Neat! Didn't realize SelectMany also flattened. If I were writing it, I might have said nodeToCacheItems.Values.Aggregate((x, y) => x.Union(y));, but I don't think that's better (or worse) than what you currently have.

I was originally confused in thinking that a given node could declare multiple plugins, and it was ok as long as only one of those was declared by all nodes, but I see that was wrong. Makes more sense now.

accidentally approved

cdmihai · 2020-12-29T22:53:59Z

/azp run #Resolved

azure-pipelines · 2020-12-29T22:54:08Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

Forgind

I've essentially looked at documentation + BuildManager so far. I know we don't agree about all the omnisharp styling things, but it also shows up in my diff without whitespace changes, which makes it more confusing.

documentation/specs/project-cache.md

src/Build/BackEnd/BuildManager/BuildManager.cs

Forgind · 2020-12-30T00:19:25Z

src/Build/BackEnd/BuildManager/BuildManager.cs

+                var solutionPath = config.Project.GetPropertyValue(SolutionProjectGenerator.SolutionPathPropertyName);
+
+                ErrorUtilities.VerifyThrow(
+                    solutionPath != null && !string.IsNullOrWhiteSpace(solutionPath) && solutionPath != "*Undefined*",


If someone opens a single project rather than a solution, we should be able to use that as an "entrypoint" rather than the solution, right? Good fallback?

Good point. I'll check and see what global properties VS sets in that case.

VS lies when opened on a single project and reports a non existing solution path. For now I'll leave it as is and reconsider if it turns to be a common case.

src/Build/BackEnd/BuildManager/BuildManager.cs

Forgind · 2020-12-30T00:53:14Z

src/Build/BackEnd/BuildManager/BuildManager.cs

+                        })
+                    .ToArray());
+
+            var cacheItems = nodeToCacheItems.Values.SelectMany(i => i).ToHashSet();


I'm not sure I follow everything here. Wouldn't this be union rather than intersection, so wouldn't this find all the plugins declared by any node? How does this help you find the one from all of them? And I'm not sure how I follow how this turns into an error.

Forgind

This is a little large to review too deeply, but I think I have a reasonable surface-level understanding now, thanks! I recognize that you wanted to imitate what is returned from an actual build (which clearly would allocate a lot), but I'm wondering if @ladipro might want to look, since he's been working to reduce (even temporary) allocations.

src/Build/BackEnd/Components/ProjectCache/CacheContext.cs

Forgind · 2020-12-30T17:12:09Z

src/Build/BackEnd/Components/ProjectCache/CacheResult.cs

+        CacheHit,
+        CacheMiss,
+        CacheNotApplicable,
+        CacheError


I'm slightly more inclined to return null on any failure case. The main question in my mind is whether you'd react differently to CacheMiss, CacheNotApplicable, and CacheError, but the right response, from what I can tell, is to pretend the cache doesn't exist and build normally for all three cases. May as well just have a single "build everything" return value.

src/Build/BackEnd/Components/ProjectCache/CacheResult.cs

src/Build/BackEnd/Components/ProjectCache/PluginLoggerBase.cs

src/Samples/ProjectCachePlugin/MockCacheFromAssembly.cs

src/Samples/ProjectCachePlugin/ProjectCachePlugin.csproj

src/Shared/CollectionHelpers.cs

src/Shared/UnitTests/MockLogger.cs

src/Shared/UnitTests/TestEnvironment.cs

Forgind · 2020-12-30T23:55:56Z

src/Build/BackEnd/BuildManager/BuildManager.cs

+                        })
+                    .ToArray());
+
+            var cacheItems = nodeToCacheItems.Values.SelectMany(i => i).ToHashSet();


Neat! Didn't realize SelectMany also flattened. If I were writing it, I might have said nodeToCacheItems.Values.Aggregate((x, y) => x.Union(y));, but I don't think that's better (or worse) than what you currently have.

I was originally confused in thinking that a given node could declare multiple plugins, and it was ok as long as only one of those was declared by all nodes, but I see that was wrong. Makes more sense now.

azure-pipelines · 2021-01-04T23:24:24Z

No pipelines are associated with this pull request.

cdmihai · 2021-01-07T01:29:58Z

/azp run

Also cleanup needless double interface declarations

Merge had some failing preconditions that do not apply in this case

… EndBuild

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

This reverts commit 2d35e8c.

benvillalobos

Posting comments as I go. Haven't quite dug into the "meat and potatoes" yet.

src/Build/Resources/Strings.resx

benvillalobos · 2021-01-13T20:53:25Z

src/Build.UnitTests/Graph/IsolateProjects_Tests.cs


-            using (var buildManagerSession = new Helpers.BuildManagerSession(_env, _buildParametersPrototype))
+            using (var buildManagerSession = new Helpers.BuildManagerSession(_env, buildParameters))


why do we need to use a copy of the buildparameters here?

Because the BuildManagerSession mutates it to add some loggers and other BM cleanup options to avoid impacting other tests.

rainersigwald

Haven't gotten through everything here but no objection. Merge away while I'm out :)

rainersigwald · 2021-01-12T22:19:14Z

src/Samples/ProjectCachePlugin/ProjectCachePlugin.csproj

+    <CopyNuGetImplementations>false</CopyNuGetImplementations>
+    <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
+
+    <TargetFrameworks>netcoreapp2.1</TargetFrameworks>


note collision here @benvillalobos

Forgind

Not 100% confident, but I don't think any of the new new code will run unless the user specifies it.

cdmihai requested review from rainersigwald and jeffkl December 7, 2020 00:49

cdmihai added the WIP Work in Progress Pull Request--do not merge yet. label Dec 7, 2020

cdmihai changed the title ~~[WIP][Project cache] Initial implementation~~ [Project cache] Initial implementation Dec 7, 2020

cdmihai added the Area: Project Cache label Dec 7, 2020

cdmihai changed the title ~~[Project cache] Initial implementation~~ Project Cache initial implementation Dec 7, 2020

cdmihai commented Dec 8, 2020

View reviewed changes

cdmihai commented Dec 11, 2020

View reviewed changes

jeffkl reviewed Dec 19, 2020

View reviewed changes

benvillalobos previously approved these changes Dec 23, 2020

View reviewed changes

benvillalobos self-requested a review December 23, 2020 00:19

cdmihai force-pushed the projectCache branch 7 times, most recently from d6193b9 to d688951 Compare December 28, 2020 23:56

cdmihai removed the WIP Work in Progress Pull Request--do not merge yet. label Dec 29, 2020

cdmihai force-pushed the projectCache branch from d260ada to a196184 Compare December 29, 2020 22:00

cdmihai force-pushed the projectCache branch from a196184 to acb4b7e Compare December 29, 2020 23:04

Forgind reviewed Dec 30, 2020

View reviewed changes

Forgind reviewed Dec 31, 2020

View reviewed changes

cdmihai force-pushed the projectCache branch 2 times, most recently from ff9d2fa to eca9219 Compare December 31, 2020 21:20

cdmihai and others added 22 commits January 11, 2021 13:26

Update System.Text.Json

2d35e8c

Item and metadata interfaces allow retrieving metadata

c5dd170

Also cleanup needless double interface declarations

Project cache plugin implementation

3a053d5

Error if not all nodes define a cache item

3ca8cc6

ProjectCacheDescriptor can be directly given an instance

c40a18a

Add BuildResult and TargetResult comparison helpers

2f4ca38

Manually add targets instead of Merge

357a69c

Merge had some failing preconditions that do not apply in this case

Avoid null pointer exception in BuildParameters

af241b9

Add more error messages and enable nullable references

e1ed8ca

Ensure cache exception get either into a BuildResult or get thrown in…

71a9bfa

… EndBuild

Tests

2b6b580

Redistribute Microsoft.Bcl.Hashcode

060b73e

ProjectCacheBase -> ProjectCachePluginBase

55ff5cd

ToString -> GetDetailedDescription

3a54fe9

Feedback

69c6e43

Feedback

a344404

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Update src/Build/Resources/Strings.resx

70e8be6

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Feedback

f1b5baf

CacheError -> None

87f0af7

Rename mock caches

a7c5f3d

feedback

151d47a

Remove Microsoft.Bcl.HashCode

7e9f578

cdmihai force-pushed the projectCache branch from ffaa0e2 to 7e9f578 Compare January 11, 2021 21:27

Revert "Update System.Text.Json"

9dcbf63

This reverts commit 2d35e8c.

benvillalobos mentioned this pull request Jan 13, 2021

Switch eng/common to linguist-vendored #6022

Merged

benvillalobos reviewed Jan 13, 2021

View reviewed changes

Feedback

bd0bcd3

rainersigwald reviewed Jan 15, 2021

View reviewed changes

Forgind approved these changes Jan 15, 2021

View reviewed changes

cdmihai merged commit ab9a839 into dotnet:master Jan 15, 2021


		using (var buildManagerSession = new Helpers.BuildManagerSession(_env, _buildParametersPrototype))
		using (var buildManagerSession = new Helpers.BuildManagerSession(_env, buildParameters))

Project Cache initial implementation #5936

Project Cache initial implementation #5936

Conversation

cdmihai commented Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai commented Dec 29, 2020 • edited by benvillalobos Loading

azure-pipelines bot commented Dec 29, 2020 • edited by benvillalobos Loading

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-pipelines bot commented Jan 4, 2021

cdmihai commented Jan 7, 2021

benvillalobos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

cdmihai commented Dec 7, 2020 •

edited

Loading

cdmihai Dec 29, 2020 •

edited

Loading

cdmihai commented Dec 29, 2020 •

edited by benvillalobos

Loading

azure-pipelines bot commented Dec 29, 2020 •

edited by benvillalobos

Loading

cdmihai Jan 13, 2021 •

edited

Loading