Skip to content
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

refactor: foundations for LSP workspaces support #2589

Merged
merged 6 commits into from
Apr 25, 2024
Merged

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Apr 24, 2024

Summary

Related to #1573

This refactoring doesn't change any logic, but it does some internal refactoring to prepare the LSP to support multiple workspaces.

I created some data structures to help to query/mutate the workspaces.

To save the workspaces data, I used slotmap::DenseSlotmap. You can read more in the documentation.

The good about slotmap is that it emits a unique key every time we insert something; that key can be used later on to access the stored data. Those keys are always unique. Also, upon deletion, the slot that was used stays free, and it gets reused when a new element is inserted.

I chose DenseSlotmap because it's the fastest during iteration and slowest upon insertion. Inserting workspace folders will be a one-time operation or a rare operation. How many times does a user add a new project to a workspace? However, choosing the correct workspace will be an operation that we will have to do often. The LSP isn't able to signal if a file belongs to a certain workspace, so we have to guess it based on its path.

Test Plan

The current tests should pass

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project A-Formatter Area: formatter A-LSP Area: language server protocol L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Apr 24, 2024
@ematipico ematipico requested review from a team April 24, 2024 16:12
Copy link

codspeed-hq bot commented Apr 24, 2024

CodSpeed Performance Report

Merging #2589 will create unknown performance changes

Comparing feat/lsp-workspaces (dccb988) with main (dc7cb57)

Summary

🆕 93 new benchmarks

Benchmarks breakdown

Benchmark main feat/lsp-workspaces Change
🆕 analyzer[css.js] N/A 84.8 ms N/A
🆕 analyzer[index.js] N/A 186.8 ms N/A
🆕 analyzer[lint.ts] N/A 160.4 ms N/A
🆕 analyzer[parser.ts] N/A 299.3 ms N/A
🆕 analyzer[router.ts] N/A 84.5 ms N/A
🆕 analyzer[statement.ts] N/A 268.7 ms N/A
🆕 analyzer[typescript.ts] N/A 431.6 ms N/A
🆕 bootstrap.css[cached] N/A 257.6 ms N/A
🆕 bootstrap.css[uncached] N/A 283.2 ms N/A
🆕 bulma.css[cached] N/A 247.9 ms N/A
🆕 bulma.css[uncached] N/A 267.9 ms N/A
🆕 foundation.css[cached] N/A 165.2 ms N/A
🆕 foundation.css[uncached] N/A 178.6 ms N/A
🆕 full.css[cached] N/A 2.5 s N/A
🆕 full.css[uncached] N/A 2.7 s N/A
🆕 materialize.css[cached] N/A 180.8 ms N/A
🆕 materialize.css[uncached] N/A 202.2 ms N/A
🆕 pure.css[cached] N/A 21.6 ms N/A
🆕 pure.css[uncached] N/A 25.1 ms N/A
🆕 semantic.css[cached] N/A 753.7 ms N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

crates/biome_service/src/settings.rs Outdated Show resolved Hide resolved
crates/biome_service/src/settings.rs Outdated Show resolved Hide resolved
crates/biome_service/src/workspace.rs Show resolved Hide resolved
/// Retrieves the settings of the organize imports
pub fn organize_imports(&self) -> &OrganizeImportsSettings {
&self.organize_imports
pub fn get_settings_by_path(&self, path: &BiomePath) -> &Settings {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these two get_settings_by_path methods seem to be the only places where we iterate over the paths, and I don't the methods being used anywhere. Do we anticipate that they will be commonly used going forward?

I'm asking because the DenseSlotMap was chosen because of fast iteration, but at least as of yet we don't do any iteration yet. Based on the current state that would not make it a good choice, but that could change if these methods become the most commonly used ones in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove two functions and kept only one: get_settings_by_path.

As mentioned in the description, in the LSP there's way to retrieve the current project when handling a file. When the LSP sends us the request textDocument/didOpen, we only get its absolute URL.

In order to understand if a user opened a file that belongs to a different project from the previous, we need to inspect its path, and that's where the iteration will come into play.

If you come up with a better solution by the time I will open the next PR, that would be awesome.

@ematipico ematipico force-pushed the feat/lsp-workspaces branch 2 times, most recently from 9375835 to e876fc0 Compare April 25, 2024 08:39
@ematipico ematipico requested a review from arendjr April 25, 2024 08:39
@ematipico ematipico force-pushed the feat/lsp-workspaces branch from e876fc0 to 6e1ae1f Compare April 25, 2024 08:46
@ematipico ematipico force-pushed the feat/lsp-workspaces branch from 6e1ae1f to 830f8d8 Compare April 25, 2024 09:03
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ematipico ematipico force-pushed the feat/lsp-workspaces branch from ae8a723 to dccb988 Compare April 25, 2024 09:39
@ematipico ematipico merged commit dc7cb57 into main Apr 25, 2024
5 checks passed
@ematipico ematipico deleted the feat/lsp-workspaces branch April 25, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Core Area: core A-Formatter Area: formatter A-LSP Area: language server protocol A-Project Area: project L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants