Try to fix a problem that hardcoded thread schedule defeats the effort to prioritize UI blocking tasks. #1159
Conversation
Preparing resource is a common routine to access evaluated project, most of the time, it is close to no-op when the evaluation is up to date. Because the code always schedules the call to the thread pool, it defeats the very reason to have a priority queue for high priority work when thread pool is too busy. It also makes it hard to throttle project evaluations through special task scheduler. The change is to try not to change the behavior, when the code is called on UI thread (as it can lead into new problems), but try to use the current task scheduler if possible.
| @@ -768,6 +769,7 @@ private Task PrepareResourceAsync(TResource resource, CancellationToken cancella | |||
| AsyncReaderWriterResourceLock<TMoniker, TResource>.Helper.ResourceState finalState = forConcurrentUse ? ResourceState.Concurrent : ResourceState.Exclusive; | |||
|
|
|||
| Task? preparationTask = null; | |||
| TaskScheduler taskScheduler = TaskScheduler.Current.MaximumConcurrencyLevel > 1 ? TaskScheduler.Current : TaskScheduler.Default; | |||
There was a problem hiding this comment.
This makes me very nervous. There are other TaskSchedulers that may be in the system besides the ones you're thinking of. ReSharper for example has their own custom schedulers. I don't know how they set their MaximumConcurrencyLevel property, but in the past, folks who have used TaskScheduler.Current (usually by accident) often find themselves in deadlocks when their continuations inadvertently rely on a Resharper scheduler which has unknown and unwanted semantics.
There was a problem hiding this comment.
yes, I am not fan of this logic, but have trouble to provide an alternative solution. Another possibility is to create a new virtual method to let the lock to potentially provide the TaskScheduler for PrepareResource work, and this can leave using TaskScheduler.Default, but the project lock service can choose the set of TaskScheduler to be compatible here. Of course, this would introduce a hard-to-explain contract. Let me know if you think that is a better solution here.
There was a problem hiding this comment.
Well, that sounds safer. And I'm a fan of as simple as possible, but no simpler. So in lieu of a simpler idea that is still safe, I like your virtual method idea. I'm guessing your override will do an actual concrete type check on the TaskScheduler to make sure it's the one you have in mind as safe and performant.
There was a problem hiding this comment.
sounds good, and it could make it much easier to apply a throttling of evaluations by using a special scheduler, instead of TaskScheduler.Default if we want to, and use a priority one for UI block/critical responsiveness path there if necessary
There was a problem hiding this comment.
I updated the PR, and please take a look.
test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterResourceLockTests.cs
Outdated
Show resolved
Hide resolved
…esourceLockTests.cs Co-authored-by: Andrew Arnott <Andrew.arnott@Microsoft.com>
Co-authored-by: Andrew Arnott <Andrew.arnott@Microsoft.com>
…to dev/lifengl/schedulingIssues
The reader/writer lock implementation always spin off a new task with hardcoded task scheduler to call PrepareResources. It happens in the critical path to access a project in the project system implementation. Most of the time, when evaluation is up to date, this is expected to be a quick check, but the extra thread pool scheduling leads it to be sensitive to thread pool issues. Because of this hardcoded scheduler usage, it defeats the effort to use a priority queue to finish UI blocking tasks when thread pool has been scheduled for many background works.
The change is to prevent changing the behavior when GetResourceAsync is called on the UI thread, as it may lead new kind of deadlocks.
This is considered to be the reason (at least one reason) why priority queue doesn't seem to work in a 15s UI delay trace provided by David.