Avoid hitting the UI thread when retrieving MEF specific services #4229
Conversation
|
LGTM! Just need to update a couple of code comments. 👍 |
|
@jebriede Thanks. Addressed your feedback + fixed the failing unit tests. |
There was a problem hiding this comment.
I love that most of the changes in the PR are moving off the overly-generic GetInstanceAsync, to the more specific GetComponentModelServiceAsync method. While GetInstanceAsync was convenient and allowed developers to not think about whether something was a MEF or VS or DTE service, it has perf impact, which we're now addressing. I'd love GetInstanceAsync to be considered an anti-pattern that we should stop using eventually. Is it feasible to mark GetInstanceAsync as obsolete, and put in supressions for all existing usage? (in a different PR) That could discourage new usage, and provide extra motivation to slowly refactor existing code to remove the supressions and use more specific methods instead.
Personally, I'm also not a fan of using MEF as a dependency injection framework. MEF is great for extensibility, allowing one team to use an interface and a different team to implement that interface. But, I know that now is not a good time to think about changing how we use MEF/do dependency injection.
| // VS Threading Rule #1 | ||
| // Access to ServiceProvider and a lot of casts are performed in this method, | ||
| // and so this method can RPC into main thread. Switch to main thread explictly, since method has STA requirement | ||
| await NuGetUIThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(); |
There was a problem hiding this comment.
GetInstanceAsync makes 3 attempts, returning early when the instance is found.
- DTE service
- MEF service
- VS Service
1 and 3 need the UI thread, and by removing this switch to UI thread, trying to get a VS service from a background thread will:
- Call into
GetDTEServiceAsyncon the background thread GetDTEServiceAsyncswitches to the UI thread- Whatever the return value, the
awaitinGetInstanceAsynccauses the continuation to be scheduled on a background thread - Calls
GetComponentModelServiceAsync, which completes on the current (background) thread - Calls into
GetGlobalServiceAsync, which switches to the UI thread - The
awaitinGetInstanceAsynccauses the continuation to be scheduled on a background thread - Finally return the value to the caller. I don't know the .NET async implementation well enough to know if the caller's continuation happens immediately, or is scheduled.
The point I'm trying to make is that all these thread switches cause latency while the task is scheduled, and then an appropriate thread, when not busy, can pick it up and run it. The more thread switches that happen, the more latency, especially on the UI thread, since there can only be one of those.
I don't know if it's a safe assumption that DTE services will never be available in MEF, but if that's true, you can re-arrange the order to try MEF first on the current thread, then switch to the UI thread before trying DTE and VS/global services, to minimize thread switching. On the other hand, if a service is available both via DTE and MEF, it's probably better for us to get it via MEF and not need a thread switch at all.
Otherwise, I don't think we should change GetInstanceAsync to stop switching to the UI thread. Your other changes to have callers use GetComponentModelService instead of GetInstanceAsync is great, but I think those changes should be sufficient for this PR. This change to GetInstanceAsync can, in my opinion, be undone.
There was a problem hiding this comment.
I honestly think #1 might be something we might be able to remove.
My plan is to ask folks if it is possible for a service to be DTE, but not in the service provider.
I am also figuring out that we still need to special System.IServiceProvider, which is disappointing.
I think we can eventually remove it. My goal is to remove legacy service acquisition methods if possible, so that you only go for Service Provider or MEF depending on the situation as you suggested. You can create sub issues in NuGet/Home#11198 if you're motivated :)
Yeah, there's no better alternative. |
Bug
Fixes: NuGet/Home#11201
Regression? Last working version:
Description
Avoid using methods that access the UI thread when requesting services exposed via MEF, use free threaded calls instead.
A few things that this PR does:
A few things that this PR doesn't do:
A review of the other codepaths will be done in Review all sync ServiceLocator calls and move to async where possible Home#11203
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation