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

Use Portable TimerQueue when using Portable ThreadPool #46266

Closed
davidfowl opened this issue Dec 20, 2020 · 8 comments · Fixed by #71864
Closed

Use Portable TimerQueue when using Portable ThreadPool #46266

davidfowl opened this issue Dec 20, 2020 · 8 comments · Fixed by #71864
Assignees
Milestone

Comments

@davidfowl
Copy link
Member

We should use the portable timer queue when the portable thread pool has been selected. As for some of the benefits:

  • The managed timer queue doesn't need to allocate when being scheduled to the thread pool since it implements IThreadPoolWorkItem
  • It simplifies the native/managed interaction between the thread pool and timer queue. Today if you switch to the portable thread pool, the native timer queue needs to schedule work onto it (
    if (UsePortableThreadPool())
    {
    GCX_COOP();
    ARG_SLOT args[] = { PtrToArgSlot(AsyncTimerCallbackCompletion), PtrToArgSlot(timerInfo) };
    MethodDescCallSite(METHOD__THREAD_POOL__UNSAFE_QUEUE_UNMANAGED_WORK_ITEM).Call(args);
    }
    else
    {
    QueueUserWorkItem(AsyncTimerCallbackCompletion,
    timerInfo,
    QUEUE_ONLY /* TimerInfo take care of deleting*/);
    }
    )
  • The other benefits from moving to managed code (easier to maintain, same code on all platforms, etc)

I've spoke to @kouvel about this in passing so filing something just to track it.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Threading untriaged New issue has not been triaged by the area owner labels Dec 20, 2020
@kouvel kouvel removed the untriaged New issue has not been triaged by the area owner label Dec 20, 2020
@kouvel kouvel added this to the 6.0.0 milestone Dec 20, 2020
@kouvel
Copy link
Member

kouvel commented Dec 28, 2020

Should be possible to do after #45901, then it also wouldn't need to be conditioned on whether the portable thread pool is enabled

@davidfowl
Copy link
Member Author

Timer related allocations (UnmanagedThreadPoolWorkItem)

image

Removal of the QCall.

WebApplication40.dll!WebApplication40.DelayAwaiter.UnsafeOnCompleted.AnonymousMethod__8_0(object state) Line 78	C#
System.Private.CoreLib.dll!System.Threading.TimerQueueTimer..cctor.AnonymousMethod__23_0(object state)	Unknown
System.Private.CoreLib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state)	Unknown
System.Private.CoreLib.dll!System.Threading.TimerQueueTimer.CallCallback(bool isThreadPool)	Unknown
System.Private.CoreLib.dll!System.Threading.TimerQueueTimer.Fire(bool isThreadPool)	Unknown
System.Private.CoreLib.dll!System.Threading.TimerQueue.FireNextTimers()	Unknown
-System.Private.CoreLib.dll!System.Threading.TimerQueue.AppDomainTimerCallback(int id)	Unknown
-[Native to Managed Transition]	
-[Managed to Native Transition]	
-System.Private.CoreLib.dll!System.Threading.UnmanagedThreadPoolWorkItem.System.Threading.IThreadPoolWorkItem.Execute()	Unknown
System.Private.CoreLib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch()	Unknown
System.Private.CoreLib.dll!System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()	Unknown
System.Private.CoreLib.dll!System.Threading.Thread.StartCallback()	Unknown

@davidfowl
Copy link
Member Author

@kouvel @mangod9 is this still on deck for 6?

@kouvel
Copy link
Member

kouvel commented Jul 19, 2021

Could probably be done as a bug fix, will see. It looks like the queuing in the native code above uses recycled memory that would likely amortize the allocations for creating/queuing a new work item.

@davidfowl
Copy link
Member Author

Honestly I'm not that worried about the allocations, I just like the idea of all of the managed code pieces being used together. It's not critical for .NET 6 but I would like to see us make more progress on the timers and IO thread pool in .NET 7

@jkotas
Copy link
Member

jkotas commented Jul 19, 2021

This should wait till .NET 7.

@jkotas jkotas modified the milestones: 6.0.0, 7.0.0 Jul 19, 2021
@davidfowl
Copy link
Member Author

@kouvel I think this it the last piece of the puzzle for managed threadpool porting.

@kouvel
Copy link
Member

kouvel commented Apr 27, 2022

I would also like to get this into 7.0, as it would allow deleting all of the native implementation and simplifying at some point thereafter.

@kouvel kouvel self-assigned this Apr 27, 2022
kouvel added a commit to kouvel/runtime that referenced this issue Jul 9, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants