-
-
Notifications
You must be signed in to change notification settings - Fork 792
perf(lsp): scan project based on ScanKind
#6099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| let execution = Execution::new_format(VcsTargeted { | ||
| staged: false, | ||
| changed: false, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this change, because it felt weird that project_key needed to be part of Execution.
| pub fn parse_html_with_cache(source: &str, cache: &mut NodeCache) -> HtmlParse { | ||
| tracing::debug_span!("Parsing phase").in_scope(move || { | ||
| let mut parser = HtmlParser::new(source); | ||
| let mut parser = HtmlParser::new(source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these logs have been removed because they cause a lot of noise
| "biome_did_change_workspace_settings", | ||
| "workspace/didChangeWatchedFiles", | ||
| if let Some(base_path) = self.session.base_path() { | ||
| if let Some(folders) = self.session.get_workspace_folders() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we weren't considering workspace folders when the configuration changed. This should fix some bugs.
arendjr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
| watch: true, | ||
| force: false, | ||
| scan_kind: ScanKind::Project, | ||
| scan_kind, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, if I understand correctly this is the main reason for this PR, right? The LSP no longer always needs to use ScanKind::Project, but it can use an actual scan_kind determined by the workspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! If you open a project that doesn't use project rules, the memory used by the biome process (daemon) will still be low because we don't crawl and save all files.
9d7468a to
2dcd162
Compare
CodSpeed Performance ReportMerging #6099 will not alter performanceComparing Summary
|
51f69bd to
fc449d5
Compare
Summary
This PR optimises the LSP scanner by computing the
ScanKindvariant when we create a project. The business logic we used in the CLI has been moved inside the Workspace. When we create a project, the Workspace also tells us how to scan it. This is very useful and functional. This is very convenient because it will allow us to tweak the LSP scanner by taking LSP settings into account.To implement this functionality, the method
open_projectnow accepts theskipandonlyparameters because they need to be factored into account when deciding how to scan a project. Of course, the LSP doesn't have any of those yet.Additionally, the CLI adds some further checks, which are the same as before. That's why the method
comput_scan_kindnow returns anOption, so we canunwrap_orand use one computed from the workspace.The rest of the changes are mostly code moved around, and unit tests updated.
Test Plan
CI should stay green, snapshots shouldn't change.