dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.35k stars 9.99k forks source link

Could we use `Parallel` to improve the `Walk` performance in Kestrel `ConnectionManager`? #27931

Closed WeihanLi closed 3 years ago

WeihanLi commented 3 years ago

Just a mind, not for sure, need double check from the team.

There's the Walk method source in ConnectionManager, you can get details from https://github.com/dotnet/aspnetcore/blob/v5.0.0/src/Servers/Kestrel/Core/src/Internal/Infrastructure/ConnectionManager.cs#L55

public void Walk(Action<KestrelConnection> callback)
{
    foreach (var kvp in _connectionReferences)
    {
        var reference = kvp.Value;

        if (reference.TryGetConnection(out var connection))
        {
            callback(connection);
        }
        else if (_connectionReferences.TryRemove(kvp.Key, out reference))
        {
            // It's safe to modify the ConcurrentDictionary in the foreach.
            // The connection reference has become unrooted because the application never completed.
            _trace.ApplicationNeverCompleted(reference.ConnectionId);
            reference.StopTrasnsportTracking();
        }

        // If both conditions are false, the connection was removed during the heartbeat.
    }
}

Since the _connectionReferences is a ConcurrentDictionary, so could we use Parallel instead foreach loop sequentially?

BrennanConroy commented 3 years ago

We don't think there would be any gain from parallelizing this. Thanks for your interest though!

WeihanLi commented 3 years ago

I think if we use parallel would improve the heart beat efficiency, may reduce the occurrence of HeartBeat too slow warning(sometimes occur in our application). Could you please have a double check on this? Thanks @BrennanConroy

davidfowl commented 3 years ago

may reduce the occurrence of HeartBeat too slow warning(sometimes occur in our application)

That likely happens for other reasons (like blocking threads in the application).

WeihanLi commented 3 years ago

Thanks for the tip @davidfowl

WeihanLi commented 3 years ago

I got the HeartBeat too slow warning again and I can not see any thread startvation, the data is from the thread pool etw event data collected by prometheus-net.DotNetRuntime

The metrics data:

The warning log:

image

There's a web API project running on asp.net core 3.1

davidfowl commented 3 years ago

Look at the thread pool counters. The thread count and thread pool queue count

WeihanLi commented 3 years ago

Could we add the thread info in the log so that we could find more helpful information easily, just like StackExchange.Redis does