Schedule proxy builds to inproc node #6386
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeffkl
approved these changes
Apr 29, 2021
Contributor
jeffkl
left a comment
jeffkl
left a comment
There was a problem hiding this comment.
I saw the perf improvement first hand, this is a great reduction in scheduling time.
cdmihai
force-pushed
the
scheduleProxyBuildsToInprocNode
branch
from
854c725 to
e792ce5
Compare
Contributor
Author
|
Added a warning when proxy builds do not get scheduled on the inproc node. This avoids silent perf regressions. |
cdmihai
added
the
merge-when-branch-open
cdmihai
removed
the
merge-when-branch-open
cdmihai
force-pushed
the
scheduleProxyBuildsToInprocNode
branch
from
e792ce5 to
b8ef569
Compare
cdmihai
force-pushed
the
scheduleProxyBuildsToInprocNode
branch
from
b8ef569 to
9c6fbaa
Compare
Contributor
Author
|
Rebased on vs16.11 and removed the dependency on #6400 in order to eliminate any risk. |
cdmihai
force-pushed
the
scheduleProxyBuildsToInprocNode
branch
from
9c6fbaa to
760b761
Compare
cdmihai
added
the
merge-when-branch-open
Forgind
reviewed
May 25, 2021
Co-authored-by: Forgind <[email protected]>
Forgind
reviewed
May 25, 2021
rainersigwald
approved these changes
May 26, 2021
Co-authored-by: Rainer Sigwald <[email protected]>
Forgind
pushed a commit
that referenced
this pull request
Jun 4, 2021
Context There are two ways in MSBuild to disable the inproc node: via the environment variable MSBuildNoInprocNode by setting BuildParameters.DisableInprocNode The implementations of these two, as you'd expect from MSBuild, are totally separate, they have nothing in common. The env var informs the Scheduler to assign all requests to out of proc nodes, regardless of their affinities. And the build parameter informs the NodeManager to just bail out on inproc node creation and return null. This means that if you set the env var and build a traversal project (dirs.proj, or a solution metaproj file), then all is fine, the scheduler silently redirects them to out of proc nodes. But if you set the build parameter instead of the env var and build the traversal dirs.proj then MSBuild fails with: MSBUILD : error MSB4223: A node of the required type InProc could not be created. This is causing #6386 to fail some unit tests which ensure that the project cache plugin can function with the inproc node disabled: https://dev.azure.com/dnceng/public/_build/results?buildId=1114344&view=ms.vss-test-web.build-test-results-tab&runId=33953534&resultId=101506&paneView=debug This is a bit inconsistent and I just don't see the reason for having two separate things. So I made BuildParamters.DisableInProcNode also trigger the Scheduler's ForceAffinityOutOfProc. I doubt it would break anybody, and would actually benefit the users that want to both disable the inproc node via the API (like VS does in certain cases) and avoid node creation exceptions. Changes Made The scheduler now forces affinity to out of proc when either the env var is set, or the build parameter is set. It will avoid build failures when the build parameter is set. Testing Added / updated unit tests.
rainersigwald
pushed a commit
that referenced
this pull request
Jul 1, 2021
…y built on oop nodes (#6635) Fixes a bug in proxy build scheduling introduced by #6386. If a the BuildRequestConfiguration associated with a proxy request has been built before on an out of proc (oop) node then the scheduler will fail with either one of: - affinity mismatch error. This happens when the proxy build is assigned to the inproc (inp) node but its configuration is already assigned to an oop node `AND` serving other existing requests, either blocked or running. - unscheduled requests remain even if there's free oop nodes that can serve them. This happens (as far as I can tell) when the proxy's configuration is already assigned to an oop node (because a previously built non proxy request was assigned there) `AND` there's no other existing requests for that configuration The fix in this PR is to not assign a proxy build to the inproc node if its configuration was previously assigned to another node.
rokonec
pushed a commit
that referenced
this pull request
Aug 5, 2021
* Fix missing project instance in project cache requests (#6568) Context Non static graph builds using the project cache didn't set the ProjectInstance on the cache request, leading to crashes in the cache. Changes Made Recreate the BuildRequestData for the cache request after the cache service evaluates projects. I was initially using the original BuildSubmission.BuildRequestData which does not contain the project instance. * Don't launch debugger window for all tests * Convert static InitializePlugin into non-static BeginBuildAsync To allow asserting service state transition * Assert state transitions in ProjectCacheService * Only initialize once for the VS workaround * Bravely set DoNotLaunchDebugger only once for all tests * Simplify branching * [vs16.11] Update dependencies from dotnet/arcade (#6625) * Update dependencies from https://github.com/dotnet/arcade build 20210628.3 Microsoft.DotNet.Arcade.Sdk From Version 5.0.0-beta.21315.2 -> To Version 5.0.0-beta.21328.3 * Don't schedule proxy builds to inproc node if their configs previously built on oop nodes (#6635) Fixes a bug in proxy build scheduling introduced by #6386. If a the BuildRequestConfiguration associated with a proxy request has been built before on an out of proc (oop) node then the scheduler will fail with either one of: - affinity mismatch error. This happens when the proxy build is assigned to the inproc (inp) node but its configuration is already assigned to an oop node `AND` serving other existing requests, either blocked or running. - unscheduled requests remain even if there's free oop nodes that can serve them. This happens (as far as I can tell) when the proxy's configuration is already assigned to an oop node (because a previously built non proxy request was assigned there) `AND` there's no other existing requests for that configuration The fix in this PR is to not assign a proxy build to the inproc node if its configuration was previously assigned to another node. * Localized file check-in by OneLocBuild Task (#6644) * 16.11 Final Branding (#6656) Co-authored-by: Rainer Sigwald <[email protected]> Co-authored-by: Mihai Codoban <[email protected]> Co-authored-by: AR-May <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet bot <[email protected]> Co-authored-by: Ben Villalobos <[email protected]> Co-authored-by: Rainer Sigwald <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Proxy builds are what project caches issue on cache hits. They are a cheap version of the expensive targets that were avoided by the cache. They need to produce the same properties and items the expensive target produced, but with none of the CPU / IO expensive stuff.
The proxy builds are super cheap because they only return properties / items. It is not worth scheduling them to out of proc nodes because:
Scheduling proxy builds to the inproc node makes a project cache build with full cache hits 16% faster.
Changes Made
Duplicated what the scheduler does to confine traversal projects to the inproc node, since those are also cheap.
Testing
Added a test