-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix issue where compiler host dropped connections under load #46510
Conversation
Got the API in place to make Begin / End listening a part of the contract. This will mean the multiple servers under the hood can be added much simpler. Simplified the dispatching code significantly
@dotnet/roslyn-compiler PTAL |
@dotnet/roslyn-compiler PTAL |
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
Done review pass (commit 8). Minor comments only. |
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Server/VBCSCompilerTests/NamedPipeClientConnectionHostTests.cs
Outdated
Show resolved
Hide resolved
Thanks for the feedback. Updated. |
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
lock (servers) | ||
{ | ||
var e = servers.GetEnumerator(); | ||
while (e.MoveNext()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use foreach
instead? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we can't unfortunately. This is a non-generic enumeration and it's using custom properties on the enumerator to get the values. #Closed
The mac leg passed ... this .. this might happen. .... |
Under load the compiler server was failing to accept client connections faster than the client connect timeout period (one second). This lead clients to abandon the server and shell out to csc / vbc directly. This is a net negative for customers, even when the compiler server is under extreme load, because the new csc / vbc processes and the compiler server will essentially be competing for the same CPU resources. Part of the benefit of the compiler server is to avoid this competition.
The most immediate cause of this problem is that the server only creates a single
NamedPipeServerStream
for accepting connections and there was a significant amount of processing that had to occur between accepting one connection and creating a newNamedPipeServerStream
for accepting new connections. Particularly problematic is that the work required several thread transitions. Under normal processing that was fast enough but under load that was not.The solution here is essentially two fold:
NamedPipeServerStream
instances in parallel to ensure that we can accept connections even when a single thread gets stalled under load.This change also tightens the tolerance level we have in our dogfood builds for failed connections to zero. This should help us catch regressions in this area in the future.
Note: This change is best read commit by commit.