CreateLinkedTokenSource is high alloc

[benaadams]

Using CreateLinkedTokenSource allocates more memory than the HttpContext! O_o

_requestAbortCts = CancellationTokenSource.CreateLinkedTokenSource(_disconnectCts.Token);

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs#L251

/cc @davidfowl @halter73

[khellang]

40 + 24 + 28 = 92 bytes per request, I think? We had a bug with this in Nancy, where it wasn't disposed properly. Turns out they're not collected otherwise 😱 Didn't work out well for memory usage 😭

[benaadams]

They are being disposed ok; just bad for GC

[benaadams]

Also the function takes a params[] so calling it allocates an array :-/

[NickCraver]

I don't see a clean way around this. There are almost enough bits exposed where this could be hoisted, but not quite:

private static readonly Action<object> _disconnectCtsAction = LinkedTokenCancelDelegate;
private static void LinkedTokenCancelDelegate(object source)
{
    CancellationTokenSource cts = source as CancellationTokenSource;
    Contract.Assert(source != null);
    cts.Cancel();
}
...
_disconnectCts.Token.Register(_disconnectCtsAction, _requestAbortCts);

The problem is there's no public API for excluding the execution context capture. If we had this exposed, it's duplicative of what CreateLinkedTokenSource is doing. On the other hand, we could eliminate several allocations here: the params and the internal m_linkingRegistrations registrations go away (since we have exactly one token and we can manually dispose the child).

I'm not sure if pursuing a new API on CancellationToken classes would go anywhere and it doesn't help the older platforms anyway...but should we? I can see this being a common pattern and there's a lot of overhead when we're talking about 2 tokens we have references to and can manually, directly dispose of.

[benaadams]

@NickCraver discussing over here https://github.com/dotnet/coreclr/issues/2113

[NickCraver]

Ah excellent - thanks @benaadams!

Unrelated but noticed when looking at allocations here: what is that _nullBuffer for?

[benaadams]

_nullBuffer is gone now, it was something to burn reads into in case the request hadn't been fully consumed. Now it fast forwards rather than actually reading.

[NickCraver]

@benaadams I could just be dense here, but where's the removal? I looked through current PRs and can't find any touching it - still seeing it in the dev branch.

[benaadams]

@NickCraver #408 though it still allocates a CancellationTokenSource per request; just doesn't do the CreateLinkedTokenSource so its down quite a bit but not gone. Still a lot of stuff going on there:

[NickCraver]

Hmm, I think we're talking about different things here (or at least, I see nothing related in #408) - I'm referring to line 35: https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs#L35

private readonly byte[] _nullBuffer = new byte[4096];

[benaadams]

Eeep! Good catch! It was meant to go as part of #335 will submit a PR

[benaadams]

Is now in #409

[NickCraver]

Okay awesome, thanks! I was in NYC this week but fly home from tomorrow; can't wait to test the latest over the weekend and hunt down remaining bottlenecks.