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.

Sign up for GitHub

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

Jump to bottom

A few missed const tweaks #17927

Merged
merged 2 commits into from
Oct 14, 2014
Merged

A few missed const tweaks #17927

merged 2 commits into from
Oct 14, 2014

Conversation

alexcrichton
Copy link
Member

  • Apply the dead code lint to constants
  • Flag all hidden/injected statics as unnamed_addr
  • Remove a hack in rustc added when consts were added, turns out it's not necessary!

@bstrie
Copy link
Contributor

bstrie commented Oct 10, 2014

@thestinger, does this address the issues you were talking about on IRC this morning?

@alexcrichton
rustc: Warn about dead constants
18e4129 
A few catch-all blocks ended up not having this case for constants.

Closes rust-lang#17925
@alexcrichton
rustc: Remove the dummy hack from check_match
c56c9fc 
Turns out you can create &'static T quite easily in a constant, I just forgot
about this!
@alexcrichton
Copy link
Member Author

Apparently unnamed addresses causes obscure errors on OSX, but not on linux, which I have been unable to track down. In the meantime I'm going to land the other fixes.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 13, 2014
@alexcrichton
rollup merge of rust-lang#17927 : alexcrichton/more-const
412f4d1
@bors bors merged commit c56c9fc into rust-lang:master Oct 14, 2014
@alexcrichton alexcrichton deleted the more-const branch November 20, 2014 03:18
lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 29, 2024
@ChayimFriedman2
Add cov_marks to test rust-lang#17927
a44152a
lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 29, 2024
@bors
Auto merge of rust-lang#17927 - ChayimFriedman2:speedup-new-usages, r…
39cc5b6 
…=Veykril

perf: Speed up search for short associated functions, especially very common identifiers such as `new`

`@Veykril` said in rust-lang/rust-analyzer#17908 (comment) that people complain searches for `new()` are slow (they are right), so here I am to help!

The search is used by IDE features such as rename and find all references.

The search is slow because we need to verify each candidate, and that requires analyzing it; the key to speeding it up is to avoid the analysis where possible.

I did that with a bunch of tricks that exploits knowledge about the language and its possibilities. The first key insight is that associated methods may only be referenced in the form `ContainerName::func_name` (parentheses are not necessary!) (Rust doesn't include a way to `use Container::func_name`, and even if it will in the future most usages are likely to stay in that form.

Searching for `::` will help only a bit, but searching for `Container` can help considerably, since it is very rare that there will be two identical instances of both a container and a method of it.

However, things are not as simple as they sound. In Rust a container can be aliased in multiple ways, and even aliased from different files/modules. If we will try to resolve the alias, we will lose any gain from the textual search (although very common method names such as `new` will still benefit, most will suffer because there are more instances of a container name than its associated item).

This is where the key trick enters the picture. The key insight is that there is still a textual property: a container namer cannot be aliased, unless its name is mentioned in the alias declaration, or a name of alias of it is mentioned in the alias declaration.

This becomes a fixpoint algorithm: we expand our list of aliases as we collect more and more (possible) aliases, until we eventually reach a fixpoint. A fixpoint is not guaranteed (and we do have guards for the rare cases where it does not happen), but it is almost so: most types have very few aliases, if at all.

We do use some semantic information while analyzing aliases. It's a balance: too much semantic analysis, and the search will become slow. But too few of it, and we will bring many incorrect aliases to our list, and risk it expands and expands and never reach a fixpoint. At the end, based on benchmarks, it seems worth to do a lot to avoid adding an alias (but not too much), while it is worth to do a lot to avoid the need to semantically analyze func_name matches (but again, not too much).

After we collected our list of aliases, we filter matches based on this list. Only if a match can be real, we do semantic analysis for it.

The results are promising: searching for all references on `new()` in `base-db` in the rust-analyzer repository, which previously took around 60 seconds, now takes as least as two seconds and a half (roughly), while searching for `Vec::new()`, almost an upper bound to how much a symbol can be used, that used to take 7-9 minutes(!) now completes in 100-120 seconds, and with less than half of non-verified results (aka. false positives).

This is the less strictly correct (but faster) branch of this patch; it can miss some (rare) cases (there is a test for that - `goto_ref_on_short_associated_function_complicated_type_magic_can_confuse_our_logic()`). There is another branch that have no false negatives but is slower to search (`Vec::new()` never reaches a fixpoint in aliases collection there). I believe it is possible to create a strategy that will have the best of both worlds, but it will involve significant complexity and I didn't bother, especially considering that in the vast majority of the searches the other branch will be more than enough. But all in all, I decided to bring this branch (of course if the maintainers will agree), since our search is already not 100% accurate (it misses macros), and I believe there is value in the additional perf.

You can find the strict branch at https://github.com/ChayimFriedman2/rust-analyzer/tree/speedup-new-usages-strict.

Should fix rust-lang#7404, I guess (will check now).
lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 29, 2024
@bors
Auto merge of rust-lang#17955 - ChayimFriedman2:fix-fast-search-with-…
cba00a8 
…scope, r=Veykril

fix: Don't enable the search fast path for short associated functions when a search scope is set

In most places where we set a search scope it is a single file, and so the fast path will actually harm performance, since it has to search for aliases in the whole project. The only exception that qualifies for the fast path is SSR (there is an exception that don't qualify for the fast path as it search for `use` items). It sets the search scope to avoid dependencies. We could make it use the fast path, but I didn't bother.

I forgot this while working on rust-lang#17927.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexcrichton @bstrie @bors