[wip] TaskHost IBuildEngine Callback Support spec for discussion #12868
Conversation
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
looks pretty good.
Should we add any ETW events as part of this? What about telemetry?
| ## 1. Problem Statement | ||
|
|
||
| The MSBuild TaskHost (`OutOfProcTaskHostNode`) implements `IBuildEngine10` but lacks support for several callbacks. TaskHost is used when: | ||
| 1. **`MSBUILDFORCEALLTASKSOUTOFPROC=1`** with `-mt` mode - forces non-thread-safe tasks out-of-proc |
There was a problem hiding this comment.
Aren't these separate cases?
| 1. **`MSBUILDFORCEALLTASKSOUTOFPROC=1`** with `-mt` mode - forces non-thread-safe tasks out-of-proc | |
| 1. **`MSBUILDFORCEALLTASKSOUTOFPROC=1`** | |
| 2. `-mt` mode - forces non-thread-safe tasks out-of-proc |
| - A) Forward to parent and actually yield (allows scheduler to run other work) | ||
| - B) Keep as no-op (current behavior, safest) | ||
|
|
||
| **Recommendation:** (B) initially - Yield/Reacquire are rarely used by tasks, and current no-op behavior has shipped. Revisit if real-world need arises. |
There was a problem hiding this comment.
I agree, though I think the most expensive tasks do tend to yield. Definitely something that can wait.
|
|
||
| | Phase | Scope | Risk | Effort | | ||
| |-------|-------|------|--------| | ||
| | 1 | `IsRunningMultipleNodes` | Low | 2 days | |
There was a problem hiding this comment.
Still unsure if this is worth forwarding the request along vs hardcoding "yeah if you're in this mode assume parallelism".
There was a problem hiding this comment.
Is -mt /m:1 (or just /mt for msbuild.exe) the only way to hit this condition? What is the practical impact on the Tasks we have that use IsRunningMultipleNodes? Do we know if any 1P Tasks care about this condition?
There was a problem hiding this comment.
it's /m:1 + a taskhost for any reason (mt + unenlightened task, env var that forces everything to a task host, explicit taskhost request in usingtask).
I can't imagine that it's widely used. I see one use in our repo in the task version of the MSBuild task that changes how StopOnFirstFailure works, which is itself a silly thing to do . . .
| | 1 | `IsRunningMultipleNodes` | Low | 2 days | | ||
| | 2 | `RequestCores`/`ReleaseCores` | Medium | 3 days | | ||
| | 3 | `Yield`/`Reacquire` | Medium | 3 days | | ||
| | 4 | `BuildProjectFile*` | High | 5-7 days | |
There was a problem hiding this comment.
BuildProjectFile is the most critical one, for XAML, right?
There was a problem hiding this comment.
My current take is that this is a niche compat layer we should invest minimum amount possible into.
-
IsRunningMultipleNodes, RequestCores/ReleaseCores may be used by tasks for some decisions that may change perf characteristics of such tasks, if the customers care about that case they should move to make the task multithreadable. just ignore
-
Yield,Reacquire this is a very "Worker Node" concept that is weird with taskhosts, we would need to host multiple taskhosts inside of 1 taskhost process and figure out how to communicate with which, and for what benefit? I think we should just ignore it forever. Same as before, if customers really want Yielding they can migrate.
-
if we don't implement the multiple taskhosts inside of 1 taskhost process from ^ I think BuildProjectFile needs to be implemented in a way that ignores max node count and just spawns an inproc node and waits until that finishes. Again if customers don't like this they should enlighten.
-
or the inproc node might "Yield" and that means it'd need to connect to a fresh taskhost where it could push non-mt-compatible tasks
There was a problem hiding this comment.
@rainersigwald does not this callback require (when called in the working node) to free the worker node for other projects to build, similar to yielding scenarios? Otherwise, I guess, we might get into the situation when we exhausted all msbuild nodes - all nodes would be blocked waiting for the resolution of the callback. So, the same might happen with task host if we implement just a blocking behavior - and much quicker because the thread nodes tied to the specific task host. I guess for proper implementation we will need to implement freeing the task host for more tasks to come to it from another projects, and a way to save and recover the state to be able to continue the execution. This sounds quite like a challenge.
There was a problem hiding this comment.
I am concerned about the short-term impact of not supporting Yield/Reacquire: they make running tool processes fairly efficient by yielding the node for other MSBuild work while running the tool "in the background".
With BuildProjectFiles: we must free the MSBuild-layer part of the node to do additional work, including running other tasks--if we don't, we can hit the exhaustion case (consider an -m:1 build with one xaml project: do some stuff on the entry project, build the "inner" one including running tasks, come back to finish).
In normal execution we would handle that by leaving a (System.Threading.)Task dangling and yielding (in the System.Threading sense) to do the other work in-proc (so you'd have both the task that called BuildProjectFile and the tasks launched in the called project running concurrently).
I think we have to support that to have a good hope of overall MT success here, since lots of projects use XAML tasks that ship in .NET Framework and can't be updated to the new model.
4 participants
#12863
Changes Made
spec only