Extract base LanguageServerProjectLoader from LanguageServerProjectSystem #78329
Conversation
ghost
added
Area-Infrastructure
untriaged
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Outdated
Show resolved
Hide resolved
| private readonly IFileChangeWatcher _fileChangeWatcher; | ||
| private readonly IGlobalOptionService _globalOptionService; | ||
| protected readonly ILoggerFactory _loggerFactory; | ||
| protected readonly ILogger _logger; |
There was a problem hiding this comment.
Keep this private, other components can make their own. Or expect the caller to pass it down, since that way it can specify which derived type it is.
| /// <summary> | ||
| /// A single gate for code that is adding work to <see cref="_projectsToLoadAndReload" />. This is just we don't have code simultaneously trying to load and unload solutions at once. | ||
| /// </summary> | ||
| protected readonly SemaphoreSlim _gate = new SemaphoreSlim(initialCount: 1); |
There was a problem hiding this comment.
This being protected is pretty strange -- seems like there should be a method to call that keeps this and the actual queue private. That comment I don't think makes sense though, given we don't actually have any unloading happening, so maybe just delete that while you're here while laughing at me for writing it.
There was a problem hiding this comment.
It seems like only the derived type will need to use the _gate, so, I made it private in there. It feels like the queue itself is not being protected by the gate, but rather the whole process of obtaining a build host and loading projects.
Based on this, it didn't feel super necessary to me to make the queue private and add protected methods for accessing it. It feels like we would just wrap/forward AddWork and WaitUntilCurrentBatchCompletesAsync. Let me know if you have strong preference here and we can work it out.
|
I will merge non-squash in hopes that the renames and stuff will be better reflected in git history that way. |
|
|
||
| if (projectToLoad.ReportTelemetry) | ||
| { | ||
| await _projectLoadTelemetryReporter.ReportProjectLoadTelemetryAsync(telemetryInfos, projectToLoad, cancellationToken); |
There was a problem hiding this comment.
No reason to address this in this PR, but @dibarbet one thing we'll have to think about is how we want to handle telemetry for single-file apps.
There was a problem hiding this comment.
Is there something specific we want to do? We should probably add a property indicating it is a single file app (if there is no other way to tell from the telemetry report already).
Not sure how much telemetry we'll get though since it requires devkit be installed
There was a problem hiding this comment.
We do pass /features:FileBasedProgram to compiler, would that be something telemetry can see? Also the "project file" path will end in .cs.
I would expect that many users using FBP would have devkit installed even if the project system used for their FBPs comes from here.
There was a problem hiding this comment.
We do pass /features:FileBasedProgram to compiler, would that be something telemetry can see?
Only if we were to thread that through. Which might be all we want to make sure we do for now.
There was a problem hiding this comment.
But @dibarbet do we have reports that report 'time to first load' or something we need to update to ensure we don't accidentally count a file-based program as the 'first load'?
There was a problem hiding this comment.
But @dibarbet do we have reports that report 'time to first load' or something we need to update to ensure we don't accidentally count a file-based program as the 'first load'?
This is mainly for tracking usage of .NET (and various workloads - for example we report what TFMs the project targets). Should .NET run programs be tracked as part of that? I assume so, but that might be a Damian question.
It isn't used for performance information currently.
I'm not sure how much it'll really matter, since the number of changes you're making in the other commits is pretty small. And in a few cases I had you make things private that you made protected, so the squashed diff might ultimately be easier. Although you could address that by merging your middle commits (i.e. the commits that aren't the rename and extracting of the binlog name generation) together so that's a bit cleaner. |
RikkiGibson
force-pushed
the
LanguageServerProjectLoader
branch
from
c0fb7e5 to
be598a9
Compare
The first commit renames file LanguageServerProjectSystem.cs to LanguageServerProjectLoader.cs.
The second commit re-creates LanguageServerProjectSystem.cs with new content.
You may find commit-by-commit review easier.
LanguageServerProjectLoader contains most of the logic from before. LanguageServerProjectSystem contains methods specifically for opening solutions and projects.
When we do file-based programs, we will introduce a new project system type, which has a method for opening a file-based program project.
Also, we explicitly stop storing LanguageServerWorkspaceFactory, and instead pass in the specific bits from it that we were using, because eventually LanguageServerWorkspaceFactory will hold both a file-based program workspace and the pre-existing host workspace.