Change Timer implementation on Unixes to use only one scheduling thread

[kouvel]

• Separated from Implement APIs for some threading metrics (CoreRT) #7066

CC @filipnavara

[kouvel]

RE this from #7066:

@filipnavara: I'm not convinced that the improvement is so small for a case where the timers are heavily used.

The max number of scheduled timers is the proc count, so the lock should be held for a very short time and it doesn't scale worse beyond that. CoreCLR queues an APC to the timer thread to insert a new timer, which is nice in that it doesn't need a lock, but I'm not aware of a way to do that in .NET and it would still involve some kind of synchronization in SetTimer to queue the insertion. It may be possible to improve it, and it can be done as needed.

[kouvel]

Made a change to move the queuing out of the lock, as that's probably the most expensive part

[filipnavara]

I became convinced that you are right about it and agree with your reasoning. I always hesitate to add contention on yet another lock, but in this particular case it's unlikely to cause an issue since the number of processor cores is usually quite low. I think there's room for improvement of how the code is structured given how the current three implementations (CoreCLR, CoreRT/Unix, CoreRT/Windows) turned out, but it's nowhere near anything I would even consider a priority.

If I identify some small areas that need to be improved I will be happy to address them specifically.

[filipnavara]

CoreCLR queues an APC to the timer thread to insert a new timer, which is nice in that it doesn't need a lock, but I'm not aware of a way to do that in .NET and it would still involve some kind of synchronization in SetTimer to queue the insertion.

It is possible to get rid of the lock by using lock-free list (eg. the basic idea from ConcurrentStack with _head/_next pointers switched with Interlocked methods). You'd still need the AutoResetEvent. Probably not worth the effort though.

Looking at the code again I came to the conclusion that SetTimer can be call more than once with the same _id while one is already active. This is more obvious in the CoreCLR code:

https://github.com/dotnet/coreclr/blob/333232d98639df980133205c41791cb9dc7f3d34/src/System.Private.CoreLib/src/System/Threading/Timer.CoreCLR.cs#L74-L85

With your implementation it will start creating ScheduleTimer objects queued to the static list instead of updating the existing ones. That's obviously a problem that still needs to be addressed. Implementation in Timer.Windows.cs seems to be fine.

One possible fix is to get rid of ScheduledTimer altogether and just use TimerQueue directly in its place like this: filipnavara@ceb5272. It's not prefect because now I introduced a loop over all the TimerQueues, not just the ones that actually have scheduled timers. That is likely not a problem in practice because the number of the queues is small. What could be a problem is if this introduces some unfairness due to a changed order. If the order wouldn't turn out to be a problem the loops/lists could be optimized with a BitArray-like structure (one for keeping track of TimerQueues where SetTimer was called, another one in place of s_timerIDsToFire).

[kouvel]

Nice catch and good ideas, will fix

[filipnavara]

LGTM... It actually resolves issue with the TimerFiringTests.MultpleTimers_PeriodicTimerIsntBlockedByBlockedCallback test in CoreFX. Can we get it merged?

[filipnavara]

@dotnet-bot test OSX10.12 Debug and CoreCLR tests
@dotnet-bot test OSX10.12 Debug and CoreFX tests

[MichalStrehovsky]

Jenkins is shut down for those configs. I'll close and reopen this to retest with Azure DevOps.

[marek-safar]

@stephentoub could you help to land this change?

[jkotas]

@filipnavara Thanks for signing off on this