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

Handle rustc_resolve cases of rustc::potential_query_instability lint #131213

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ismailarilik
Copy link
Contributor

This PR removes #![allow(rustc::potential_query_instability)] line from compiler/rustc_resolve/src/lib.rs and converts FxHash{Map,Set} types into FxIndex{Map,Set} to suppress lint errors.

A somewhat tracking issue: #84447

r? @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2024
@compiler-errors
Copy link
Member

r? petrochenkov may have more opinions about this given the crate in question

@ismailarilik ismailarilik marked this pull request as ready for review October 4, 2024 04:42
@cjgillot
Copy link
Contributor

cjgillot commented Oct 5, 2024

  1. Could you point to where all these maps are iterated over?
  2. shift_remove is a perf footgun, please use swap_remove instead.

@ismailarilik
Copy link
Contributor Author

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:1090:63
     |
1090 |                     suggestions.extend(this.macro_use_prelude.iter().filter_map(
     |                                                               ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale
     = note: `-D rustc::potential-query-instability` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(rustc::potential_query_instability)]`

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:1108:60
     |
1108 |                     suggestions.extend(this.extern_prelude.iter().filter_map(|(ident, _)| {
     |                                                            ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_keys` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:1366:54
     |
1366 |             for ident in self.extern_prelude.clone().into_keys() {
     |                                                      ^^^^^^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:1471:47
     |
1471 |         let unused_macro = self.unused_macros.iter().find_map(|(def_id, (_, unused_ident))| {
     |                                               ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `keys` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:1960:14
     |
1960 |             .keys()
     |              ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:1964:22
     |
1964 |                     .iter()
     |                      ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `keys` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:2347:33
     |
2347 |             self.extern_prelude.keys().map(|ident| ident.name).collect::<Vec<_>>();
     |                                 ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_iter` can result in unstable query results
    --> compiler/rustc_resolve/src/diagnostics.rs:2847:22
     |
2847 |                     .into_iter();
     |                      ^^^^^^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_iter` can result in unstable query results
   --> compiler/rustc_resolve/src/ident.rs:989:30
    |
989 |         for single_import in &resolution.single_imports {
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late.rs:2263:50
     |
2263 |                 let mut distinct_iter = distinct.into_iter();
     |                                                  ^^^^^^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `keys` can result in unstable query results
    --> compiler/rustc_resolve/src/late.rs:2799:53
     |
2799 |                         .extend(parent_rib.bindings.keys().map(|ident| (*ident, ident.span)));
     |                                                     ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late.rs:5048:79
     |
5048 |         for (id, span) in late_resolution_visitor.diag_metadata.unused_labels.iter() {
     |                                                                               ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_iter` can result in unstable query results
   --> compiler/rustc_resolve/src/late/diagnostics.rs:763:34
    |
763 |             for (ident, &res) in &rib.bindings {
    |                                  ^^^^^^^^^^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_iter` can result in unstable query results
   --> compiler/rustc_resolve/src/late/diagnostics.rs:945:51
    |
945 |                     for (label_ident, node_id) in &label_rib.bindings {
    |                                                   ^^^^^^^^^^^^^^^^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late/diagnostics.rs:1918:14
     |
1918 |             .iter()
     |              ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late/diagnostics.rs:2127:38
     |
2127 |                 for (ident, &res) in &rib.bindings {
     |                                      ^^^^^^^^^^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late/diagnostics.rs:2153:57
     |
2153 | ...                   names.extend(extern_prelude.iter().flat_map(|(ident, _)| {
     |                                                   ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late/diagnostics.rs:2590:14
     |
2590 |             .iter()
     |              ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
    --> compiler/rustc_resolve/src/late/diagnostics.rs:2599:43
     |
2599 |             let (ident, _) = rib.bindings.iter().find(|(ident, _)| ident.name == symbol).unwrap();
     |                                           ^^^^
     |
     = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
   --> compiler/rustc_resolve/src/macros.rs:347:58
    |
347 |         for (_, &(node_id, ident)) in self.unused_macros.iter() {
    |                                                          ^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `iter` can result in unstable query results
   --> compiler/rustc_resolve/src/macros.rs:355:80
    |
355 |         for (&(def_id, arm_i), &(ident, rule_span)) in self.unused_macro_rules.iter() {
    |                                                                                ^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `into_values` can result in unstable query results
   --> compiler/rustc_resolve/src/rustdoc.rs:413:62
    |
413 |     let doc = prepare_to_doc_link_resolution(&doc_fragments).into_values().next().unwrap();
    |                                                              ^^^^^^^^^^^
    |
    = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

@ismailarilik
Copy link
Contributor Author

ismailarilik commented Oct 5, 2024

shift_remove is a perf footgun, please use swap_remove instead.

Yeah, it means O(1) instead of O(n) but doesn't preserve the relative order.

@bors

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 22, 2024

There are many places in which these maps are iterated, I'll try to go through all of them.

  • single_imports - iterated by early identifier resolution to find the matching import.
    I cannot prove that it's order independent, especially in presence of errors, better to convert it to IndexSet.
    Ok to use shift_remove, the sets are very small and it's safer to preserve the order (imports will be in source order).
  • macro_use_prelude - iterated by typo suggestion search, the suggestions are deterministically sorted later, so no need to convert to IndexSet

@petrochenkov
Copy link
Contributor

  • extern_crate_map - the iteration in diagnostics is accumulated into a single bool with .any(), no need to convert to index map
  • accessible_path_strings - iteration is used as a weird way to check for set.len() == 1, no need to convert to index set
  • unused_labels - iterated to report all unused labels, all labels are inserted first and then removed if used.
    Better sort by node id or span just before reporting instead of converting to index map.
  • let distinct: FxHashSet<_> = candidates ... - again a weird way to check set.len() == 1 with an iterator.
  • extern_prelude - iterated in diagnostics to collect suggestions, which are then predictably sorted later
  • module_map - again, iterated in diagnostics to collect suggestions, which are then predictably sorted later
  • unused_macros - same as unused_labels
  • unused_macro_rules - similar to unused_macros, but no removals, so can convert to index map to report in source order
  • IdentMap (bindings) - iterated for suggestions in a few places in diagnostics, not sure whether everything is properly sorted there.
    Surprisingly there are also removals from this map, not sure what is the best course of action here.
    (Also, could you remove that useless IdentMap alias?)

@petrochenkov
Copy link
Contributor

@ismailarilik
Could you rebase the PR? I'll run benchmarks.
@rustbot author

@rustbot rustbot 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 Oct 23, 2024
@ismailarilik ismailarilik force-pushed the handle-potential-query-instability-lint-for-rustc-resolve branch from 37b12b1 to fb9cc98 Compare October 24, 2024 05:43
@ismailarilik
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2024
@petrochenkov
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 24, 2024
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2024
…instability-lint-for-rustc-resolve, r=<try>

Handle `rustc_resolve` cases of `rustc::potential_query_instability` lint

This PR removes `#![allow(rustc::potential_query_instability)]` line from [`compiler/rustc_resolve/src/lib.rs`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_resolve/src/lib.rs#L12) and converts `FxHash{Map,Set}` types into `FxIndex{Map,Set}` to suppress lint errors.

A somewhat tracking issue: rust-lang#84447

r? `@compiler-errors`
@bors
Copy link
Contributor

bors commented Oct 24, 2024

⌛ Trying commit fb9cc98 with merge d65eab5...

@bors
Copy link
Contributor

bors commented Oct 24, 2024

☀️ Try build successful - checks-actions
Build commit: d65eab5 (d65eab5107d105fd9626d08d285e90c3d3317acc)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d65eab5): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
0.2% [0.1%, 0.4%] 26
Regressions ❌
(secondary)
0.9% [0.1%, 2.1%] 21
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.6% [-0.6%, -0.6%] 1
All ❌✅ (primary) 0.2% [0.1%, 0.4%] 26

Max RSS (memory usage)

Results (primary 0.4%, secondary 0.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [0.5%, 4.9%] 5
Regressions ❌
(secondary)
5.6% [1.6%, 13.0%] 9
Improvements ✅
(primary)
-1.9% [-2.9%, -0.6%] 3
Improvements ✅
(secondary)
-3.4% [-8.1%, -1.9%] 13
All ❌✅ (primary) 0.4% [-2.9%, 4.9%] 8

Cycles

Results (primary 2.0%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)
2.3% [2.1%, 2.5%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.0% [2.0%, 2.0%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 779.097s -> 779.212s (0.01%)
Artifact size: 333.61 MiB -> 333.61 MiB (0.00%)

compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

I had some technical difficulties to do that, sorry; I am probably not qualified enough for it. =/

cargo install rustup-toolchain-install-master

, then

cargo build --release

./target/release/collector profile_local cachegrind \
    +66701c42263042f7120385725606edeb987ad4f1 \
    --rustc2 +4a889a956e4c0269e73e3876ecb652fc844b5bb6 \
    --include unused-warnings \
    --profiles Check \
    --scenarios Full

in a clone of https://github.com/rust-lang/rustc-perf should work, assuming a typical linux distro like Ubuntu.

(But I already did that, see the comments above.)

@ismailarilik ismailarilik force-pushed the handle-potential-query-instability-lint-for-rustc-resolve branch from 818ee0a to 0ecb033 Compare November 3, 2024 13:17
@petrochenkov
Copy link
Contributor

I reverted it and tried to sort it

That's likely going to be even slower, and we probably shouldn't do something like span sorting in non-diagnostic code.
Let's use swap_remove instead, preserving the original ordering shouldn't be very important in those cases.

@petrochenkov petrochenkov 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 3, 2024
@ismailarilik
Copy link
Contributor Author

I reverted it and tried to sort it

That's likely going to be even slower, and we probably shouldn't do something like span sorting in non-diagnostic code. Let's use swap_remove instead, preserving the original ordering shouldn't be very important in those cases.

Done: 2ff01e8

@ismailarilik
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2024
@petrochenkov
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 9, 2024
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
…instability-lint-for-rustc-resolve, r=<try>

Handle `rustc_resolve` cases of `rustc::potential_query_instability` lint

This PR removes `#![allow(rustc::potential_query_instability)]` line from [`compiler/rustc_resolve/src/lib.rs`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_resolve/src/lib.rs#L12) and converts `FxHash{Map,Set}` types into `FxIndex{Map,Set}` to suppress lint errors.

A somewhat tracking issue: rust-lang#84447

r? `@compiler-errors`
@bors
Copy link
Contributor

bors commented Nov 9, 2024

⌛ Trying commit 0944d8c with merge 9e5ef87...

@bors
Copy link
Contributor

bors commented Nov 9, 2024

☀️ Try build successful - checks-actions
Build commit: 9e5ef87 (9e5ef87b6df4a60deb1ad010d026d72f46c5f4e7)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9e5ef87): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 0.3%] 4
Regressions ❌
(secondary)
0.8% [0.1%, 2.0%] 21
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.4% [-0.6%, -0.2%] 3
All ❌✅ (primary) 0.1% [0.0%, 0.3%] 4

Max RSS (memory usage)

Results (primary 1.6%, secondary 1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.6% [0.8%, 2.3%] 2
Regressions ❌
(secondary)
6.1% [1.8%, 14.0%] 8
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.7% [-4.7%, -2.9%] 6
All ❌✅ (primary) 1.6% [0.8%, 2.3%] 2

Cycles

Results (secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.3% [1.7%, 2.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.559s -> 781.555s (0.13%)
Artifact size: 335.38 MiB -> 335.40 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 9, 2024
@petrochenkov
Copy link
Contributor

Could you submit the part of this PR that just adds #[allow]s and comments (FIXME comments if the allow is not yet justified), without actual changes in logic?
I'll deal with the remaining changes later.

(Oh, and type IdentMap can be removed in that PR too.)
@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 10, 2024
@ismailarilik
Copy link
Contributor Author

ismailarilik commented Nov 11, 2024

Could you submit the part of this PR that just adds #[allow]s and comments (FIXME comments if the allow is not yet justified), without actual changes in logic? I'll deal with the remaining changes later.

(Oh, and type IdentMap can be removed in that PR too.) @rustbot author

I think working from this PR is better since there is a somewhat history here. Feel free to close it and work on it later. Sorry for being unqualified for things to do in this PR.

BTW, I already removed the type IdentMap.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants