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

make DedicatedThreadPool.QueueUserWorkItem<T> generic #6156

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Oct 6, 2022

Changes

Done to avoid boxing allocations - however, the tasks this effects are few and far between so there's not much in the way of measurable impact. Still, may not be a bad idea.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Latest v1.4 Benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.2006 (21H2)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.201
  [Host]     : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT
  DefaultJob : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT

Method TaskCount Configurator Mean Error StdDev Gen 0 Allocated
RunDispatcher 1000000 ChannelD 2,461.4 ms 40.62 ms 38.00 ms 161000.0000 641 MB
RunDispatcher 1000000 DefaultThreadPool 1,012.2 ms 35.57 ms 104.88 ms 15000.0000 61 MB
RunDispatcher 1000000 ForkJoin(remote) 596.8 ms 20.85 ms 61.48 ms 7000.0000 31 MB
RunDispatcher 1000000 ForkJoin(sys) 620.4 ms 12.40 ms 26.69 ms 7000.0000 31 MB
RunDispatcher 1000000 TaskD 1,181.1 ms 13.48 ms 12.61 ms 23000.0000 92 MB

This PR's Benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.2006 (21H2)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.201
  [Host]     : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT
  DefaultJob : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT

Method TaskCount Configurator Mean Error StdDev Median Gen 0 Allocated
RunDispatcher 1000000 ChannelD 2,484.7 ms 22.17 ms 19.65 ms 2,482.8 ms 161000.0000 641 MB
RunDispatcher 1000000 DefaultThreadPool 1,123.4 ms 59.06 ms 174.15 ms 1,172.5 ms 15000.0000 61 MB
RunDispatcher 1000000 ForkJoin(remote) 606.5 ms 12.10 ms 34.90 ms 612.9 ms 7000.0000 31 MB
RunDispatcher 1000000 ForkJoin(sys) 625.1 ms 12.12 ms 15.76 ms 625.3 ms 7000.0000 31 MB
RunDispatcher 1000000 TaskD 1,129.0 ms 31.68 ms 93.41 ms 1,149.7 ms 23000.0000 92 MB

@Aaronontheweb Aaronontheweb added akka-actor perf akka.net v1.4 Issues affecting Akka.NET v1.4 labels Oct 6, 2022
@Aaronontheweb Aaronontheweb added this to the 1.4.44 milestone Oct 6, 2022
@Aaronontheweb
Copy link
Member Author

Originally pasted the wrong baseline benchmark figures - this has been fixed.

@Aaronontheweb
Copy link
Member Author

Ah, might need to be another spot I make generic in order to eliminate all of the boxing allocations - the ThreadPoolWorkQueue<T> itself.

@Aaronontheweb
Copy link
Member Author

Same as before - no impact really

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.2006 (21H2)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.201
  [Host]     : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT
  DefaultJob : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT

Method TaskCount Configurator Mean Error StdDev Median Gen 0 Allocated
RunDispatcher 1000000 ChannelD 2,542.9 ms 20.09 ms 15.69 ms 2,545.7 ms 161000.0000 641 MB
RunDispatcher 1000000 DefaultThreadPool 992.5 ms 45.72 ms 134.82 ms 993.7 ms 15000.0000 61 MB
RunDispatcher 1000000 ForkJoin(remote) 609.0 ms 12.09 ms 26.80 ms 613.3 ms 7000.0000 31 MB
RunDispatcher 1000000 ForkJoin(sys) 619.1 ms 12.31 ms 19.16 ms 621.6 ms 7000.0000 31 MB
RunDispatcher 1000000 TaskD 1,138.9 ms 29.87 ms 88.08 ms 1,164.7 ms 23000.0000 92 MB

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Arkatufus Arkatufus enabled auto-merge (squash) October 7, 2022 14:52
@Aaronontheweb Aaronontheweb disabled auto-merge October 7, 2022 15:59
@Aaronontheweb Aaronontheweb merged commit 23e5d92 into akkadotnet:v1.4 Oct 7, 2022
@Aaronontheweb Aaronontheweb deleted the dtp-queue-user-work-item-generic branch October 7, 2022 15:59
@to11mtm
Copy link
Member

to11mtm commented Oct 7, 2022

I'd almost want to review the IL/ASM around this...

It's tricky because AFAIK not all IRunnable queued are structs... right?

Also, I'd worry that there -might- still be boxing as a result...

I mean, here's a -really-, REALLY crazy idea. Looking at #6143, I see you added this:

_pool.QueueUserWorkItem(new RequestWorkerTask(this));

Is there a reason, that a given DTP, couldn't just do:

public class RequestWorkerTask : IRunnable //Yes, make it a class.
{
  //existing impl
}

//In DedicatedThreadPoolTaskScheduler

private readonly RequestWorkerTask _workerReqTask = new RequestWorkerTask(this);

        private void RequestWorker()
        {
            _pool.QueueUserWorkItem(_workerReqTask);
        ]

That way you'd eliminate boxing while only paying the single alloc... Right?

@Aaronontheweb
Copy link
Member Author

It's tricky because AFAIK not all IRunnable queued are structs... right?

Virtually none are structs.

@to11mtm
Copy link
Member

to11mtm commented Oct 7, 2022

Then yeah try my idea .....

As it stands I would -expect- any struct IRunnable to wind up boxing when queued/dequeued to the ConcurrentQueue in DTP.

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Oct 8, 2022
)

* make `DedicatedThreadPool.QueueUserWorkItem<T>` generic

Done to avoid boxing allocations

* make adding methods generic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
akka.net v1.4 Issues affecting Akka.NET v1.4 akka-actor perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants