-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Performance] Regression on Fortunes - Windows #101936
Comments
Is anything in Fortunes using Channel.CreateUnbounded to create a channel that's used on a per-request code path? |
Npgsql seems to use it, @roji ? |
Yep, an unbounded channel is the internal implementation of Npgsql's connection pool (Max Pool Size is enforced in other ways, so the channel itself is unbounded). Is there any particular concern here? Was this a regression in the Channel implementation? /cc @NinoFloris @vonzshik |
Not for you 😄
That's what I'm questioning. One of the changes in the diff range was a change that shouldn't have impacted throughput of the channel created by |
@roji, which of these /where would end up being used on a hot path in Fortunes? |
Tagging subscribers to this area: @dotnet/area-system-threading-channels |
@stephentoub that's probably going to be the one in the connector. The way it works is that each connector (that's a physical connection to pg) starts their own async loop to handle query responses. Then, whenever data source receives queries, it dispatches them to a connector with the least in-flight query count and after each dispatch it attempts to write that query to connector's socket. If that write does complete synchronously, it's just going to immediately dispatch another query to the exact same connector. That loop repeats until write doesn't complete synchronously. As for the code, here queries are received from data source. Not sure whether it's relevant, but data source write loop is single threaded, so there shouldn't be any contentions. |
The only other one we have is the one we use for the pooling (just as @roji mentioned above). It potentially could be quite hot since the way fortunes designed to:
Now, there is a catch. This particular path shouldn't ever be executed with fortunes. The reason for this is because we use multiplexing mode with TE benchmarks (as it's the most performing one), and it completely avoids connector renting, instead going through multiplexing data source (which I described above). @roji is there a chance this benchmark doesn't use multiplexing? |
Sorry for taking so long to answer here. It seems like this (non-platform) benchmark also uses multiplexing, so there indeed shouldn't be a path where the unbounded channel is used. The other case of channel-usage has already been discussed above - it's not unbounded in any case... |
@sebastienros, does the regression persist? Based on all of this, I don't see how the Channel PR could have impacted this, but it's also seemingly the only commit in that diff range that could possibly be related. |
@stephentoub it's still there. To confirm, if it doesn't take much to build an assembly with and without the commit then that would help me to confirm if we are looking at the correct change. |
Isolated to these commits: 774bc93...d688ac2
Only on Windows
320K RPS to 300K RPS.
There is also a visible first request regression on the same commits.
Command line
Between
9.0.0-preview.4.24223.5
and9.0.0-preview.4.24223.6
The text was updated successfully, but these errors were encountered: