Runtime: API Proposal: Expose TheadPool.UnsafeQueueCustomWorkItem and IThreadPoolWorkItem

[stephentoub]

_From @davidfowl on August 30, 2018 3:3_

When looking at a memory dump, it would be great if we could see data about work items in the various queues. Some of the things that would be interesting to see per work item

• A display name (HttpRequest/TaskContinuation) that can be provided by the caller
• An elapsed time to know how long an item was in the queue

This sort of info might help when looking at dumps trying to diagnose issues with the thread pool.

To enable this and other optimized scheduling scenarios, we should expose an interface that would allow customization of the underlying work item.

Proposal:

```C#
public interface IThreadPoolWorkItem
{
void Execute();
}

public class ThreadPool
{
public static void UnsafeQueueUserWorkItem(IThreadPoolWorkItem workItem, bool preferLocal);
}
```

These are advanced APIs and as such, there's no overload that captures the ExecutionContext (I'm not sure how it could anyways)

cc @stephentoub @kouvel @vancem

_Copied from original issue: dotnet/coreclr#19758_

[stephentoub]

_From @davidfowl on August 31, 2018 18:13_

I had a thought. Instead of exposing these properties what if we just exposed something like IThreadPoolWorkItem? That would make it easier to implement it on user objects that have more context.

[stephentoub]

If this is about dumps, that data could all be stored in the state object that the caller provides, as that would be visible/inspectable in a dump, too, and that can be done with the implementation and API as they already exist.

I don't think we should add a display name to work items; we did that initially with tasks, and it ended up not being particularly useful, and backed it out... plus it almost certainly wouldn't be pay-for-play, at least in terms of memory, as every work item would need to be capable of holding on to a name. Similarly, I'd worry about having to store the queued time onto a work item, as it would both increase the size of the work item and, depending on the desired accuracy, could entail a very measurable perf hit on acquiring the current time.

We've talked in the past about exposing a variant of IThreadPoolWorkItem (we wouldn't want to expose the abort-related method that's now legacy in core), and the corresponding ThreadPool.UnsafeQueueCustomWorkItem method. I'm generally fine with that. It would allow for certain schemes to be implemented more efficiently for someone that really knows what they're doing, and would allow for some additional diagnostics on direct usage, as you're suggesting.

[stephentoub]

_From @davidfowl on September 2, 2018 20:39_

If this is about dumps, that data could all be stored in the state object that the caller provides, as that would be visible/inspectable in a dump, too, and that can be done with the implementation and API as they already exist.

I realize that, closures do work they are just extremely obscure to build tooling on top of.

We've talked in the past about exposing a variant of IThreadPoolWorkItem (we wouldn't want to expose the abort-related method that's now legacy in core), and the corresponding ThreadPool.UnsafeQueueCustomWorkItem method. I'm generally fine with that. It would allow for certain schemes to be implemented more efficiently for someone that really knows what they're doing, and would allow for some additional diagnostics on direct usage, as you're suggesting.

Yep, that's why I kinda changed my suggestion to be exposing something like IThreadPoolWorkItem instead of adding those fields to every work item. That might be the best compromise and would allow callers to provide a custom work item instances.

Part of the thing driving this is to enable building tooling on top of the thread pool queues to allow visibility into the queued work items.

[stephentoub]

_From @davidfowl on September 3, 2018 8:53_

Just thought of another benefit of exposing this API, in Kestrel, we'd basically shave most of the QUWI allocations down since we're just invoking the callback over and over but incur the cost of a QueueUserWorkItem allocation when we don't need to:

https://github.com/aspnet/KestrelHttpServer/blob/8f6cb72ef27d86c9f691ba39851aa958a265cd5e/src/Kestrel.Transport.Sockets/Internal/IOQueue.cs#L13

The IOQueue itself would implement IThreadPoolWorkItem (or whatever variant doesn't have MarkAborted):

```C#
public class IOQueue : PipeScheduler, IThreadPoolWorkItem
{
private readonly object _workSync = new object();
private readonly ConcurrentQueue _workItems = new ConcurrentQueue();
private bool _doingWork;

public override void Schedule(Action<object> action, object state)
{
    var work = new Work
    {
        Callback = action,
        State = state
    };

    _workItems.Enqueue(work);

    lock (_workSync)
    {
        if (!_doingWork)
        {
            System.Threading.ThreadPool.UnsafeQueueCustomWorkItem(this);
            _doingWork = true;
        }
    }
}

public void ExecuteWorkItem()
{
    while (true)
    {
        while (_workItems.TryDequeue(out Work item))
        {
            item.Callback(item.State);
        }

        lock (_workSync)
        {
            if (_workItems.IsEmpty)
            {
                _doingWork = false;
                return;
            }
        }
    }
}

private struct Work
{
    public Action<object> Callback;
    public object State;
}

}
```

Pretty neat.

[stephentoub]

That's what I meant by "It would allow for certain schemes to be implemented more efficiently for someone that really knows what they're doing,"

[stephentoub]

_From @davidfowl on September 3, 2018 14:51_

Yep I was just calling out one of those concrete cases

[stephentoub]

_From @davidfowl on September 3, 2018 15:5_

Should I file a new bug or change this one to be the concrete API proposal?

[stephentoub]

Thanks. Either's fine.

[stephentoub]

Proposal

Generally looks fine. A few notes:

• bool forceGlobal should instead be inverted to bool preferLocal to match the QueueUserWorkItem overload with that argument we already exposed: https://github.com/dotnet/coreclr/blob/0821f6f06f34146053356bf944d01aff086bb312/src/System.Private.CoreLib/src/System/Threading/ThreadPool.cs#L1316
• ExecuteWorkItem should probably just be Invoke (or Execute), as WorkItem already appears in the type name.
• We should consider just making UnsafeQueueCustomWorkItem be an overload of UnsafeQueueUserWorkItem... there'd be the existing overloads that take a delegate/state, and the new overload that takes an interface instance.
• The method should probably return void (that's missing in the proposal). The existing overloads return bool, so we could match that for consistency, but the methods always return true, so the result is meaningless.

there's no overload that captures the ExecutionContext (I'm not sure how it could anyways)

Agreed. If you use this overload, everything is up to you, including propagating context.

[stephentoub]

_From @davidfowl on September 4, 2018 2:0_

Updated the proposal.

[stephentoub]

_From @vancem on September 4, 2018 20:22_

I think this proposal is OK. It is conceptually very simple. It really is just exposing another UnsafeQueueUserWorkItem that instead of passing the pieces of the work item (the delegate and state) it allows the (advanced) user to take control of the work item state (typically to add more state, but Also to allow pooling or other advanced things).

I agree we should overload the method name UnsafeQueueUserWorkItem (it is doing the same thing just with different starting points), but because we chose that we need to change the return type to bool' to match the other overloads.

[terrajobst]

Seems reasonable, but the review brought up a few things:

1. Are we OK with people being able to queue instances of the existing types that implement IThreadPoolWorkItem, such as Task?

2. Are we OK with people being able to to invoke IThreadPoolWorkItem.Execute() on Task?

3. Can we hide the interface a bit, by e.g. making it a nested type of ThreadPool?

@stephentoub says he needs to think about that 🤔

[davidfowl]

How long will it take to think about that 😄 ?

[stephentoub]

Until I or someone can come up with a way to do this safely while still meeting the goals, which I've not been able to do. I am concerned about the issues @bartonjs and @GrabYourPitchforks raised: Task is not hardened against doing ThreadPool.UnsafeQueueUserWorkItem(task, ...) nor ((IThreadPoolWorkItem)task).Execute() and adding such hardening would add measurable cost. Doing that would lead to hard to diagnose race conditions and other such problems. We could document "don't do that", but you know people will: you've got an IThreadPoolWorkItem, and oh look, an API that queues such work items, cool.

[vancem]

I have not thought about it carefully, but it seems we could rename the current IThreadPoolWorkItem to be IThreadPoolWorkItemInternal Have IThreadPoolWorkItem inherit from IThreadPoolWorkItemInternal. All current logic works, and all NEW types that use the public IThreadPoolWorkItem are also allowed, but existing types can't be passed to UnsafeQueueUserWorkItem (Because they don't implement that interface).

Does that address the concern?

[JeffCyr]

These concerns are not limited to Task, it's as bad if a new type implements IThreadPoolWorkItem in a library and the user of that library manually calls Execute or pass it to UnsafeQueueUserWorkItem.

However I don't see this as a big deal if the xmldoc says why you should not do it. An analyzer could even produce a warning when using the method and error if Task if passed directly to it.

[stephentoub]

Does that address the concern?

@vancem, a public interface can't inherit an internal one, can it? That approach was one of the first I'd considered but dismissed it as a result.

[stephentoub]

One approach I'm still mulling over is making ThreadPoolWorkItem an abstract class instead of an interface. Then we could have it implement an internal interface. But it would burn a base class for anyone wanting to derive from it.

[benaadams]

What about

namespace System.Runtime.ThreadPool
{
    public interface IThreadPoolWorkItem
    {
        void Execute();
    }
}

public partial class ThreadPool
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    public static void UnsafeQueueUserWorkItem(IThreadPoolWorkItem workItem, bool preferLocal);
}

[JeffCyr]

@stephentoub The abstract base class is a little inconvenient, but it could also fix the general case if ThreadPool.UnsafeQueueWorkItem was transformed by a protected method on the base class. Only the implementor could queue it on the ThreadPool.

[fredrikhr]

What @benaadams said, but maybe also apply [EditorBrowsable(EditorBrowsableState.Never)] to the interface method.

That would prevent many people calling Execute on Task (when Task is casted to IThreadPoolWorkItem), because you'd have to go out of your way to actually call that

[JeffCyr]

@stephentoub The stronger encapsulation of a base class is appealing, but it could force an extra allocation when deriving is not possible which half defeat the purpose of this api.

Could the interface work if the public ThreadPool.UnsafeQueueWorkItem method ends up calling Task.ExecuteEntry and keep an internal method that calls Task.ExecuteEntryUnsafe. Using the api with task would still not be recommended but it would prevent the race condition.

[stephentoub]

Have IThreadPoolWorkItem inherit from IThreadPoolWorkItemInternal

https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0061

These concerns are not limited to Task, it's as bad if a new type implements IThreadPoolWorkItem in a library and the user of that library manually calls Execute or pass it to UnsafeQueueUserWorkItem.

My expectation is that the vast majority of implementations of this interface (which I expect to be a fairly small number, relatively) will be internal. In contrast, Task is a widely used exchange type, hence my focus on it.

I don't see this as a big deal if the xmldoc says why you should not do it.

Even so, the type implements the interface, there's a method that's explicit purpose is to queue things that implement that interface, you know it's going to happen :). People might even do it thinking they're supposed to: I've created a task, now I want it to run, oh look that method makes it run, let me call it.

To some extent I'm playing devil's advocate here; I'm probably more on the fence than I'm coming across. But it does give me pause.

System.Runtime.ThreadPool

@benaadams, I don't understand; how does putting the interface in a different namespace help? You're just saying that someone would need to import another namespace in order to explicitly cast to the interface? VS will be very helpful there in aiding the user to do so. ;)

[EditorBrowsable(EditorBrowsableState.Never)]

We could take it to the extreme and put it on the the UnsafeQueueUserWorkItem method, on the Execute method, and on the interface itself; it wouldn't show up anywhere in IntelliSense (until you successfully wrote a compiling call, then hovering over the method would still show you the IntelliSense for it).

Maybe that's sufficient mitigation, along with the "Unsafe" prefix and the docs saying "Only pass objects for which you own the type and know it's safe to pass; never, ever, ever pass another object, like Task... awful, horrible, no good, very bad things will happen if you even think of doing so." I'm always hesitant to rely on EditorBrowsableState.Never, but maybe in this case?

The stronger encapsulation of a base class is appealing, but it could force an extra allocation when deriving is not possible which half defeat the purpose of this api.

This is what I meant by "But it would burn a base class for anyone wanting to derive from it.". I suspect it wouldn't actually defeat much of the purpose, though, in that I still suspect the vast majority cases would still be ok with it. I see two main use cases:

• The one that started this issue: you've got additional state you want to carry with the work item in a strongly-typed fashion that'll help with debugging and other such things. In that case, you likely are fine with it as a base type.
• An object that you use over and over to repeatedly queue. In that case, it's most likely associated with some other, longer-lived object, and it'd be ok incurring one additional allocation that's then used over and over and over, while the long-lived object has whatever base type it wants.

Obviously there's some hand-waving here, and obviously there are always caveats. I also prefer an interface in this situation. For reference, though, there are currently six implementations of the interface in Corelib, and all of them (except for Task) would be ok with the base class approach (one of them would just require making it the base type of a base type it already has).

Could the interface work if the public ThreadPool.UnsafeQueueWorkItem method ends up calling Task.ExecuteEntry and keep an internal method that calls Task.ExecuteEntryUnsafe. Using the api with task would still not be recommended but it would prevent the race condition.

No. This feature needs to be pay-for-play, which means it can't add cost to every other work item, which means we don't want to make the ThreadPool, for example, store work items as objects and type test for various interfaces it might need to invoke through.

[JeffCyr]

@stephentoub All good points, the choice is difficult :)

No. This feature needs to be pay-for-play, which means it can't add cost to every other work item, which means we don't want to make the ThreadPool, for example, store work items as objects and type test for various interfaces it might need to invoke through.

I meant, a ThreadPool.UnsafeQueueUserWorkItemInternal(Task task) method could set a ScheduledByRuntime flag on the task that will be checked in Task.ExecuteWorkItem to call ExecuteEntryUnsafe, this should not add any overhead.

The public ThreadPool.UnsafeQueueUserWorkItem would not set that flag and the safe ExecuteEntry method would be used. So you could bypass the task scheduling with this api but it would still be thread safe.

That combined with appropriate roslyn analyzer and the issue is mostly mitigated.

FYI Here is a case where I would need an extra alloc if it ends up being a base class:
https://github.com/JeffCyr/SeerDotNet/blob/bff4e29cf95f2d961ab674119560472d8fb03148/src/Seer.Futures/Runtime/FutureAsyncMethodBuilder.cs#L251

[stephentoub]

FYI Here is a case where I would need an extra alloc if it ends up being a base class

Not if you changed the base class of Promise ;)

a ThreadPool.UnsafeQueueUserWorkItemInternal(Task task) method could set a ScheduledByRuntime flag on the task that will be checked in Task.ExecuteWorkItem to call ExecuteEntryUnsafe

What prevents someone from passing that same Task object to the public method after that flag has already been set?

[JeffCyr]

What prevents someone from passing that same Task object to the public method after that flag has already been set?

Oh that's right I haven't thought that through. What if IThreadPoolWorkItem has a OnWorkItemQueued method that Task can implement with a throw new NotSupportedException(), the public api would call the method and not the internal which only accept a task.

Not if you changed the base class of Promise ;)

Yes it could but that would not be clean :)

[stephentoub]

What if IThreadPoolWorkItem has a OnWorkItemQueued method that Task can implement with a ThrowNotSupportedException, the public api would call the method and not the internal which only accept a task.

Now every work item (other than internal ones) involves another interface call. So, yeah, it'd be pay-for-play, at the expense of, well, expense :) It's an interesting idea, though. Maybe? Still wouldn't help the cast-and-call case, though.

Yes it could but that would not be clean :)

Welcome to my world ;)

[davidfowl]

Please no base class 😒. I honestly don’t think this is that big a deal but I’m hoping we can come to some practical solution here.

I really hate the editable browsable never solution. It’s a public API let’s not treat everyone like toddlers by hiding it (I have toddlers so I can say that). The BCL is used by both high and low level libraries and applications. Maybe there’s a more novel way to mark these APIs so that something like an analyzer can flag them as advanced or something.

[JeffCyr]

Okay what about:
```c#
public interface IThreadPoolWorkItem
{
//Roslyn analyzer to issue warning (or error?) when called directly
void Execute();
}

// Implemented by internal classes
public interface IUnsafeCustomThreadPoolWorkItem : IThreadPoolWorkItem
{ }

// Implemented by public classes
public interface ICustomThreadPoolWorkItem : IThreadPoolWorkItem
{
void OnWorkItemQueued();
}

public class ThreadPool
{
public static void UnsafeQueueUserWorkItem(IUnsafeCustomThreadPoolWorkItem workItem, bool preferLocal);

public static void UnsafeQueueUserWorkItem(ICustomThreadPoolWorkItem workItem, bool preferLocal);

internal static void UnsafeQueueUserWorkItem(IThreadPoolWorkItem workItem, bool preferLocal);

}

public class Task : IThreadPoolWorkItem // Explicitly implemented
{

}
```

Task

You can't call the public ThreadPool api since it doesn't accept an IThreadPoolWorkItem.

Internal types

They implement IUnsafeCustomThreadPoolWorkItem and only have one virtual call.

Public types

They implement ICustomThreadPoolWorkItem and can implement safety check in OnWorkItemQueued to prevent double enqueue. This extra satefy cost an extra virtual call.

---

You could still cast to IThreadPoolWorkItem and call Execute, but the roslyn analyzer warning (or error?) is strong indication that you shouldn't

[stephentoub]

Two public interfaces is an interesting approach; I agree that could address the ThreadPool case. I don't think the extra interface method is that beneficial, though; any detection you did there could instead be done in Execute, without an extra interface dispatch, and then it's just a matter of failure mode. As for an analyzer, I hear you, but a) it can be turned off and b) if it's not it has dev-time/build-time cost (maybe negligible?)... doesn't seem to add a whole lot of value beyond the never browsable attribution.

[davidfowl]

The value is that it doesn’t break intellsense. It can detect the case when a Task can is passed to the API so it is different and better. Sure it can be turned off but what does that matter?

[stephentoub]

It can detect the case when a Task can is passed to the API so it is different and better.

It can... sometimes. Unless you're planning on implementing interprocedural flow analysis, it will not catch:
```C#
Foo(task);
...
internal void Foo(IThreadPoolWorkItem workItem) => workItem.Execute();

and I suspect it wouldn't even catch:
```C#
IThreadPoolWorkItem item = task;
...
item.Execute();

without non-trivial effort.

Sure it can be turned off but what does that matter?

It matters because the moment you start having a non-trivial number of false positives, analyzers get turned off because someone deems them to yield more pain than benefit, and then you have a very misuable API without the very protections that deemed it acceptable to ship anyway.

It matters because it may be turned off by one person on a team and then bite another unsuspecting developer on the team.

It matters because either the analyzer looks just for a pattern of ((IThreadPoolWorkItem)task).Execute(), in which case it misses a bunch of other potential use, or it looks for any usage of Execute, in which case it's going to have false positives for all legitimate usage, at which point it gets turned off.

I'll also add that, on a practical matter, relying on an analyzer here turns this from a small "this will take someone an hour to implement and test" feature to a much larger one. The analyzer will need to be written and tested. There are currently no analyzers that ship with coreclr/corefx, so someone will need to figure out how that's done, how the relevant dependencies are appropriately leveraged in the coreclr/corefx build system, how the component is packaged, how it's installed as part of .NET Core deployment, etc. (it shouldn't just be with VS, but rather anywhere the API lives)., how it's also available for other implementations, like Mono. And so on. At that point, this moves well beyond a "this would be a very small addition, we can perceive there'd be at least some small benefit, and so we might as well" to "this adds a lot of work, now and on-going maintenance, so someone needs to prove that the benefits outweigh the costs, which means demonstrating, for example, that this will measurably move the needle on key real-world benchmarks, impactfully improve the debugging experience on real apps, etc."

It’s a public API let’s not treat everyone like toddlers by hiding it

This isn't about treating people "like toddlers." This about not adding APIs that lead developers to a pit of failure.

[stephentoub]

I really hate the editable browsable never solution. It’s a public API let’s not treat everyone like toddlers by hiding it

I also generally dislike EditorBrowsable, as I noted earlier ("I'm always hesitant to rely on EditorBrowsableState.Never"). In this case, though, I'm not sure it's so bad. The Execute API we're talking about here is one where we want a visibility that's effectively "it can be called internally, but implemented publicly"... almost as if we supported a sort of "protected internal" on interface members. Marking it EditorBrowsableState.Never gets relatively close that, at least from an IntelliSense/usability perspective. If the interface itself is left unadorned but the Execute method is annotated, then it won't show up in IntelliSense for someone dotting off the interface:

but the interface will show up for someone implementing it:

and VS will still offer to implement the interface for you, including Execute:

So, as a strawman, if we were to take @JeffCyr's tweak on the two interface idea (having both be public instead of the derived public and the base internal) in order to protect against queueing Task, and combine that with the [EditorBrowsable(EditorBrowsableState.Never)] for Execute, that gets us to:
```C#
public static class ThreadPool
{
...
public static bool UnsafeQueueUserWorkItem(ICustomWorkItem workItem);

public interface ICustomWorkItem : IWorkItem { }

public interface IWorkItem
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    void Execute();
}

}

public class Task : IWorkItem { ... }
```
I'd much prefer it if there wasn't the strangeness about the base interface, of course, and if we could actually prevent the Execute invocation rather than just hoping no one notices they can call it.

To me, that ends up being the best of the variants thus far. Which, unless we can come up with something even better, leaves me seeing the following reasonable options:
(1) Ship that.
(2) Don't add the API.
(3) Add the original API, document it, and hope for the best.

[JeffCyr]

(1) Ship that.

You would probably want to hide IWorkItem with [EditorBrowsable(EditorBrowsableState.Never)] too, because we want to discourage people from using entirely.

I'm not sure if nesting the interfaces under ThreadPool is a good thing, because you browse the ThreadPool's members a lot more than all types under System.Threading. Having the nested interfaces under ThreadPool polutes its intellisense for the 99% use-case.

(2) Don't add the API.

Please no :)

---

(4) Use the base abstract class
I prefer (1), but the abstract class does solve the safety issue if options (1) and (3) are discarded:

```c#
public static class ThreadPool
{
internal void UnsafeQueueUserWorkItem(ThreadPoolWorkItem workItem, bool preferLocal);
}

public abstract class ThreadPoolWorkItem
{
internal void ExecuteInternal()
{
Execute();
}

protected virtual void Execute();

protected void UnsafeQueueWorkItem(bool preferLocal)
{
    ThreadPool.UnsafeQueueUserWorkItem(this, preferLocal);
}

}

```

[stephentoub]

You would probably want to hide IWorkItem with [EditorBrowsable(EditorBrowsableState.Never)] too, because we want to discourage people from using entirely.

If we want to discourage people from using it entirely, then we shouldn't ship it in the first place ;-)

What we want is to discourage calling the method, but implementers of the interface are fine to use it, and marking the interface as such would hide it from them, too. At that point you may as well hide the ThreadPool overload, too. The argument there would presumably be that this is so advanced, you have to know about it already in order to use it.

because you browse the ThreadPool's members a lot more than all types under System.Threading. Having the nested interfaces under ThreadPool polutes its intellisense for the 99% use-case.

Yes and no... assuming "ThreadPool" is in the name of the interface, it would then show up in IntelliSense as you were typing "ThreadPool" in order to be able to call UnsafeQueueUserWorkItem. But you might be right that such a case is the lesser of two evils.

I prefer (1), but the abstract class does solve the safety issue if options (1) and (3) are discarded:

Right, this is why I raised it earlier. It solves all the problems, at the expense of introducing a new one.

[stephentoub]

There's one other option to address the UnsafeQueueUserWorkItem case, that addresses all the problems, albeit at a small potential expense for public usage that would need to be measured:
```C#
public static bool UnsafeQueueUserWorkItem(IWorkItem workItem)
{
if (workItem is Task) throw ...;
UnsafeQueueUserWorkItemInternal(workItem);
return true;
}

internal static void UnsafeQueueUserWorkItemInternal(IWorkItem workItem)
{
...
}
```
so the public API gets one added type check. In most cases hopefully that would be negligable.

That still leaves the (arguably less likely) direct Execute invoke case to ponder.

[stephentoub]

I'm also going to explore the impact of various changes to how ThreadPool executes things to see if there might be a way to handle this internally without measurable negative impact on existing cases.

[benaadams]

@stephentoub if you could put those tests on github somewhere (perhaps in a gist); I have a PR that they would be helpful with ;)

[stephentoub]

Thank you everyone for the good discussion.

At this point, I've mostly convinced myself (via implementing and measuring various approaches) that my concerns about extra overhead on the dispatch path were largely unnecessary, meaning we could add a type check for the interface and have it be in the noise. That means we could change the implementation so that Task isn't an IThreadPoolWorkItem and is instead recognized specially by the ThreadPool, which then means there are no concerns around the API ending up exposing CoreLib implementation details because no such details are exposed (Task is the only CoreLib type that was implementing the interface and that was exposed in any way out of the library).

With that, we have @terrajobst's three questions...

Are we OK with people being able to queue instances of the existing types that implement IThreadPoolWorkItem, such as Task?

Now irrelevant.

Are we OK with people being able to to invoke IThreadPoolWorkItem.Execute() on Task?

Now irrelevant.

Can we hide the interface a bit, by e.g. making it a nested type of ThreadPool?

As @JeffCyr highlighted, it wouldn't necessarily hide it, it would just move it to different IntelliSense, i.e. you type "ThreadPool" then "." and then the method you want to call... if you have "IThreadPoolWorkItem" at the same level as "ThreadPool", then you see it as you're typing "T...h...r...e...a..." etc. in the IntelliSense list. But if you make it a nested type, then you see it when you dot off "ThreadPool" in order to see the list of what you can access. I'm not sure which is better. So unless I hear otherwise, I'll just stick with it in the System.Threading namespace.

if you could put those tests on github somewhere (perhaps in a gist); I have a PR that they would be helpful with ;)

Unlike many other ThreadPool changes that involve synchronization and load and the like, the nice thing about this one is it only has an impact on the dispatch loop, which means simple tests get at any overheads, so I just used simple tests like:
```C#
const int Iters = 100_000;

[Benchmark(OperationsPerInvoke = Iters)]
public static void QuwiMonomorphic()
{
    using (var ce = new CountdownEvent(Iters))
    {
        for (int i = 0; i < Iters; i++)
        {
            ThreadPool.QueueUserWorkItem(s => ((CountdownEvent)s).Signal(), ce);
        }
        ce.Wait();
    }
}

[Benchmark(OperationsPerInvoke = Iters)]
public static void QuwiPolymorphic()
{
    using (var ce = new CountdownEvent(Iters))
    {
        for (int i = 0; i < Iters / 2; i++)
        {
            ThreadPool.QueueUserWorkItem(s => ((CountdownEvent)s).Signal(), ce);
            ThreadPool.QueueUserWorkItem<CountdownEvent>(s => s.Signal(), ce, preferLocal: false);
        }
        ce.Wait();
    }
}

[Benchmark(OperationsPerInvoke = Iters)]
public static void TaskOnly()
{
    using (var ce = new CountdownEvent(Iters))
    {
        for (int i = 0; i < Iters; i++)
        {
            Task.Factory.StartNew(s => ((CountdownEvent)s).Signal(), ce, default, TaskCreationOptions.DenyChildAttach | TaskCreationOptions.PreferFairness, TaskScheduler.Default);
        }
        ce.Wait();
    }
}

[Benchmark(OperationsPerInvoke = (Iters / 3) * 3)]
public static void TaskAndQuwiPolymorphic()
{
    using (var ce = new CountdownEvent((Iters / 3) * 3))
    {
        for (int i = 0; i < Iters / 3; i++)
        {
            ThreadPool.QueueUserWorkItem(s => ((CountdownEvent)s).Signal(), ce);
            ThreadPool.QueueUserWorkItem<CountdownEvent>(s => s.Signal(), ce, preferLocal: false);
            Task.Factory.StartNew(s => ((CountdownEvent)s).Signal(), ce, default, TaskCreationOptions.DenyChildAttach | TaskCreationOptions.PreferFairness, TaskScheduler.Default);
        }
        ce.Wait();
    }
}

```
though I ran them individually, and even then it wasn't clear to me the monomorphic vs polymorphic were actually measuring what I wanted them to measure; it was also hard to get very stable numbers. Even so, it gave a decent sense for any overheads incurred.

[benaadams]

it was also hard to get very stable numbers

Threadpool's random local queue start selection for steals is one reason is hard

[stephentoub]

Threadpool's random local queue start selection for steals is one reason is hard

Yes in general. But there shouldn't be any stealing here.

[stephentoub]

Closed by dotnet/corefx#32859