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

Removed use of iteration through a HashMap/HashSet in rustc_incremental and replaced with IndexMap/IndexSet #110040

Merged
merged 11 commits into from
Jun 8, 2023

Conversation

ndrewxie
Copy link
Contributor

@ndrewxie ndrewxie commented Apr 7, 2023

This allows for the #[allow(rustc::potential_query_instability)] in rustc_incremental to be removed, moving towards fixing #84447 (although a LOT more modules have to be changed to fully resolve it). Only HashMaps/HashSets that are being iterated through have been modified (although many structs and traits outside of rustc_incremental had to be modified as well, as they had fields/methods that involved a HashMap/HashSet that would be iterated through)

I'm making a PR for just 1 module changed to test for performance regressions and such, for future changes I'll either edit this PR to reflect additional modules being converted, or batch multiple modules of changes together and make a PR for each group of modules.

@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 Apr 7, 2023
@ndrewxie
Copy link
Contributor Author

ndrewxie commented Apr 7, 2023

Aah it appears some tests are failing, ig I didn't hit all the test cases on my local machine while testing (more specifically, the cranelift and gcc backend). I'll make a few edits, then retry tomorrow

@rust-log-analyzer

This comment has been minimized.

@cuviper
Copy link
Member

cuviper commented Apr 7, 2023

Note that IndexMap iterator order is only deterministic with respect to the insertion order, so you must also ensure that all insertions and removals are also deterministic to get the kind of stability that rustc wants. Or you can take steps like sorting the map after that's done, so iteration will follow the sort order.

(I'm not saying this PR is necessarily a problem in that way, just noting it for caution.)

@ndrewxie
Copy link
Contributor Author

ndrewxie commented Apr 7, 2023

Aah, I see. After I fix the issue with cranelift build failing, I'll go investigate that, and keep it in mind for future changes as well :)

@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@ndrewxie
Copy link
Contributor Author

ndrewxie commented Apr 7, 2023

Also just a quick question - I failed the build test for codegen_cranelift and codegen_gcc because the tests I was doing on my local machine never hit those modules. Is there a way I can test those as well?

For codegen_cranelift, I found using --target=i686-pc-windows-gnu worked, but is there an equivalent target I can use to make sure my code doesn't break codegen_gcc?

Thanks!

@bjorn3
Copy link
Member

bjorn3 commented Apr 7, 2023

./x.py check should check both cg_clif and cg_gcc. ./x.py build will only build them if they are listed in the codegen-backends option of config.toml.

@ndrewxie
Copy link
Contributor Author

ndrewxie commented Apr 7, 2023

./x.py check should check both cg_clif and cg_gcc. ./x.py build will only build them if they are listed in the codegen-backends option of config.toml.

ohhhh I see, I haven't really been using ./x.py check... maybe I should start doing so, my tabletop jet engine laptop would certainly thank me

@Noratrieb
Copy link
Member

@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 Apr 7, 2023
@bors
Copy link
Contributor

bors commented Apr 7, 2023

⌛ Trying commit 916c1ca501f3e134b8cd8e61201d5a01a9b19d12 with merge e709771db4bde2ba3203fc1eb0b315b71b35c8c6...

@bors
Copy link
Contributor

bors commented Apr 8, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e709771db4bde2ba3203fc1eb0b315b71b35c8c6): comparison URL.

Overall result: no relevant changes - no action needed

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.

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

Instruction count

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

Max RSS (memory usage)

Results

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.7% [2.5%, 2.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 8, 2023
@lcnr
Copy link
Contributor

lcnr commented Apr 12, 2023

with rust-lang/compiler-team#533 we should be able to instead use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/unord/struct.UnordMap.html for a lot of these i think? 🤔

not sure what's the current status here but it feels preferable to use these whenever possible, cc @michaelwoerister

@ndrewxie
Copy link
Contributor Author

with rust-lang/compiler-team#533 we should be able to instead use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/unord/struct.UnordMap.html for a lot of these i think? 🤔

Ooo, that's a pretty cool data structure, didn't know that existed! Yeah, I guess that provides an even better determinism guarantee than FxIndexMap, because we either have to work with it in an order-agnostic way, or sort it. IDK if it'll cause performance regressions from extra .sorts() (in places where iteration by Fn instead of FnMut isn't sufficient), but I'll be happy to migrate over (whenever possible) and run those performance tests if michaelwoerister confirms that it's indeed the best practice :)

@michaelwoerister
Copy link
Member

IndexMap and UnordMap can both make things deterministic but have slightly different properties:

  • IndexMap makes sure that iteration order matches insertion order. That means that one has to be careful to insert things in a deterministic way. E.g. the result of some_regular_hashmap.iter().collect::<IndexMap<_, _>>() will still not be deterministic, and it's not always easy to see if insertion order is deterministic.
  • UnordMap "erases" the iteration order. It is safe to feed things into it in any order one wants. It's API allows to do iteration-order-independent operations because they are safe to do (and often that's all that's needed). It also allows to extract its items into an ordered collection, but only in a deterministic way. However, this can be expensive operation since we have to sort the items in a way that is stable across compilation sessions.

I've been trying to use UnordMap where possible because it is a bit more robust (i.e. it introduces a kind of indeterminism firewall). But for maps where it's trivially obvious that they are built deterministically, IndexMap is a good choice too.

@lcnr
Copy link
Contributor

lcnr commented Apr 24, 2023

@ndrewxie do you want to try to move some of these uses to UnordMap? please retag this to S-waiting-to-review whenever you're happy. I think it's fine to use IndexMap in places where it's not stricty necessary but would prefer to replace as much as possible with UnordMap

@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 Apr 24, 2023
@ndrewxie
Copy link
Contributor Author

Alright, will do 👍

@rust-log-analyzer

This comment has been minimized.

@ndrewxie ndrewxie force-pushed the issue-84447-partial-1 branch from 69ba7dc to f5f638c Compare June 5, 2023 04:17
@ndrewxie
Copy link
Contributor Author

ndrewxie commented Jun 5, 2023

@rustbot label -S-waiting-on-author +S-waiting-on-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 Jun 5, 2023
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

I have to admit that the bool argument to into_sorted_stable_ord is very confusing to me and even after looking it up in the documentation I always have to wait a second and think about whether it is correct.

I would like us to either add a const CAN_USE_UNSTABLE_SORT: bool to StableOrd or alternatively always use a stable sort and only add unstable sorting where it is actually performance critical.

Please either do that in this PR or I will open a github issue once your PR lands.

Other thoughts: UnordItems using into() via From instead of having a method called collect is also confusing imo.

after dealing with review comments:

r=me,michaelwoerister

compiler/rustc_incremental/src/persist/fs.rs Outdated Show resolved Hide resolved
compiler/rustc_incremental/src/persist/fs.rs Outdated Show resolved Hide resolved
compiler/rustc_incremental/src/persist/fs.rs Outdated Show resolved Hide resolved
Comment on lines 51 to 53
.items()
.map(|(_, path)| path.as_str())
.into_sorted_stable_ord(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

&String should implement StableOrd

Copy link
Contributor Author

@ndrewxie ndrewxie Jun 8, 2023

Choose a reason for hiding this comment

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

Hmm, apparently not. Would it be correct to impl StableOrd for &T if T is StableOrd? I believe it should, but just checking in (if not, then I'll just add the impl for &String on its own)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be correct to impl StableOrd for &T if T is StableOrd?

yes

compiler/rustc_interface/src/queries.rs Outdated Show resolved Hide resolved
@lcnr lcnr 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 Jun 6, 2023
@michaelwoerister
Copy link
Member

I would like us to either add a const CAN_USE_UNSTABLE_SORT: bool to StableOrd ...

@lcnr, that sounds like a great idea! If you open an issue, feel free to assign it to me.

… few misc issues, added collect to UnordItems
@rustbot
Copy link
Collaborator

rustbot commented Jun 8, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

…fixed a few misc issues, added collect to UnordItems
@ndrewxie
Copy link
Contributor Author

ndrewxie commented Jun 8, 2023

Alright, I applied the review suggestions and switched UnordItems::into_sorted_stable_ord to use CAN_USE_UNSTABLE_SORT, and added collect to UnordItems

@rustbot label -S-waiting-on-author +S-waiting-on-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 Jun 8, 2023
@rust-log-analyzer

This comment has been minimized.

@michaelwoerister
Copy link
Member

Thanks, @ndrewxie -- looks good!

@bors r=lcnr,michaelwoerister

@bors
Copy link
Contributor

bors commented Jun 8, 2023

📌 Commit 3f324a8 has been approved by lcnr,michaelwoerister

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2023
@bors
Copy link
Contributor

bors commented Jun 8, 2023

⌛ Testing commit 3f324a8 with merge a0df04c...

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

a few nits for code which can be cleaned up now. Feel free to do them in a separate PR once this has landed

@bors
Copy link
Contributor

bors commented Jun 8, 2023

☀️ Test successful - checks-actions
Approved by: lcnr,michaelwoerister
Pushing a0df04c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 8, 2023
@bors bors merged commit a0df04c into rust-lang:master Jun 8, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 8, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a0df04c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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% [0.5%, 3.5%] 2
Regressions ❌
(secondary)
1.3% [1.3%, 1.3%] 1
Improvements ✅
(primary)
-3.3% [-6.1%, -0.6%] 2
Improvements ✅
(secondary)
-2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) -0.6% [-6.1%, 3.5%] 4

Cycles

Results

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)
- - 0
Improvements ✅
(primary)
-2.9% [-4.2%, -1.0%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.9% [-4.2%, -1.0%] 7

Binary size

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

Bootstrap: 647.603s -> 648.385s (0.12%)

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 21, 2023
…compiler-errors

Removed unnecessary &String -> &str, now that &String implements StableOrd as well

Applied a few nits suggested by lcnr to PR rust-lang#110040 (nits can be found [here](rust-lang#110040 (review)).)

Making a new PR because the old one was already merged, and given that this just applies changes that were already suggested, reviewing it should be fairly open-and-shut.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.