feature: Create `UnindexedProject` notification to be sent to the client #15863

davidbarsky · 2023-11-09T21:01:06Z

(Note that this branch contains commits from #15830, which I'll rebase atop of as needed.)

Based on the discussion in #15837, I've added a notification and off-by-default toggle to send that notification from handle_did_open_text_document. I'm happy to rename/tweak this as needed.

I've been using this for a little bit, and it does seem to cause a little bit more indexing/work in rust-analyzer, but it's something that I'll profile as needed, I think.

davidbarsky · 2023-11-12T17:13:02Z

Did some profiling of this on my Mac and the profile below is generally representative:

   15ms - GlobalState::handle_event
       13ms - handle_did_open_text_document
           13ms - run_unindexed_project
               13ms - GlobalState::process_changes
                    1ms - RootDatabase::apply_change
                        1ms - RootDatabase::apply_change
                        0   - RootDatabase::request_cancellation (1 calls)
                        0   - ???
                   11ms - ???
                0   - relevant_crates (1 calls)
                0   - ???
            0   - ???
        0   - GlobalState::process_changes (1 calls)
        1ms - ???

13ms doesn't feel like the best, but is probably acceptable for an opt-in toggle.

davidbarsky · 2023-11-15T18:54:01Z

CI failed because of a timeout in receiving a notification, which doesn't feel great.

Veykril · 2023-11-15T19:22:55Z

Blocking in a notification handler is gonna end in a bad experience as notifications are handled on the main thread which means blocking there blocks processing anything else aside from thread pool tasks. It would probably make more sense to add a new Task kind for this and spawn the special handler on the task pool.

crates/rust-analyzer/src/handlers/notification.rs

davidbarsky · 2023-11-19T00:19:50Z

Moved over to a task-based approach. I'm not fully sure if this is the approach that you had in mind since GlobalState::process_changes isn't running on a different thread, but I guess no other caller of it really is either.

Veykril · 2023-11-21T14:01:13Z

crates/rust-analyzer/src/main_loop.rs

+                let _p = profile::span("run_unindexed_project");
+
+                tracing::debug!(changes = self.process_changes(), "processes changes");
+                let snap = self.snapshot();
+                let id =
+                    crate::lsp::from_proto::file_id(&snap, &uri).expect("Unable to get file ID");
+
+                if let Ok(crates) = &snap.analysis.crates_for(id) {
+                    if crates.is_empty() {
+                        tracing::debug!(?uri, "rust-analyzer does not track this file");
+                        self.send_notification::<ext::UnindexedProject>(
+                            ext::UnindexedProjectParams {
+                                text_documents: vec![lsp_types::TextDocumentIdentifier { uri }],
+                            },
+                        );
+                    } else {
+                        tracing::warn!("was unable to get analysis for crate")
+                    }
+                }


Now we are still processing it on the main thread :D All of this should be done in the spawned task on the thread pool, where as Task::FileIndexState should be used to return the result of the computation here (the snap.analysis.crates_for(id) result) and then have the task handler here just send the notification.

After chatting about this over DMs, I'm not going to take the suggested approach and make this noop task more substantial since there is no way to ensure that this code runs after GlobalState::process_changes() has been called without doing one of the following:

Add a retry mechanism for tasks, which would make this a de-facto spin loop. This might be okay!

Creating a mechanism of enqueuing a task, but deferring spawning them until after a turn of GlobalState::handle_event. This ensures that GlobalState::process_changes() has been called.

crates/rust-analyzer/src/main_loop.rs

crates/rust-analyzer/src/handlers/notification.rs

davidbarsky · 2023-11-21T23:17:57Z

Right, I made two sets of changes that I would be completely happy with backing out on:

I made a background task queue with a lightweight struct wrapper. Its only purpose is spawn a task after GlobalState::process_changes.
To avoid adding a regular Task whose only purpose is to send a notification to the client, I opted to make lsp_server::ReqQueue implement Clone and I wrapped in an Arc and call Arc::make_mut as appropriate. This was necessary because the spawned task would move self across threads.
- As an alternative, I can inline all two lines of GlobalState::send_notification into GlobalState::handle_queued_task and undo the changes made to lsp_server::ReqQueue and any subsequent knock-on changes. This means that if a QueuedTask ever grows an enum variant that needs to send a request, they might need to take the approach that I currently took with ReqQueue, but that day may never come—after all, the current approach to concurrency in rust-analyzer worked well until now.

davidbarsky · 2023-12-04T19:16:17Z

(Squashed commits; rebased against master. No meaningful change.)

davidbarsky · 2023-12-06T19:26:09Z

I've decided to back out of changes where I factored the GlobalState.sender and GlobalState.req_queue fields into a dedicated Conn struct alongside all the other, supporting changes (pervasive use of Arc::make_mut, cloneable fields in lsp_server). I've instead inlined the contents of GlobalState::send_notification into the event handler, which feels fine to me.

Anyways, I've had this PR deployed in my employer's buck2-based monorepo and it has driven down instance of rust-analyzer not running for folks dramatically.

bors · 2023-12-22T11:06:32Z

☔ The latest upstream changes (presumably #16182) made this pull request unmergeable. Please resolve the merge conflicts.

Veykril

There is a similar thing I added for whether proc-macros have changed in #16247, we might be able to switch that to this approach as well. Will need to check

editors/code/src/ctx.ts

davidbarsky · 2024-01-10T17:43:02Z

There is a similar thing I added for whether proc-macros have changed in #16247, we might be able to switch that to this approach as well. Will need to check

Having briefly looked over #16247, it's certainly a maybe. I considered using the request queue system, but that only really is able to handle a single event at a time. The task-based approach I did here can handle multiple events, which can happen with both FileDidOpenNotifications and proc macros being changed.

crates/rust-analyzer/src/global_state.rs

crates/rust-analyzer/src/main_loop.rs

bors · 2024-02-05T17:33:41Z

☔ The latest upstream changes (presumably #16470) made this pull request unmergeable. Please resolve the merge conflicts.

Veykril

clippy is complaining on CI

crates/rust-analyzer/src/global_state.rs

Veykril · 2024-02-08T09:45:46Z

@bors delegate+

bors · 2024-02-08T09:45:49Z

✌️ @davidbarsky, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

…tting.

davidbarsky · 2024-02-08T19:35:01Z

@bors r=@Veykril

bors · 2024-02-08T19:35:03Z

📌 Commit 6330b02 has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-02-08T19:36:11Z

⌛ Testing commit 6330b02 with merge 1370784...

bors · 2024-02-08T19:47:37Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 1370784 to master...

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2023

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from 51d2ed8 to 29852bb Compare November 9, 2023 21:11

Veykril reviewed Nov 15, 2023

View reviewed changes

crates/rust-analyzer/src/handlers/notification.rs Outdated Show resolved Hide resolved

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from 243d25e to cdb7fc7 Compare November 19, 2023 00:02

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from cdb7fc7 to 4391d0a Compare November 20, 2023 17:41

Veykril reviewed Nov 21, 2023

View reviewed changes

lnicola reviewed Nov 21, 2023

View reviewed changes

crates/rust-analyzer/src/main_loop.rs Outdated Show resolved Hide resolved

Veykril reviewed Nov 21, 2023

View reviewed changes

crates/rust-analyzer/src/handlers/notification.rs Outdated Show resolved Hide resolved

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2023

davidbarsky requested a review from Veykril November 21, 2023 15:59

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from 448eafb to 26236ea Compare November 21, 2023 23:14

davidbarsky mentioned this pull request Nov 29, 2023

Add an Optional CargoTargetSpec-like Data to rust-project.json #15892

Open

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch 2 times, most recently from 87f79ff to e568362 Compare December 4, 2023 19:15

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from e568362 to 89057db Compare December 6, 2023 19:09

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch 2 times, most recently from d33ad25 to aa7e33d Compare December 6, 2023 19:43

Veykril reviewed Jan 10, 2024

View reviewed changes

editors/code/src/ctx.ts Outdated Show resolved Hide resolved

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch 2 times, most recently from 73f8fad to 3202c35 Compare January 16, 2024 21:30

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from 3202c35 to c35478e Compare January 16, 2024 23:51

davidbarsky mentioned this pull request Jan 17, 2024

Smoother rust-project.json Generation #16377

Open

davidbarsky requested a review from Veykril January 17, 2024 19:26

Veykril reviewed Jan 18, 2024

View reviewed changes

crates/rust-analyzer/src/global_state.rs Outdated Show resolved Hide resolved

crates/rust-analyzer/src/main_loop.rs Outdated Show resolved Hide resolved

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch 2 times, most recently from ccdd0fc to b6155f0 Compare January 23, 2024 20:14

davidbarsky requested a review from Veykril January 26, 2024 23:22

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from b6155f0 to 79503ba Compare February 7, 2024 20:31

Veykril approved these changes Feb 8, 2024

View reviewed changes

crates/rust-analyzer/src/global_state.rs Outdated Show resolved Hide resolved

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from 79503ba to c3db016 Compare February 8, 2024 19:00

feature: Add a UnindexedProject notification and a corresponding se…

6330b02

…tting.

davidbarsky force-pushed the davidbarsky/start-of-monorepo-mode branch from c3db016 to 6330b02 Compare February 8, 2024 19:23

bors merged commit 1370784 into rust-lang:master Feb 8, 2024
11 checks passed

davidbarsky deleted the davidbarsky/start-of-monorepo-mode branch February 8, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Create `UnindexedProject` notification to be sent to the client #15863

feature: Create `UnindexedProject` notification to be sent to the client #15863

davidbarsky commented Nov 9, 2023

davidbarsky commented Nov 12, 2023 •

edited

davidbarsky commented Nov 15, 2023

Veykril commented Nov 15, 2023

davidbarsky commented Nov 19, 2023

Veykril Nov 21, 2023

davidbarsky Nov 21, 2023

davidbarsky commented Nov 21, 2023

davidbarsky commented Dec 4, 2023 •

edited

davidbarsky commented Dec 6, 2023

bors commented Dec 22, 2023

Veykril left a comment

davidbarsky commented Jan 10, 2024 •

edited

bors commented Feb 5, 2024

Veykril left a comment

Veykril commented Feb 8, 2024

bors commented Feb 8, 2024

davidbarsky commented Feb 8, 2024

bors commented Feb 8, 2024

bors commented Feb 8, 2024

bors commented Feb 8, 2024

feature: Create UnindexedProject notification to be sent to the client #15863

feature: Create UnindexedProject notification to be sent to the client #15863

Conversation

davidbarsky commented Nov 9, 2023

davidbarsky commented Nov 12, 2023 • edited

davidbarsky commented Nov 15, 2023

Veykril commented Nov 15, 2023

davidbarsky commented Nov 19, 2023

Veykril Nov 21, 2023

Choose a reason for hiding this comment

davidbarsky Nov 21, 2023

Choose a reason for hiding this comment

davidbarsky commented Nov 21, 2023

davidbarsky commented Dec 4, 2023 • edited

davidbarsky commented Dec 6, 2023

bors commented Dec 22, 2023

Veykril left a comment

Choose a reason for hiding this comment

davidbarsky commented Jan 10, 2024 • edited

bors commented Feb 5, 2024

Veykril left a comment

Choose a reason for hiding this comment

Veykril commented Feb 8, 2024

bors commented Feb 8, 2024

davidbarsky commented Feb 8, 2024

bors commented Feb 8, 2024

bors commented Feb 8, 2024

bors commented Feb 8, 2024

feature: Create `UnindexedProject` notification to be sent to the client #15863

feature: Create `UnindexedProject` notification to be sent to the client #15863

davidbarsky commented Nov 12, 2023 •

edited

davidbarsky commented Dec 4, 2023 •

edited

davidbarsky commented Jan 10, 2024 •

edited