Skip to content

Version 0.3.1995 rust-analyzer process spins out of control in macOS VSCode #17409

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

Closed
davidhewitt opened this issue Jun 12, 2024 · 13 comments
Closed
Labels
C-bug Category: bug

Comments

@davidhewitt
Copy link
Contributor

davidhewitt commented Jun 12, 2024

Followup of #17378 - as similar to #17378 (comment) I am also still experiencing the same issue of high CPU on version 0.3.1995 (and also on main at fa486e6).

Doing some debugging locally, I find that the infinite loop mentioned in #17378 (comment) seems to still the culprit.

I inserted a debug print:

$ git diff 
diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs
index e8504979b..546f1e49a 100644
--- a/crates/rust-analyzer/src/config.rs
+++ b/crates/rust-analyzer/src/config.rs
@@ -2532,6 +2532,7 @@ macro_rules! _impl_for_config_data {
                 $vis fn $field(&self, source_root: Option<SourceRootId>) -> &$ty {
                     let mut par: Option<SourceRootId> = source_root;
                     while let Some(source_root_id) = par {
+                        dbg!(source_root_id, &self.source_root_parent_map);
                         par = self.source_root_parent_map.get(&source_root_id).copied();
                         if let Some((config, _)) = self.ratoml_files.get(&source_root_id) {
                             if let Some(value) = config.$field.as_ref() {

... and this is my output:

[crates/rust-analyzer/src/config.rs:336:1] source_root_id = SourceRootId(
    200,
)
[crates/rust-analyzer/src/config.rs:336:1] &self.source_root_parent_map = {
    SourceRootId(
        202,
    ): SourceRootId(
        200,
    ),
    SourceRootId(
        200,
    ): SourceRootId(
        202,
    ),
    // elided for brevity
}
[crates/rust-analyzer/src/config.rs:336:1] source_root_id = SourceRootId(
    202,
)
[crates/rust-analyzer/src/config.rs:336:1] &self.source_root_parent_map = {
    SourceRootId(
        202,
    ): SourceRootId(
        200,
    ),
    SourceRootId(
        200,
    ): SourceRootId(
        202,
    ),
    // elided for brevity
}
// and repeat ad infinitum between source root 202 and 200

So I think the patch in #17381 is probably insufficient because here I don't have a source root with a parent of itself; instead somehow I have a cycle between two source roots 202 and 200.

I guess there's brute force solutions to prevent this infinite loop like bailing out if the number of iterations ever exceeds the total size of source_root_parent_map. Or maybe there's similar ideas to #17381 that can prune this graph.

... I would go further and submit a patch but I really need to go to sleep, sorry 🙈

cc @Veykril @roife

@davidhewitt davidhewitt added the C-bug Category: bug label Jun 12, 2024
@davidhewitt
Copy link
Contributor Author

It looks like @roife even predicted this case in #17381 (comment) ... but reading the PR thread, it's not clear to me if something was done to prevent that case. 🤔

@davidhewitt
Copy link
Contributor Author

I opened #17412 with one possible way to resolve this.

@roife
Copy link
Member

roife commented Jun 13, 2024

For the example in #17381 (comment):

rootA: ["/ROOT/a", "/ROOT/a/b/c/d"]
rootB: ["/ROOT/a/b"]

For rootA, we can just consider /ROOT/a.

However, for more complex situations, we might need to consider in graph, such as computing the dominator node as the common ancestor in the graph. (I haven't figured out yet if there will be such complex cases.)

@Veykril
Copy link
Member

Veykril commented Jun 13, 2024

#17412 papers over this but we should fix the map to not contain cycles

bors added a commit that referenced this issue Jun 13, 2024
fix: add a breaker to avoid infinite loops from source root cycles

See #17409

This patch prevents infinite looping from cycles by giving up if the number of source roots checked for a config value reaches the total number of source roots.

Alternative more precise options include creating a set of all source roots visited and giving up as soon as a cycle is encountered, but I wasn't sure how costly an allocation would be here for performance.

Can confirm that locally this fixes the problem for me.
@roife
Copy link
Member

roife commented Jun 14, 2024

Should we use SourceRoot as the location for ratoml? If there are ratoml files in two different paths within the same SourceRoot, how would we handle that?"

@roife
Copy link
Member

roife commented Jun 14, 2024

After reviewing project-model/workspace.rs, I identified three possible root paths: the code root directory (crates), out_dir, and extra_targets (see https://github.com/rust-lang/rust-analyzer/blob/master/crates/project-model/src/workspace.rs#L610-L625). Among these, out_dir is unlikely to cause circular references, as it resides under target. Therefore, the most probable cause of circular references is extra_targets.

However we may only take pkg_root into consideration🤔. I'll try to fix it.

@roife
Copy link
Member

roife commented Jun 15, 2024

Hi @davidhewitt, is the project you are working on open-source? A reproducible example of the bug would be really helpful 🙏

Alternatively, I want to know if a configuration like this exists in the project?

[lib] # or doc, etc.
path = "main.rs"

@davidhewitt
Copy link
Contributor Author

Yep all open source - the PyO3 repo :)

I like workspaces a lot, so I have a few related checkouts also included as workspace folders:

pyo3             # https://github.com/PyO3/pyo3
pythonize        # https://github.com/davidhewitt/pythonize
setuptools-rust  # https://github.com/PyO3/setuptools-rust
pyo3-scratch     # a simple lib project, just local directory which I put adjacent to the pyo3 checkout
pyo3-benches     # this is a separate workspace at the pyo3-benches subdirectory inside the PyO3 repository

@roife
Copy link
Member

roife commented Jun 18, 2024

I cannot reproduce this bug on my machine. It's a bit strange. 🤔

@davidhewitt
Copy link
Contributor Author

davidhewitt commented Jun 18, 2024

In case it helps, relevant settings:

// pyo3.code-workspace
{
	"folders": [
		{
			"path": "pyo3"
		},
		{
			"path": "pyo3-scratch"
		},
		{
			"path": "setuptools-rust"
		},
		{
			"path": "pyo3/pyo3-benches"
		},
		{
			"path": "pythonize"
		}
	],
	"settings": {
		"rust-analyzer.cargo.features": [
			"pyo3/full"
		],
		"rust-analyzer.interpret.tests": true,
		"rust-analyzer.testExplorer": true,
	}
}

... I don't think I've got any other settings which are relevant (I took a quick scan through my user settings).

@roife
Copy link
Member

roife commented Jun 18, 2024

Thanks, I am now able to reproduce it successfully.

@roife
Copy link
Member

roife commented Jun 19, 2024

I have figured out the cause of the issue:

pyo3/pyo3-benches use pyo3 as dependency, so r-a places pyo3 and pyo3/pyo3-benches/target/debug/build/pyo3-54b39ada054ab9a7/out in one SourceRoot; meanwhile, pyo3/pyo3-benches is in another SourceRoot. It results in the circle I described in #17381 (comment).

Maybe when calculating source_root_parent_map, we should consider only pkg_root and ignore out_dir and extra_targets (https://github.com/rust-lang/rust-analyzer/blob/crates/project-model/src/workspace.rs#L618).

@Veykril, could you provide some suggestions?

update:

An odd situation might still lead to the issue. Consider the following project structure: A/B/C. Assuming C uses A/a.rs as an extra target, so A/B/C/ and A/ are grouped into one SourceRoot. Meanwhile, project B/ is placed into a separate new SourceRoot. If we open both projects A, B, and C, it is still a circle.

@Veykril
Copy link
Member

Veykril commented Jun 19, 2024

We should just try to recognize cycles and discard the cycle inducing edge. The source root system isn't ideal for this lookup in general but its the best we have right now hence why we use it.

bors added a commit that referenced this issue Jun 20, 2024
fix: ensure there are no cycles in the source_root_parent_map

See #17409

We can view the connections between roots as a graph. The problem is that this graph may contain cycles, so when adding edges, it is necessary to check whether it will lead to a cycle.

Since we ensure that each node has at most one outgoing edge (because each SourceRoot can have only one parent), we can use a disjoint-set to maintain the connectivity between nodes. If an edge’s two nodes belong to the same set, they are already connected.

Additionally, this PR includes the following three changes:

1. Removed the workaround from #17409.
2. Added an optimization: If `map.contains_key(&SourceRootId(*root_id as u32))`, we can skip the current loop iteration since we have already found its parent.
3. Modified the inner loop to iterate in reverse order with `roots[..idx].iter().rev()` at line 319. This ensures that if we are looking for the parent of `a/b/c`, and both `a` and `a/b` meet the criteria, we will choose the longer match (`a/b`).
@roife roife closed this as completed Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

3 participants