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

Remove dead code in rustc_session::Options #84802

Merged
merged 4 commits into from
May 2, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 1, 2021

  • Don't recompile the same functions for each debugging option
    This reduces the amount of items in the crate by quite a lot.
  • Remove unused parse_opt_list and parse_pathbuf_push functions
  • Remove unused macro parameters
  • Remove allow(dead_code).

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 1, 2021
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2021
@jyn514
Copy link
Member Author

jyn514 commented May 1, 2021

I expect this should help bootstrap time for rustc_session quite a bit (other crates shouldn't be affected).

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 1, 2021
@bors
Copy link
Contributor

bors commented May 1, 2021

⌛ Trying commit 2828dec942bdc6c8299dbc74411bfa1270ebbf79 with merge 5ce9808b3b5bfb44d46efe3fe2d5d152fd51f39a...

jyn514 added 3 commits May 1, 2021 19:01
This reduces the amount of items in the crate by quite a lot.
This also remove the `allow(dead_code)`.
@jyn514 jyn514 force-pushed the option-dead-code branch from 2828dec to b19c02c Compare May 1, 2021 23:02
@jyn514
Copy link
Member Author

jyn514 commented May 1, 2021

Err ... I guess there's not a way to cancel an ongoing try run, and I don't know a way to tell rust-timer to queue a commit that isn't the latest.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented May 1, 2021

⌛ Trying commit b19c02c with merge a64224eaa14138ddc834a78d854d070df9154409...

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Overall looks fine to me, presuming perf is happy.

pub const parse_wasi_exec_model: &str = "either `command` or `reactor`";
pub const parse_split_debuginfo: &str =
"one of supported split-debuginfo modes (`off` or `dsymutil`)";
&[ $( (stringify!($opt), crate::options::parse::$opt, $crate::options::desc::$parse, $desc) ),* ];
Copy link
Member

Choose a reason for hiding this comment

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

nit: one of these uses $crate, the other does not. Doesn't matter in practice as it's crate local (I think) but let's be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing - I made this change locally, but I don't want to push since it will cancel the perf run.

diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs
index 735188768e4..cd28517bfbc 100644
--- a/compiler/rustc_session/src/options.rs
+++ b/compiler/rustc_session/src/options.rs
@@ -286,7 +286,7 @@ fn dep_tracking_hash(&self, _for_crate_hash: bool, error_format: ErrorOutputType
 
     pub type $setter_name = fn(&mut $struct_name, v: Option<&str>) -> bool;
     pub const $stat: &[(&str, $setter_name, &str, &str)] =
-        &[ $( (stringify!($opt), crate::options::parse::$opt, $crate::options::desc::$parse, $desc) ),* ];
+        &[ $( (stringify!($opt), $crate::options::parse::$opt, $crate::options::desc::$parse, $desc) ),* ];
 
     // Sometimes different options need to build a common structure.
     // That structure can kept in one of the options' fields, the others become dummy.

@bors
Copy link
Contributor

bors commented May 1, 2021

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

@rust-timer
Copy link
Collaborator

Queued a64224eaa14138ddc834a78d854d070df9154409 with parent 6e2a344, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (a64224eaa14138ddc834a78d854d070df9154409): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 2, 2021
@jyn514
Copy link
Member Author

jyn514 commented May 2, 2021

It looks like the time for rustc_session went ... up? 😕 the compiler should be doing strictly less work, maybe it's just noise?

@Mark-Simulacrum
Copy link
Member

Likely noise. Dead code doesn't usually influence performance too much as it's not generally codegen'd, and rustc itself is pretty fast. This still seems like an improvement overall to me, though, at least in terms of readability. @bors r+

@bors
Copy link
Contributor

bors commented May 2, 2021

📌 Commit b19c02c has been approved by Mark-Simulacrum

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2021
@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 May 2, 2021
@bors
Copy link
Contributor

bors commented May 2, 2021

⌛ Testing commit b19c02c with merge 89ebad5...

@bors
Copy link
Contributor

bors commented May 2, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 89ebad5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 2, 2021
@bors bors merged commit 89ebad5 into rust-lang:master May 2, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 2, 2021
@jyn514 jyn514 deleted the option-dead-code branch May 2, 2021 14:07
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 2, 2021
Fix nit in rustc_session::options

Addresses rust-lang#84802 (comment) - I never actually pushed the commit before that PR got merged.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.

7 participants