Skip to content
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

QUIC: Reuse QuicStream instances on a connection #49972

Open
JamesNK opened this issue Mar 22, 2021 · 7 comments
Open

QUIC: Reuse QuicStream instances on a connection #49972

JamesNK opened this issue Mar 22, 2021 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Quic tenet-performance Performance related issue
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Mar 22, 2021

In Kestrel we want to reduce per-request HTTP/3 allocations to the minimum possible. Ideally that would be zero.

With HTTP/1.1 and HTTP/2 Kestrel is able to reuse per-request instances. For example, after a HTTP/2 request is gracefully completed (i.e. wasn't aborted and finished writing a frame with the close stream bit set) the stream is returned to a pool on the connection. Future requests on the connection will reuse the pooled stream by reseting it back to its initial state instead of creating a new one. This is discussed in a blog post.

At the moment a new QuicStream is created for each HTTP/3 request on a connection:

public class QuicConnection
{
    // Existing
    QuicStream OpenUnidirectionalStream();
    QuicStream OpenBidirectionalStream();
    ValueTask<QuicStream> AcceptStreamAsync();

    // New?
    void ReturnCompletedStream(QuicStream stream);
}

Support should be added to System.Net.Quic for optionally returning a QuicStream back to the connection when an app is no longer using it. The instance can then be reused by one of the factory methods above.

Extra things to consider:

  • Validate QuicStream is in a valid state before pooling it.
  • Maximum number of pooled streams. In Kestrel there is a max of 100 HTTP/2 pooled streams allowed on a connection. I don't think this needs to be configurable.
  • Remove unused streams after a certain amount of time. In Kestrel the HTTP/2 pooled streams on a connection are removed if not used within a certain amount of time. In place so idle connections don't unnessarily hold onto memory. I don't think this needs to be configurable.
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 22, 2021
@ghost
Copy link

ghost commented Mar 22, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

In Kestrel we want to reduce per-request HTTP/3 connections to the minimum possible. Ideally that would be zero.

With HTTP/1.1 and HTTP/2 Kestrel is able to reuse per-request instances. For example, after a HTTP/2 request is gracefully completed (i.e. wasn't aborted and finished writing a frame with the close stream bit set) the stream is returned to a pool on the connection. Future requests on the connection will reuse the pooled stream by reseting it back to its initial state instead of creating a new one. This is discussed in a blog post.

At the moment a new QuicStream is created for each HTTP/3 request on a connection:

public class QuicConnection
{
    // Existing
    QuicStream OpenUnidirectionalStream();
    QuicStream OpenBidirectionalStream();
    ValueTask<QuicStream> AcceptStreamAsync();

    // New?
    void ReturnCompletedStream(QuicStream stream);
}

Support should be added to System.Net.Quic for optionally returning a QuicStream back to the connection when an app is no longer using it. The instance can then be reused by one of the factory methods above.

Author: JamesNK
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@scalablecory scalablecory added this to the Future milestone Mar 25, 2021
@scalablecory scalablecory added api-suggestion Early API idea and discussion, it is NOT ready for implementation tenet-performance Performance related issue and removed untriaged New issue has not been triaged by the area owner labels Mar 25, 2021
@karelz
Copy link
Member

karelz commented Sep 21, 2021

Triage: Leave it in Future, if it pops in perf analysis, we can move it to 7.0.

@wegylexy
Copy link
Contributor

Unlike a TCP stream, which is the connection, QUIC multiplexes streams over a single connection.
Correct me if I am wrong. In HTTP/2, requests are multiplexed with frames on the same stream. In HTTP/3, each request has its own lightweight stream exclusive for that particular request. Resetting a QUIC stream aborts it. This is how HTTP/3 works over QUIC.
Nevertheless, it may be possible to reuse the .NET wrapper on a future instance of native MsQuic stream, which is super lightweight. See also dotnet/aspnetcore#33680.

@ManickaP
Copy link
Member

it may be possible to reuse the .NET wrapper on a future instance of native MsQuic stream

That's how I understood the issue. I think it's highly unlikely msquic would support re-using their stream object, but maybe I'm wrong, @nibanks?

@nibanks
Copy link

nibanks commented Apr 22, 2022

MsQuic stream objects are highly optimized for creation and deletion (most of the time it's just pushing and popping from a pool list). So don't really need/want to support reuse. Now, if that's not easy to do in C#, perhaps you could support reuse by just deleting the old one and opening a new one.

@ManickaP
Copy link
Member

Yeah, I thought so, and since native memory doesn't influence GC, recreating the msquic object wouldn't have any negative impact on pooling our managed wrapper.

@wfurt
Copy link
Member

wfurt commented Apr 22, 2022

We could add API that allows to assign new stream to existing QuicStream instead of always allocations new one.
like stream.Close(); stream = QuicConnection.AcceptAsync(stream, token);
We can certainly use pool allocation behind the scenes but that has also some cost and does not give choices to the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Quic tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

7 participants