Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce cross-process resource management for tasks #5859

Merged
merged 118 commits into from Mar 30, 2021
Merged

Conversation

@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Nov 5, 2020

Add RequestCores and ReleaseCores APIs to IBuildEngine8.

These APIs can advise a task that wishes to do work with its own internal parallelism. The task can request as many (abstracted) CPU cores as it desires, and the MSBuild engine will keep track of how many have been requested and prevent the machine from being completely overloaded.

Since this is advisory only, existing tasks will be unaffected. The Visual C++ tasks plan to opt into this.

See resource-management.md for more design and implementation details.

Fixes #74

BenVillalobos and others added 30 commits Feb 25, 2020
Got straightforward unit tests working.
Added null check and cached the Semaphore.
This reverts commit f60c66a.
This should alleviate hangs that were happening as a result of leaks
that happened when MSBuild/CallTarget got a resource, then
we started building other projects.
Otherwise, we can deadlock: outer task gets a resource, (logically) yields to build other projects; they block on getting a resource.
@ladipro ladipro marked this pull request as ready for review Mar 26, 2021
Copy link
Member

@Forgind Forgind left a comment

Partial review

}
else
{
// Resource release is a one-way call, no response is expected. We release the cores as instructed

This comment has been minimized.

@Forgind

Forgind Mar 26, 2021
Member

There could be an issue if the ReleaseCores call for a process that uses a lot of cores gets lost somehow. I'm wondering if, in addition to being able to release cores, there should be an auto-release after a certain amount of time. Perhaps the scheduler pings the task, and if it responds that it still needs cores, it resets it; otherwise, it reclaims them?

This comment has been minimized.

@ladipro

ladipro Mar 29, 2021
Contributor

There are countless ways a task can hang the build. Yes, the new API needs to be used carefully. With great power comes great responsibility, as they say. What you are suggesting sounds too complicated and it would not prevent all hangs. I can imagine tasks would use a hard-coded "yes" when asked if they still need cores and pass int.MaxValue for timeout.

Copy link
Member

@Forgind Forgind left a comment

My only reservation is that there aren't a lot of features to prevent users from shooting themselves in the foot, especially in unusual circumstances with highly oversubscribed machines and/or low-priority builds, but I don't think that should hold you back. Thanks for picking this up and driving it through!

src/Build/BackEnd/Components/Scheduler/Scheduler.cs Outdated Show resolved Hide resolved
/// <summary>
/// Retrieves the <see cref="IBuildEngine9" /> version of the build engine interface provided by the host.
/// </summary>
public IBuildEngine9 BuildEngine9 => (IBuildEngine9)BuildEngine;

This comment has been minimized.

@Forgind

Forgind Mar 29, 2021
Member

As I mentioned in the meeting, I slightly prefer reusing IBuildEngine8, but this is fine.

@Forgind Forgind merged commit 1629921 into main Mar 30, 2021
8 checks passed
8 checks passed
@dotnet-bot
MSBuild #20210330.1 succeeded
Details
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210330.2 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants