Fix issue where compiler host dropped connections under load#46510
Merged
jaredpar merged 11 commits intodotnet:masterfrom Aug 7, 2020
Merged
Fix issue where compiler host dropped connections under load#46510jaredpar merged 11 commits intodotnet:masterfrom
jaredpar merged 11 commits intodotnet:masterfrom
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
Member
Author
|
@dotnet/roslyn-compiler PTAL |
Member
Author
|
@dotnet/roslyn-compiler PTAL |
cston
reviewed
Aug 5, 2020
333fred
reviewed
Aug 5, 2020
333fred
reviewed
Aug 5, 2020
333fred
reviewed
Aug 5, 2020
333fred
reviewed
Aug 5, 2020
cston
reviewed
Aug 5, 2020
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
cston
reviewed
Aug 5, 2020
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
Member
|
Done review pass (commit 8). Minor comments only. |
cston
reviewed
Aug 5, 2020
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
cston
reviewed
Aug 5, 2020
cston
reviewed
Aug 5, 2020
cston
reviewed
Aug 5, 2020
cston
reviewed
Aug 5, 2020
src/Compilers/Server/VBCSCompilerTests/NamedPipeClientConnectionHostTests.cs
Outdated
Show resolved
Hide resolved
cston
reviewed
Aug 5, 2020
Member
Author
|
Thanks for the feedback. Updated. |
cston
reviewed
Aug 6, 2020
src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs
Outdated
Show resolved
Hide resolved
cston
reviewed
Aug 6, 2020
cston
reviewed
Aug 6, 2020
| lock (servers) | ||
| { | ||
| var e = servers.GetEnumerator(); | ||
| while (e.MoveNext()) |
Contributor
There was a problem hiding this comment.
Could we use foreach instead? #Closed
Member
Author
There was a problem hiding this comment.
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
cston
approved these changes
Aug 6, 2020
333fred
approved these changes
Aug 7, 2020
Member
Author
|
The mac leg passed ... this .. this might happen. .... |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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
NamedPipeServerStreamfor accepting connections and there was a significant amount of processing that had to occur between accepting one connection and creating a newNamedPipeServerStreamfor 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:
NamedPipeServerStreaminstances 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.