perf: Segregate syntax and semantic diagnostics #17775
Conversation
rustbot
added
the
S-waiting-on-review
| .attach_first_edition(file_id) | ||
| .unwrap_or_else(|| EditionedFileId::current_edition(file_id)); | ||
|
|
||
| // [#3434] Only take first 128 errors to prevent slowing down editor/ide, the number 128 is chosen arbitrarily. |
There was a problem hiding this comment.
Previous comment was pointing the wrong issue no.34344
| /// due to macros. | ||
| pub fn diagnostics( | ||
| pub fn semantic_diagnostics( |
There was a problem hiding this comment.
Calling db.parses once in fn syntax_diagnotics(..) and again in this function that previously only called once in fn diagnotics(..) is a bit reluctant, but I think that it won't harm perf much due to lru cache here;
rust-analyzer/crates/base-db/src/lib.rs
Lines 60 to 63 in aa00ddc
ShoyuVanilla
force-pushed
the
segregate-diags
branch
from
f8af927 to
eec6375
Compare
| let slice2 = slice.clone(); | ||
| self.task_pool.handle.spawn(ThreadIntent::LatencySensitive, { | ||
| let snapshot = self.snapshot(); | ||
| let subscriptions = subscriptions.clone(); |
There was a problem hiding this comment.
| let slice2 = slice.clone(); | |
| self.task_pool.handle.spawn(ThreadIntent::LatencySensitive, { | |
| let snapshot = self.snapshot(); | |
| let subscriptions = subscriptions.clone(); | |
| self.task_pool.handle.spawn(ThreadIntent::LatencySensitive, { | |
| let snapshot = self.snapshot(); | |
| let subscriptions = subscriptions.clone(); | |
| let slice = slice.clone(); |
no need to come up with a new name then
| )) | ||
| } | ||
| }); | ||
| self.task_pool.handle.spawn(ThreadIntent::LatencySensitive, { |
There was a problem hiding this comment.
This duplicates the amount of tasks we do now, it also spawns semantic and parser diagnostics tasks in lockstep which means we might push queue up some syntax diagnostic tasks after semantic ones. I think a better way to model this might be having the DiagnosticsTaskKind::Syntax spawn the corresponding semantic one once its finished computing. That way we make sure to first do the syntax ones.
There was a problem hiding this comment.
it also spawns semantic and parser diagnostics tasks in lockstep which means we might push queue up some syntax diagnostic tasks after semantic ones.
That's terrible I couldn't think about it 😅
I think that with TaskPool::spawn_with_sender, I can publish two tasks in order within a single job
ShoyuVanilla
force-pushed
the
segregate-diags
branch
from
eec6375 to
eea1e9b
Compare
|
Since the syntax and the semantic diagnostics are always called in serial order, I think that it might be better to rollback the segregation |
|
Thanks! |
|
☀️ Test successful - checks-actions |
|
This seems to have broken diagnostics 😅 I don't get any native ones at all anymore |
| }) | ||
| { | ||
| // don't signal an update if the diagnostics are the same | ||
| return; |
There was a problem hiding this comment.
Oh, yes, indeed 🥲 terrible mistake. I'll fix this one or two hours later once I got home
fix: Native diagnostics not working
This should be a `continue` now
_Originally posted by `@Veykril` in #17775 (comment)
I've tested the release compile output with IDE in the original PR, but my test workspace had only one `.rs` file 🤦 😢
|
Alright this works great (aside from that one small issue 😄), syntax errors in |
fix: Native diagnostics not working
This should be a `continue` now
_Originally posted by `@Veykril` in rust-lang/rust-analyzer#17775 (comment)
I've tested the release compile output with IDE in the original PR, but my test workspace had only one `.rs` file 🤦 😢
Closes #17731