Make LS checkers time out after being idle, segment based on use#3363
Make LS checkers time out after being idle, segment based on use#3363jakebailey merged 12 commits intomainfrom
Conversation
|
|
||
| if c.WasCanceled() { | ||
| // Canceled checkers must be disposed. | ||
| p.log(fmt.Sprintf("checkerpool: Checker %d for request %s was canceled, disposing", index, holdTag(requestID))) |
There was a problem hiding this comment.
FWIW we have these logs, but the log func we had passed into the pool is actually a noop. I have not fixed that in this PR.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the project checker pooling mechanism to split diagnostics vs query usage and add idle-time disposal, with configuration wired through session options.
Changes:
- Introduces
CheckerPoolOptionsonSessionOptionsand routes diagnostic requests to a dedicated checker viacore.CheckerPurpose. - Replaces the old
CheckerPoolimplementation with a channel-semaphore-basedcheckerPoolsupporting idle cleanup and program/pool discard. - Adds extensive tests covering routing, affinity, contention, discard, and cleanup behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/project/snapshot.go | Discards a program’s checker pool when the program is no longer referenced by any snapshot. |
| internal/project/session.go | Adds CheckerPoolOptions to session configuration. |
| internal/project/projectcollectionbuilder.go | Discards old pools on program updates and rebinds pool from Program via GetCheckerPool(). |
| internal/project/project.go | Switches projects to the new checkerPool and wires in options on program creation. |
| internal/project/checkerpool.go | Major rewrite: diagnostics vs query checkers, per-request/file affinity, idle cleanup timer, discard behavior, channel semaphores. |
| internal/project/checkerpool_test.go | New test suite validating behavior of the redesigned pool. |
| internal/lsp/server.go | Marks document diagnostics requests as CheckerPurposeDiagnostics. |
| internal/core/context.go | Adds checker-purpose context plumbing (WithCheckerPurpose / GetCheckerPurpose). |
| internal/compiler/program.go | Exposes program checker pool via GetCheckerPool(). |
|
Since diagnostics requests typically come in big waves (one for each open file on startup or any change), I was skeptical of this approach, but I can't see any negative affect after opening a dozen or so files in the VS Code repo. I think it's fine. Is any of the Copilot feedback important? |
|
Yeah, it all seems plausible; I will look at it (was just preoccupied with other stuff and accidentally archived the email with these comments). |
|
Fixed the comments, but, part of it is that the API changes. Though I think the approach here is fine, I'm not entirely certain how this should work for API users, or even tsgolint, where they might want more of these checkers? |
|
@camc314 Do you know how this might affect your usage of these checkers? Is there some sort of setting that would make this not break you, if so? |
|
@jakebailey thanks for the ping I tried this branch on tsgolint, and it all seemed to work ok - thanks for checking. |
Fixes #2115
This is a redo of the project
CheckerPool.N+1checkers, one of which is for diagnostics, one for the API, the rest are for temporary queries. It still defaults to 4 (so, one diagnostic checker, 3 query checkers, one API checker), but is now configurable directly onSessionOptions. That'll mean tsgolint doesn't have to patch us, side benefit.refcount=0, then we'll throw away all idle checkers and stop the cleanup task, moving the pool into a mode where it instantly cleans up any new checker it creates (keeping old snapshots working, albeit less efficiently).context.Contextaware and stop waiting for a checker on cancelled requests, it's just a bigger refactor to plumb errors through.I'll note that while it's "configurable", I just mean code-wise; I am not actually exposing editor settings. Maybe that's a good idea.
It may be that going down to just one diagnostic checker is too restrictive; perhaps that is part of the speed boost in the new LSP. I could expand this out so we have more than one, potentially. Or make it configurable.