Speed up VisualStudioProject construction #60351
Conversation
Loading these services during the VisualStudioProject constructor means they're being loaded during the initial project creation which is earlier than they're needed and can hold up other work. Let's not ask for them until we actually need them.
src/VisualStudio/Core/Def/ProjectSystem/VisualStudioProjectFactory.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/VisualStudioProjectFactory.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
force-pushed
the
speed-up-visualstudioproject-construction
branch
from
6f978e9 to
b8a6063
Compare
src/VisualStudio/Core/Def/ProjectSystem/VisualStudioProjectFactory.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
force-pushed
the
speed-up-visualstudioproject-construction
branch
from
b8a6063 to
e05579f
Compare
| // task is completed, we know that the WaitUntilFullyLoadedAsync call will have actually finished and we're | ||
| // fully loaded. | ||
| var isFullyLoadedTask = workspaceStatusService?.IsFullyLoadedAsync(CancellationToken.None); | ||
| var isFullyLoaded = isFullyLoadedTask is { IsCompleted: true } && isFullyLoadedTask.GetAwaiter().GetResult(); |
There was a problem hiding this comment.
does this .IsCompleted check actually work well? i'm worried that some part of the IsFullyLaodedAsync call is actually async (Even trivially), and we just get a task back that this is always false for.
i think i woudl like it if IsFullyLaodedAsync actually guaranteed the behavior you're looking for here by having a single task that it creates and caches and then returns to all future callers. since i think we can only fully load once... that seems viable.
There was a problem hiding this comment.
@CyrusNajmabadi I agree the pattern isn't good, it's not new here. @sharwell might recall if we actually know if this works?
There was a problem hiding this comment.
Original conversation where this pattern was discussed: #48215 (comment)
There is probably a question as to whether we need to maintain the telemetry here at all, given everything that has gone on since Paul originally requested it.
| // After the call to EnsureDocumentOptionProvidersInitializedAsync, everything can be off the UI thread. | ||
| // Thus, we have a ConfigureAwait(false) on the call and switch explicitly after. | ||
| await _visualStudioWorkspaceImpl.EnsureDocumentOptionProvidersInitializedAsync(cancellationToken).ConfigureAwait(false); | ||
| await TaskScheduler.Default; |
There was a problem hiding this comment.
Don't ConfigureAwait(false) and await TaskScheduler.Default do the same thing?
It's possible await TaskScheduler.Default is already completed so it runs synchronously, but seems redundant, maybe I'm missing something?
There was a problem hiding this comment.
@Therzok: the .ConfigureAwait(false) only matters if the EnsureDocumentOptionProvidersInitializedAsync actually yields; if it completes synchronously we'd stay on the UI thread.
There was a problem hiding this comment.
Which to clarify: that means we need the await TaskScheduler.Default; but as @sharwell pointed out we don't want to have the first await come to the UI thread only to immediately bounce to the UI thread, so we kinda end up with this (admittedly odd) pattern.
There was a problem hiding this comment.
That is very good to know, thank you! System.Threading.Task makes me doubt I know anything.
ghost
added this to the 
Closes #55297