Change new QueueUserWorkItem method to use TState

[stephentoub]

In .NET Core 2.1 we've added this new overload:

public static bool QueueUserWorkItem(WaitCallback callBack, object state, bool preferLocal)

We should change it to instead be:

public static bool QueueUserWorkItem<TState>(Action<TState> callBack, TState state, bool preferLocal)

This has several benefits:

1. Action<object> with object state can be used, without a mismatch of delegate signatures. Action<object> is now commonly used in these situations.
2. We can now avoid boxing value type arguments passed in.
3. And because of (2) and value tuples, we can easily pass any number of arguments in without allocating a separate object to store them all.

We should fix this before we ship 2.1.

cc: @benaadams, @davidfowl, @kouvel

[benaadams]

So very much for this!

Action and Action<T> is everywhere and interloping with WaitCallback needs a wrapper lambda which means you loose the state field QueueUserWorkItem(lamda, Action)

readonly static WaitCallback lamda = state => ((Action)state)();

Which also means for Action<T> you have to allocate an additional closure to capture the state in a regular Action

[davidfowl]

YESSSS 😄

[jkotas]

We can now avoid boxing value type arguments passed in.

But the implementation is still going to allocate the workitem on the heap, right? Isn't it just shifting where the boxing happens?

[stephentoub]

Isn't it just shifting where the boxing happens?

I'm not following. The ThreadPool allocates an IThreadPoolWorkItem implementation to store into the queue. That IThreadPoolWorkItem contains the delegate, the object state, and additional state like ExecutionContext. That's true whether or not the object state is a value type that was boxed. The boxing is yet another allocation on top of that work item object.

If you mean the value type is still going to end up on the heap, then yes. But it's going to end up there as part of the one object rather than having a separate one allocated for it.

[jkotas]

Right, that's what I meant. So it saves ~3 pointers worth of allocation per work item. This saving does not depend on the state size. we can easily pass any number of arguments in without allocating an object to store them all made it sound like you can somehow pass in arbitrarily large state for free. (And the price you pay for this is larger code.)

[stephentoub]

made it sound like you can somehow pass in arbitrarily large state for free

That's not what I intended to suggest, and in general I'd expect the number to be small, e.g. 2, for example another state-accepting delegate with a different type and the associated state to go with it.

[omariom]

@jkotas may not like it but what if ThreadPoolWorkItem<TState>? :)

[davidfowl]

It'll eventually get boxed when the in ExecutionContext.Run happens.

[jkotas]

ThreadPoolWorkItem<TState>

I expect that the implementation of the proposed API is going to use type like this internally. Potentially two types like this if it does the optimization for default execution context like the current QueueUserWorkItem.

[stephentoub]

I expect that the implementation of the proposed API is going to use type like this internally. Potentially two types like this if it does the optimization for default execution context like the current QueueUserWorkItem.

Yup.

[davidfowl]

It'll eventually get boxed when the in ExecutionContext.Run happens.

Actually, I'm wrong here, I forget we pass the work item itself as the state!

[stephentoub]

Fixed.