Repository navigation
feat: workspace symbols warmup (Phase 2) - #158
AlexCannonball wants to merge 6 commits into
Conversation
|
At the moment still a very early attempt to implement eager warmup + lazy query evaluation from #130 (comment) TODO list from the top of my head:
|
|
Quick update on Phase 2 refactoring: I'm currently pushing all heavy synchronous workloads (Tree-sitter processing, As expected, introducing Right now, the IDE is reporting 100+ cascading type mismatch and lifetime errors. I am currently grinding through these compiler errors layer by layer to align all the type boundaries and will continue pushing updates to this PR as soon as the core compilation passes. Still on it! |
|
Are you still working on this? |
|
Hello @coder3101 Yes, I do. Currently I'm trying to switch LSP notification handling to async. The order of notifications must be preserved and |
74ed283 to
3a4bdcf
Compare
|
@coder3101 Hello, I've updated the draft to share some progress.
I will also comment some interesting findings in separate line comments. So far I'm breaking down
I understand that currently import and grammar diagnostics probably won't work. First, I'd like to compile and test the metamodel cache state and the notification edge cases. |
| .output() | ||
| .ok()?; | ||
| let wait_future = child.wait_with_output(); | ||
| let timeout_result = tokio::time::timeout(CLANG_FORMAT_TIMEOUT, wait_future).await; |
There was a problem hiding this comment.
Improvements:
async, not blocking the main thread- using stdin input, so a temp file isn't needed
- added timeout protection
|
|
||
| pub use clang::ClangFormatter; | ||
|
|
||
| pub trait ProtoFormatter: Sized { |
There was a problem hiding this comment.
Haven't discovered this trait purpose.
There was a problem hiding this comment.
Could have been used for unifying other formatters like buf, clangd etc.
There was a problem hiding this comment.
Thank you! As I remember, There were some troubles with the async trait. I'll keep this open and will try to restore the trait after the cache and async-ification are stabilized.
| // Run protoc and capture output | ||
| match cmd.output() { | ||
| Ok(output) => { | ||
| match tokio::time::timeout(PROTOC_TIMEOUT, cmd.kill_on_drop(true).output()).await { |
There was a problem hiding this comment.
- not blocking the main thread
- timeout protection
|
|
||
| pub struct ProtoLanguageServer { | ||
| pub client: ClientSocket, | ||
| pub(crate) log_handle: log::LogReloadHandle, |
There was a problem hiding this comment.
Moved this to the notification worker.
| } | ||
| } | ||
|
|
||
| const CLANG_FORMAT_TIMEOUT: Duration = Duration::from_secs(2); |
There was a problem hiding this comment.
Should be configurable from config file, on slower systems with large file 2s might be too low.
There was a problem hiding this comment.
Maybe just increasing the timeout from 2 seconds to 5–10 seconds will be enough?
There was a problem hiding this comment.
if not configurable, setting to 5 second is fine, given its async anyways.
There was a problem hiding this comment.
Thank you, I've set it to 5: 51e4c92
I'm not sure about the setting(s) because having too many config options isn't always good for users. Also it increases the complexity:
- Validation and boundaries
- The main TOML config
- Client settings
- CLI args
- ENV
Anyways, we can add a setting later as a follow-up, if needed.
| file_path: &str, | ||
| include_paths: &[String], | ||
| ) -> Vec<Diagnostic> { | ||
| const PROTOC_TIMEOUT: Duration = Duration::from_secs(2); |
| pub shutdown_received: bool, | ||
| pub configs: Arc<RwLock<WorkspaceProtoConfigs>>, | ||
| pub shutdown_token: CancellationToken, | ||
| notification_tx: UnboundedSender<Notification>, |
There was a problem hiding this comment.
Thank you, it's a very good question. I guess I was too optimistic, like, "if everything works fine, the unbounded channel will never consume too many memory"😀
I've changed it to a fail-fast bounded sender, please take a look: 44d7cbf
https://docs.rs/async-lsp/latest/async_lsp/router/struct.Router.html#method.notification
Also, since async-lsp routes notifications through a synchronous API handler, we can't legitimately .await on a full bounded channel without completely stalling the main events loop or introducing heavy overengineering. A fail-fast try_send combined with an explicit WouldBlock error seems to be the most pragmatic and reliable approach here.
A fail-fast strategy for the notification worker channel.
Set `clang-format` and `protoc` run timeout to 5 seconds.
Subscribe to the client file watchers and skip reporting progress if the client doesn't support `window/workDoneProgress/create`
Improvements related to Phase 2 from #130