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

Remove NamedVarMap. #136270

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Remove NamedVarMap. #136270

merged 1 commit into from
Jan 31, 2025
+37 −72

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jan 30, 2025 •
edited
Loading

NamedVarMap is extremely similar to ResolveBoundVars. The former contains two UnordMap<ItemLocalId, T> fields (obscured behind ItemLocalMap typedefs). The latter contains two
SortedMap<ItemLocalId, T> fields. We construct a NamedVarMap and then convert it into a ResolveBoundVars by sorting the UnordMaps, which is unnecessary busywork.

This commit removes NamedVarMap and constructs a ResolveBoundVars directly. SortedMap and NamedVarMap have slightly different perf characteristics during construction (e.g. speed of insertion) but this code isn't hot enough for that to matter.

A few details to note.

  • A FIXME comment is removed.
  • The detailed comments on the fields of NamedVarMap are copied to ResolveBoundVars (which has a single, incorrect comment).
  • BoundVarContext::map is renamed.
  • ResolveBoundVars gets a derived Default impl.

r? @jackh726

@nnethercote
Remove NamedVarMap.
022c0ce 
`NamedVarMap` is extremely similar to `ResolveBoundVars`. The former
contains two `UnordMap<ItemLocalId, T>` fields (obscured behind
`ItemLocalMap` typedefs). The latter contains two
`SortedMap<ItemLocalId, T>` fields. We construct a `NamedVarMap` and
then convert it into a `ResolveBoundVars` by sorting the `UnordMap`s,
which is unnecessary busywork.

This commit removes `NamedVarMap` and constructs a `ResolveBoundVars`
directly. `SortedMap` and `NamedVarMap` have slightly different
perf characteristics during construction (e.g. speed of insertion) but
this code isn't hot enough for that to matter.

A few details to note.
- A `FIXME` comment is removed.
- The detailed comments on the fields of `NamedVarMap` are copied to
  `ResolveBoundVars` (which has a single, incorrect comment).
- `BoundVarContext::map` is renamed.
- `ResolveBoundVars` gets a derived `Default` impl.
@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 Jan 30, 2025
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2025
@bors
Copy link
Contributor

bors commented Jan 30, 2025

⌛ Trying commit 022c0ce with merge 78f2ee6...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2025
@bors
Auto merge of rust-lang#136270 - nnethercote:rm-NamedVarMap, r=<try>
78f2ee6 
Remove `NamedVarMap`.

`NamedVarMap` is extremely similar to `ResolveBoundVars`. The former contains two `UnordMap<ItemLocalId, T>` fields (obscured behind `ItemLocalMap` typedefs). The latter contains two
`SortedMap<ItemLocalId, T>` fields. We construct a `NamedVarMap` and then convert it into a `ResolveBoundVars` by sorting the `UnordMap`s, which is unnecessary busywork.

This commit removes `NamedVarMap` and constructs a `ResolveBoundVars` directly. `SortedMap` and `NamedVarMap` have slightly different perf characteristics during construction (e.g. speed of insertion) but this code isn't hot enough for that to matter.

A few details to note.
- A `FIXME` comment is removed.
- The detailed comments on the fields of `NamedVarMap` are copied to `ResolveBoundVars` (which has a single, incorrect comment).
- `BoundVarContext::map` is renamed.
- `ResolveBoundVars` gets a derived `Default` impl.

r? `@ghost`
@bors
Copy link
Contributor

bors commented Jan 30, 2025

☀️ Try build successful - checks-actions
Build commit: 78f2ee6 (78f2ee6a3f92add485dfc3b65be2ca33d50aa6f9)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (78f2ee6): comparison URL.

Overall result: ✅ improvements - 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 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
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

Cycles

Results (secondary -1.0%)

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

Binary size

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

Bootstrap: 775.772s -> 776.605s (0.11%)
Artifact size: 328.49 MiB -> 328.48 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2025
@nnethercote
Copy link
Contributor Author

nnethercote commented Jan 30, 2025 •
edited
Loading

No perf effect, as expected.

@nnethercote nnethercote marked this pull request as ready for review January 30, 2025 04:55
@nnethercote
Copy link
Contributor Author

@bors rollup

jackh726
jackh726 approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

One question, r=me with it answered

compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Jan 30, 2025

📌 Commit 022c0ce has been approved by jackh726

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 Jan 30, 2025
@jhpratt jhpratt mentioned this pull request Jan 31, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
@bors
Auto merge of rust-lang#136331 - jhpratt:rollup-curo1f4, r=jhpratt
25a1657 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#135414 (Stabilize `const_black_box`)
 - rust-lang#136150 (ci: use windows 2025 for i686-mingw)
 - rust-lang#136258 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 11))
 - rust-lang#136270 (Remove `NamedVarMap`.)
 - rust-lang#136278 (add constraint graph to polonius MIR dump)
 - rust-lang#136287 (LLVM changed the nocapture attribute to captures(none))
 - rust-lang#136291 (some test suite cleanups)
 - rust-lang#136296 (float::min/max: mention the non-determinism around signed 0)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dcf1134 into rust-lang:master Jan 31, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 31, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
@rust-timer
Unrolled build for rust-lang#136270
4d947b8 
Rollup merge of rust-lang#136270 - nnethercote:rm-NamedVarMap, r=jackh726

Remove `NamedVarMap`.

`NamedVarMap` is extremely similar to `ResolveBoundVars`. The former contains two `UnordMap<ItemLocalId, T>` fields (obscured behind `ItemLocalMap` typedefs). The latter contains two
`SortedMap<ItemLocalId, T>` fields. We construct a `NamedVarMap` and then convert it into a `ResolveBoundVars` by sorting the `UnordMap`s, which is unnecessary busywork.

This commit removes `NamedVarMap` and constructs a `ResolveBoundVars` directly. `SortedMap` and `NamedVarMap` have slightly different perf characteristics during construction (e.g. speed of insertion) but this code isn't hot enough for that to matter.

A few details to note.
- A `FIXME` comment is removed.
- The detailed comments on the fields of `NamedVarMap` are copied to `ResolveBoundVars` (which has a single, incorrect comment).
- `BoundVarContext::map` is renamed.
- `ResolveBoundVars` gets a derived `Default` impl.

r? `@jackh726`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

Labels
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
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @rust-timer @bors @jackh726 @rustbot