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 -Zkeep-hygiene-data. #117737

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Remove -Zkeep-hygiene-data. #117737

merged 1 commit into from
Nov 13, 2023

Conversation

nnethercote
Copy link
Contributor

It was added way back in #28585 under the name -Zkeep-mtwt-tables. The justification was:

This is so that the resolution results can be used after analysis,
potentially for tool support.

There are no uses of significance in the code base, and various Google searches for both option names (and variants) found nothing of interest. I think this can safely be removed.

r? @davidtwco

@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 Nov 9, 2023
@nnethercote
Copy link
Contributor Author

I think David meant to r+ instead of using the GitHub approval.

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Nov 9, 2023

📌 Commit d704c1d has been approved by davidtwco

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 Nov 9, 2023
@nnethercote
Copy link
Contributor Author

@bors rollup

@davidtwco
Copy link
Member

I think David meant to r+ instead of using the GitHub approval.

I was just waiting for the CI to pass but then got caught up in other things and didn't get back to this quick enough.

@petrochenkov
Copy link
Contributor

@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 Nov 9, 2023
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned davidtwco Nov 9, 2023
@nnethercote
Remove -Zkeep-hygiene-data.
2e40d11 
It was added way back in rust-lang#28585 under the name `-Zkeep-mtwt-tables`. The
justification was:

> This is so that the resolution results can be used after analysis,
> potentially for tool support.

There are no uses of significance in the code base, and various Google
searches for both option names (and variants) found nothing of interest.

@petrochenkov says removing this part (and it's only part) of the
hygiene data is dubious. It doesn't seem that big, so let's just keep it
around.
@nnethercote nnethercote force-pushed the rm-Zkeep-hygiene-data branch from d704c1d to 2e40d11 Compare November 10, 2023 03:01
@nnethercote
Copy link
Contributor Author

I removed clear_syntax_context_map, let's see if memory usage is noticeably changed.

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

bors commented Nov 10, 2023

⌛ Trying commit 2e40d11 with merge a35f9c1...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2023
@bors
Auto merge of rust-lang#117737 - nnethercote:rm-Zkeep-hygiene-data, r…
a35f9c1 
…=<try>

Remove `-Zkeep-hygiene-data`.

It was added way back in rust-lang#28585 under the name `-Zkeep-mtwt-tables`. The justification was:

> This is so that the resolution results can be used after analysis,
> potentially for tool support.

There are no uses of significance in the code base, and various Google searches for both option names (and variants) found nothing of interest. I think this can safely be removed.

r? `@davidtwco`
@bors
Copy link
Contributor

bors commented Nov 10, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a35f9c1): 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -6.5% [-12.3%, -0.7%] 2
Improvements ✅
(secondary)		 -0.4% [-0.4%, -0.4%] 1
All ❌✅ (primary) -6.5% [-12.3%, -0.7%] 2

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.6% [0.4%, 1.4%] 11
Regressions ❌
(secondary)		 3.6% [1.0%, 6.2%] 10
Improvements ✅
(primary)		 -0.5% [-0.6%, -0.5%] 3
Improvements ✅
(secondary)		 -2.9% [-12.5%, -0.5%] 9
All ❌✅ (primary) 0.4% [-0.6%, 1.4%] 14

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.5% [0.4%, 0.6%] 3
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -2.8% [-9.5%, -0.5%] 4
Improvements ✅
(secondary)		 -1.1% [-1.1%, -1.1%] 1
All ❌✅ (primary) -1.4% [-9.5%, 0.6%] 7

Binary size

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

Bootstrap: 673.197s -> 672.701s (-0.07%)
Artifact size: 311.10 MiB -> 311.13 MiB (0.01%)

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

max-rss is our noisiest metric, so it's hard to say with 100% certainty. But this doesn't look like it had any significant performance effect. I think it's ok to merge.

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

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 13, 2023

📌 Commit 2e40d11 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2023
@TaKO8Ki TaKO8Ki mentioned this pull request Nov 13, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2023
@bors
Auto merge of rust-lang#117881 - TaKO8Ki:rollup-n7jtmgj, r=TaKO8Ki
531cb83 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#117737 (Remove `-Zkeep-hygiene-data`.)
 - rust-lang#117830 (Small improvements in object lifetime default code)
 - rust-lang#117858 (Compute layout with spans for better cycle errors in coroutines)
 - rust-lang#117863 (Remove some unused stuff from `rustc_index`)
 - rust-lang#117872 (Cranelift isn't available on non-nightly channels)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bf8cf45 into rust-lang:master Nov 13, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 13, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2023
@rust-timer
Unrolled build for rust-lang#117737
aaf9e92 
Rollup merge of rust-lang#117737 - nnethercote:rm-Zkeep-hygiene-data, r=petrochenkov

Remove `-Zkeep-hygiene-data`.

It was added way back in rust-lang#28585 under the name `-Zkeep-mtwt-tables`. The justification was:

> This is so that the resolution results can be used after analysis,
> potentially for tool support.

There are no uses of significance in the code base, and various Google searches for both option names (and variants) found nothing of interest. I think this can safely be removed.

r? `@davidtwco`
@nnethercote nnethercote deleted the rm-Zkeep-hygiene-data branch November 13, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@petrochenkov petrochenkov

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.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @bors @davidtwco @petrochenkov @rust-timer @rustbot