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

Fix OOM caused by term search #17203

Merged
merged 4 commits into from
May 13, 2024
Merged

Fix OOM caused by term search #17203

merged 4 commits into from
May 13, 2024

Conversation

kilpkonn
Copy link
Contributor

@kilpkonn kilpkonn commented May 8, 2024

The issue came from multi Cartesian product for exprs with many (25+) arguments, each having multiple options.
The solution is two fold:

Avoid blowing up in Cartesian product

Before the logic was:
1. Find expressions for each argument/param - there may be many
2. Take the Cartesian product (which blows up in some cases)
4. If there are more than 2 options throw them away by squashing them to Many
Now the logic is:
1. Find expressions for each argument/param and squash them to Many if there are more than 2 as otherwise we are guaranteed to also have more than 2 after taking the product which means squashing them anyway.
2. Take the Cartesian product on iterator
3. Start consuming it one by one
4. If there are more than 2 options throw them away by squashing them to Many (same as before)

This is also why I had to update some tests as the expressions get squashed to many more eagerly.

Use fuel to avoid long search times and high memory usage

Now all the tactics use should_continue: Fn() -> bool to chech if they should keep iterating (Similarly to chalk).
This reduces the search times by a magnitude, for example from ~139ms/hole to ~14ms/hole for ripgrep crate.
There are slightly less expressions found, but I think speed gain worth it for usability.
Also note that syntactic hits decreases more because of squashing so you simple need to run search multiple times to get full terms.
Also the worst case time (For example nalgebra crate cus it has tons of generics) has search times mostly under 200ms.

Benchmarks on ripgrep crate
Before:

Tail Expr syntactic hits: 291/1692 (17%)
Tail Exprs found: 1253/1692 (74%)
Term search avg time: 139ms

After:

Tail Expr syntactic hits: 239/1692 (14%)
Tail Exprs found: 1226/1692 (72%)
Term search avg time: 14ms

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2024
ex core::ops::Deref::deref(&t) (use core::ops::Deref) [type_could_unify]
ex core::ops::Deref::deref(&T(S)) (use core::ops::Deref) [type_could_unify]
lc m [local]
Copy link
Member

Choose a reason for hiding this comment

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

Are these results non-deterministic-ish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, will debug later why exactly the ordering changed.

Copy link
Member

Choose a reason for hiding this comment

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

Would be great if you could take a quick look at that, will merge this given the ordering change being possibly non-deterministic shouldn't be related to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked that collapsing terms before Cartesian product https://github.com/rust-lang/rust-analyzer/pull/17203/files#diff-5797341fbf7c8fcf87bd0bfd50c533bff963b96d1185fadbda31cfebc114fdbaR130-R135 causes the order to change. Since I think there is no preferred order I think the current order is find.
If you got any good suggestions for preferred order then I'm happy to change it - would like to switch from Vec to FxHashSet anyway for its uniqueness guarantees, but couldn't really do it as it would cause "random" order

@Veykril 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 May 13, 2024
@Veykril
Copy link
Member

Veykril commented May 13, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented May 13, 2024

📌 Commit ab18604 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 13, 2024

⌛ Testing commit ab18604 with merge 9db1258...

@bors
Copy link
Collaborator

bors commented May 13, 2024

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

1 similar comment
@bors
Copy link
Collaborator

bors commented May 13, 2024

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

@bors bors merged commit 9db1258 into rust-lang:master May 13, 2024
11 checks passed
@bors
Copy link
Collaborator

bors commented May 13, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants