Simplify MainNodeSdkResolverService #7244

jeffkl · 2022-01-10T18:08:44Z

Context

The MainNodeSdkResolverService process incoming requests from out-of-proc nodes to handle SDK resolution. The main node has a queue of all incoming packets that are processed by appropriate handlers. The MainNodeSdkResolverService currently takes SdkResolverRequests and places them in a queue for processing. This seems unnecessary to have two queues and two threads draining the queues.

Changes Made

Remove queue and thread that process it in favor of launching a fire-and-forget task that resolves the SDK and sends the response.

Testing

Existing tests cover this area

Notes

Forgind · 2022-01-10T18:27:31Z

I saw your commit is called WIP, but this isn't a draft, and it doesn't have the WIP label. Do you want reviews?

jeffkl · 2022-01-10T19:00:39Z

@Forgind Yeah sorry my original commit was a work in progress and I ran the tests locally with some failures. So I pushed the commit and remembered that some tests just don't pass locally so I opened the PR and forgot my commit name was WIP. Should I update the commit message or will we just update it if/when this is merged?

Forgind · 2022-01-10T22:55:50Z

Not a problem; we can update it when it's merged. I just wasn't sure whether you'd wanted it reviewed 🙂

Forgind

Does this still react properly to shutdown events? I didn't see anything about that. Otherwise, looks great! Love taking out excess complication.

jeffkl · 2022-01-11T20:38:30Z

Does this still react properly to shutdown events? I didn't see anything about that.

Good question, I think the packets will just stop coming in but existing requests will still be running. Let me see if I can simulate a shutdown an see what actually happens. If it doesn't respond well, I'll add cancellation support to the Task.Run().

src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs

jeffkl · 2022-01-18T17:25:24Z

Does this still react properly to shutdown events?

I looked into this and shutdown events aren't really applicable anymore since the tasks that respond are fire-and-forget and there's nothing to shutdown. Cancellation doesn't work, but it never has before since cancellation tokens aren't passed into evaluator.

src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs

ladipro · 2022-01-19T12:54:31Z

src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs

-                            _requestReceivedEvent.Reset();
+                    nodeManager.SendData(request.NodeId, response);
+                }
+            }).ConfigureAwait(continueOnCapturedContext: false);


nit: Does continueOnCapturedContext: false make a difference when the task has no continuation?

The more I look into it, it seems like this method is already running on a thread pool thread. The communication receives packets and runs these with a worker queue. Maybe I don't even need to use Task.Run() at all. Let me look into it.

Latest version doesn't call Task.Run() this this method is already running in a thread pool

Wait, it is running in the thread pool but it doesn't mean that it can be synchronous. Here's the callstack I see with my test build:

Microsoft.Build.dll!Microsoft.Build.BackEnd.SdkResolution.MainNodeSdkResolverService.PacketReceived(int node, Microsoft.Build.BackEnd.INodePacket packet) Line 81 C# Microsoft.Build.dll!Microsoft.Build.BackEnd.NodePacketFactory.PacketFactoryRecord.RoutePacket(int nodeId, Microsoft.Build.BackEnd.INodePacket packet) Line 110 C# Microsoft.Build.dll!Microsoft.Build.BackEnd.NodePacketFactory.PacketFactoryRecord.DeserializeAndRoutePacket(int nodeId, Microsoft.Build.BackEnd.ITranslator translator) Line 102 C# Microsoft.Build.dll!Microsoft.Build.BackEnd.NodePacketFactory.DeserializeAndRoutePacket(int nodeId, Microsoft.Build.BackEnd.NodePacketType packetType, Microsoft.Build.BackEnd.ITranslator translator) Line 58 C# Microsoft.Build.dll!Microsoft.Build.BackEnd.NodeManager.DeserializeAndRoutePacket(int nodeId, Microsoft.Build.BackEnd.NodePacketType packetType, Microsoft.Build.BackEnd.ITranslator translator) Line 261 C# Microsoft.Build.dll!Microsoft.Build.BackEnd.NodeProviderOutOfProcBase.NodeContext.ReadAndRoutePacket(Microsoft.Build.BackEnd.NodePacketType packetType, byte[] packetData, int packetLength) Line 1090 C# > Microsoft.Build.dll!Microsoft.Build.BackEnd.NodeProviderOutOfProcBase.NodeContext.BodyReadComplete(System.IAsyncResult result) Line 1144 C#

The frame at the bottom has:

// Read the next packet. BeginAsyncPacketRead();

as the next statement to execute. Can we really afford to block processing of further packets from the node until the SDK is resolved?

I was looked at it. With current state of code we can afford to block packet processing until SDK is resolved. Nodes are single threaded, and node is blocked and wait for response while it sends request to main node for SDK resolution. So it is guaranteed, if I understand it correctly, that there is actually no other packet to be processed - on that particular namedpipe.

rainersigwald · 2022-01-19T16:06:30Z

Also, how do you assess risk here? Would it be possible to preserve the old behavior under a changewave Just In Case™️?

jeffkl · 2022-02-01T17:00:14Z

Also, how do you assess risk here? Would it be possible to preserve the old behavior under a changewave Just In Case™️?

I don't think there's much risk here. From what I can tell, fundamental flaws in this code would almost immediately show up for any build that's using out-of-proc nodes. There could be a performance impact but I expect things to be faster since the new code should be launching less threads. I think we're early enough in the release cycle to catch any big problems. I do want to try an experimental insertion and install the build just to test out the perf. My local builds aren't NGen'd and are always slower...

jeffkl · 2022-02-02T20:05:53Z

@rainersigwald can I get an experimental insertion to install and try out please?

Forgind · 2022-02-02T21:51:30Z

(Resolved offline)

jeffkl · 2022-02-03T04:56:06Z

Results are in, everything works! I cloned https://github.com/msbuild/msbuildsdks which uses a lot of MSBuild project SDKs to test

Current version (17.0)

D:\msbuildsdks>msbuild /version
Microsoft (R) Build Engine version 17.0.0+c9eb9dd64 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

17.0.0.52104

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:08.96

My Version

D:\msbuildsdks>msbuild /version
Microsoft (R) Build Engine version 17.2.0-preview-22102-01+ab20718e2 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

17.2.0.10201

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:05.59

So my change doesn't make anything slower. I'd imagine I'm also benefiting from other unshipped performance gains so I won't take any credit there. But nothing is broken!

jeffkl · 2022-02-03T05:00:27Z

Also forgot to mention, builds in Visual Studio work just fine as well!

Forgind · 2022-02-03T15:41:10Z

I'd imagine I'm also benefiting from other unshipped performance gains so I won't take any credit there.

Very optional, but you can try downloading a recent main build (ideally from right before your change, but eh) to see if it changes anything, I wouldn't be surprised if that was entirely your change!

rokonec

Good job. If I understand code correctly, this changes shall not cause regression in MSBuild command line builds. In VS I recommend to measure and compare at some high CPU cores count machine and large projects.

src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs

jeffkl · 2022-02-18T17:41:02Z

@rainersigwald anything we're waiting on for this PR?

rainersigwald

I'm still a bit worried about not having a way to opt back into the old behavior, but if you're super confident that this is better in all cases let's get it in for preview 2.

jeffkl · 2022-02-18T17:55:20Z

I'm still a bit worried about not having a way to opt back into the old behavior, but if you're super confident that this is better in all cases let's get it in for preview 2.

I did a lot of testing and couldn't find any cases where it was causing issues. I loaded a large solution in Visual Studio, did a large command-line build, I added delays to a test SDK resolver, and DDRITs passed. The sooner this goes in the better for sure, I'm really hoping we don't need an escape hatch since this code path is used so much, we should find if there are any problems with it very soon.

jeffkl mentioned this pull request Jan 10, 2022

[Epic]: NuGet-based MSBuild project SDK resolver improvements NuGet/Home#11441

Open

18 tasks

Forgind approved these changes Jan 11, 2022

View reviewed changes

rainersigwald reviewed Jan 11, 2022

View reviewed changes

src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs Outdated Show resolved Hide resolved

ladipro reviewed Jan 19, 2022

View reviewed changes

jeffkl force-pushed the simplify-mainnodesdkresolverservice branch 2 times, most recently from ff1aab1 to ab20718 Compare February 2, 2022 16:43

rokonec approved these changes Feb 8, 2022

View reviewed changes

src/Build/BackEnd/Components/SdkResolution/MainNodeSdkResolverService.cs Outdated Show resolved Hide resolved

jeffkl added 3 commits February 14, 2022 10:53

Simplify MainNodeSdkResolverService

3c224ae

Stop using Task.Run and don't name the thread

1c9115b

Address comment

7ba2fe9

jeffkl force-pushed the simplify-mainnodesdkresolverservice branch from 0431f45 to 7ba2fe9 Compare February 14, 2022 18:53

rainersigwald approved these changes Feb 18, 2022

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 28, 2022

AR-May merged commit 2e82c9f into dotnet:main Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify MainNodeSdkResolverService #7244

Simplify MainNodeSdkResolverService #7244

jeffkl commented Jan 10, 2022

Forgind commented Jan 10, 2022

jeffkl commented Jan 10, 2022

Forgind commented Jan 10, 2022

Forgind left a comment

jeffkl commented Jan 11, 2022

jeffkl commented Jan 18, 2022

ladipro Jan 19, 2022

jeffkl Feb 1, 2022

jeffkl Feb 2, 2022

ladipro Feb 7, 2022

rokonec Feb 12, 2022

rainersigwald commented Jan 19, 2022

jeffkl commented Feb 1, 2022

jeffkl commented Feb 2, 2022

Forgind commented Feb 2, 2022

jeffkl commented Feb 3, 2022

jeffkl commented Feb 3, 2022

Forgind commented Feb 3, 2022

rokonec left a comment •

edited

Loading

jeffkl commented Feb 18, 2022

rainersigwald left a comment

jeffkl commented Feb 18, 2022

Simplify MainNodeSdkResolverService #7244

Simplify MainNodeSdkResolverService #7244

Conversation

jeffkl commented Jan 10, 2022

Context

Changes Made

Testing

Notes

Forgind commented Jan 10, 2022

jeffkl commented Jan 10, 2022

Forgind commented Jan 10, 2022

Forgind left a comment

Choose a reason for hiding this comment

jeffkl commented Jan 11, 2022

jeffkl commented Jan 18, 2022

ladipro Jan 19, 2022

Choose a reason for hiding this comment

jeffkl Feb 1, 2022

Choose a reason for hiding this comment

jeffkl Feb 2, 2022

Choose a reason for hiding this comment

ladipro Feb 7, 2022

Choose a reason for hiding this comment

rokonec Feb 12, 2022

Choose a reason for hiding this comment

rainersigwald commented Jan 19, 2022

jeffkl commented Feb 1, 2022

jeffkl commented Feb 2, 2022

Forgind commented Feb 2, 2022

jeffkl commented Feb 3, 2022

Current version (17.0)

My Version

jeffkl commented Feb 3, 2022

Forgind commented Feb 3, 2022

rokonec left a comment • edited Loading

Choose a reason for hiding this comment

jeffkl commented Feb 18, 2022

rainersigwald left a comment

Choose a reason for hiding this comment

jeffkl commented Feb 18, 2022

rokonec left a comment •

edited

Loading