[perf] System.Data.SqlClient allocates lots of closures with ContinueWith

[jamesqo]

Ran this grep query:

grep --include='*.cs' --exclude-dir=tests --exclude-dir=ref -r 'ContinueWith[^(]*(([^,]*)' .

It printed out the following results: https://gist.github.com/anonymous/7afb47c97f2f632c7ddf963111ad3136

The closure in WinHttpHandler is fixed by dotnet/corefx#7785, but there appear to be a bunch of them in System.Data.SqlClient that could be removed. Opening up a tracking issue to remove them (which I'll do myself).

---

edit: Just realized I wasn't picking up on some of them, because most are written in the form:

task.ContinueWith(t => ...)

rather than

task.ContinueWith((t) => ...)

so I ran a new query, which seems to pick up more matches. See: https://gist.github.com/anonymous/14aa1dea4fe3a26e42e338c5b30b09b4

[jamesqo]

Related: as justinvp pointed out here, it's also possible to use subclassing in some cases (e.g. TaskCompletionSource<T>) to avoid tuple allocations where there's already another class being allocated. Here's a list of some of the places that could use this optimization: https://gist.github.com/jamesqo/5c24959b7c15a154831901166ae32fbe

Another possible use case is for WinHttpRequestState; maybe instead of having a Tcs property, it could instead subclass TaskCompletionSource to avoid the allocation here.

[jamesqo]

@joshfree Could you add a 'grabbed by community' tag to this issue? The associated PR for this (#7789) was recently closed, but I'm still working on it locally.

[karelz]

@jamesqo are still working on the issue? (I don't think so, so changing the labels ... I'll be happy to revert it back if I am wrong)

[Wraith2]

@karelz I think this can be closed now after dotnet/corefx#34134 was merged everything that I can prevent allocating closures does.

[karelz]

I let @afsanehr @keeratsingh @saurabh500 decide here (sorry I don't have any context).

[Wraith2]

No problem, they're all alerted now so that's fine. 😁