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

Simplify hir::PathSegment #101228

Merged
merged 5 commits into from
Sep 5, 2022
Merged

Conversation

nnethercote
Copy link
Contributor

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 31, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2022

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2022
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

I'm not sure if removing the Options is a good idea or not. But I did measurements and found that over 90% of PathSegments had both the hir_id and the res, so it seems like they might as well be mandatory. Let me know what you think.

@petrochenkov
Copy link
Contributor

I think it's a good idea, since these IDs and resolutions are already actively used by lints and similar, it's better to make sure that they are always set.

compiler/rustc_hir/src/hir.rs Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
@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 Aug 31, 2022
@nnethercote
Copy link
Contributor Author

I have addressed the review comments.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2022

📌 Commit 11020f1 has been approved by petrochenkov

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 2, 2022
…nt, r=petrochenkov

Simplify `hir::PathSegment`

r? `@petrochenkov`
@Dylan-DPC
Copy link
Member

failed in rollup

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 2, 2022
@nnethercote
Copy link
Contributor Author

I fixed the rollup failure.

@nnethercote
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Sep 5, 2022

📌 Commit 08a00eb has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 5, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 5, 2022
@bors
Copy link
Contributor

bors commented Sep 5, 2022

⌛ Testing commit 08a00eb with merge 2dc703f...

@bors
Copy link
Contributor

bors commented Sep 5, 2022

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 2dc703f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2022
@bors bors merged commit 2dc703f into rust-lang:master Sep 5, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 5, 2022
@nnethercote nnethercote deleted the simplify-hir-PathSegment branch September 5, 2022 23:00
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2dc703f): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.7% [0.3%, 1.3%] 16
Regressions ❌
(secondary)
1.3% [0.9%, 1.8%] 14
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.7% [0.3%, 1.3%] 16

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.8% [-1.8%, -1.8%] 1
All ❌✅ (primary) - - 0

Cycles

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

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@rustbot rustbot added the perf-regression Performance regression. label Sep 6, 2022
@nnethercote
Copy link
Contributor Author

Weird regressions here. I did some downloads and diffs with Cachegrind. The tt-muncher and html5ever-0.26.0 ones are due to changes in parse_tt, which this PR didn't touch, but which is hot and I think somewhat prone to perturbations due to minor changes. The keccak one is due to process_obligations, which is also hot and prone to perturbations. And I didn't expect the changes in this PR to have much effect, it really wasn't changing things much.

@nnethercote
Copy link
Contributor Author

Some evidence here that these apparent regressions weren't real.

@nnethercote
Copy link
Contributor Author

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 6, 2022
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 9, 2022
…, r=petrochenkov

Simplify `hir::PathSegment`

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-rustdoc Relevant to the rustdoc 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