Async experimentation service #30929
Conversation
| private readonly MethodInfo _isCachedFlightEnabledInfo; | ||
| private readonly IAsyncServiceProvider _asyncServiceProvider; | ||
|
|
||
| private Optional<Func<string, bool>> _isCachedFlightEnabled; |
There was a problem hiding this comment.
since this is in master, i would think the risk woudl be lower for trying something like AsyncLazy (either roslyn's or JTFs). I'd still prefer that myself.
There was a problem hiding this comment.
AsyncLazy includes a single-invocation guarantee for the initializer at the expense of complexity. This feature does not rely on this constraint.
There was a problem hiding this comment.
Well, complexity is in the eye of the beholder :) When i see lazy intialization code that doesn't go through one of these helpers, then you have to then spend cycles going "ok... is this going to be ok in the presence of multiple initializers coming in". Sometimes an abstraction/indirection can help toward reducing complexity.
That said, i'm not going to press on this. This is all self contained, and if you feel that the current approach is fine, that's ok with me. I might request though that we put a comment in the code explicitly stating that we've though about the case where this is called into from multiple threads, and it will still be safe in that case since it will just mean in rare cases we get a little extra computation that is then thrown away. The comment at least makes it clear we thought about this though. Thanks!
There was a problem hiding this comment.
@jasonmalinowski I think we need a helper for asynchronous optimistic initialization...
Basically the asynchronous form of LazyInitializer.EnsureInitialized<T>(T, Func<T>).
There was a problem hiding this comment.
Yeah, I'd use AsyncLazy, if only because it means the field can be made readonly and nobody can access the other thing by accident. If this was perf-sensitive then maybe I'd care.
There was a problem hiding this comment.
p.s.: I should say "AsyncLazy"-like thing, as ours today probably isn't safe for JTF.
| @@ -69,7 +72,7 @@ public void OnSuggestedActionExecuted(SuggestedAction action) | |||
| !IsVsixInstalled() && | |||
| IsCandidate()) | |||
| { | |||
| ShowInfoBarIfNecessary(); | |||
| ThreadingContext.JoinableTaskFactory.Run(() => ShowInfoBarIfNecessaryAsync(CancellationToken.None)); | |||
There was a problem hiding this comment.
can we (manually) test to make sure this works safely and doesn't like deadlock or anything :D
src/VisualStudio/Core/Def/Implementation/Experimentation/KeybindingResetDetector.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/Experimentation/KeybindingResetDetector.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Experimentation/VisualStudioExperimentationService.cs
Outdated
Show resolved
Hide resolved
| { | ||
| ThisCanBeCalledOnAnyThread(); |
There was a problem hiding this comment.
How are we able to prevent additional calls on this that aren't using JTF? I agree this is a good and appropriate use in the Roslyn codebase, although I assume we've already asked the team that owns SVsExperimentationService what it would take to make it fully free-threaded? It's just that right now this is special in our codebase and different than everything else, which almost guarantees a bug at some point.
There was a problem hiding this comment.
How are we able to prevent additional calls on this that aren't using JTF?
I don't understand this question.
although I assume we've already asked the team that owns SVsExperimentationService what it would take to make it fully free-threaded
In this pull request, SVsExperimentationService is used under the assumption that it is already free-threaded. This behavior is carried over from the behavior before this pull request.
It's just that right now this is special in our codebase and different than everything else,
I don't understand this comment.
|
💡 Can remove the explicit instantiation of this service now: |
|
I'm waiting to clean this up after dev16.0.x merges forward to dev16.0-preview2. |
|
This is no longer applicable (#55902) |
This pull request builds on #30916 by converting the service to asynchronous with a fully free-threaded constructor.