Avoid creating SmallVecs in global_llvm_features
#97579
Conversation
rustbot
added
the
T-compiler
|
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -218,6 +218,9 @@ pub fn check_tied_features( | |||
| sess: &Session, | |||
| features: &FxHashMap<&str, bool>, | |||
| ) -> Option<&'static [&'static str]> { | |||
| if features.len() == 0 { | |||
There was a problem hiding this comment.
Nit: this is probably better written as something like:
if !features.is_empty() {
for tied in ... {
...
}
}
return None;
The code isn't particularly deeply nested here and avoids having to specify None as a return twice.
| @@ -485,35 +490,35 @@ pub(crate) fn global_llvm_features(sess: &Session, diagnostics: bool) -> Vec<Str | |||
| } | |||
| diag.emit(); | |||
| } | |||
| Some((enable_disable, feature)) | |||
|
|
|||
| if diagnostics && tied_features.iter().any(|tied| tied.contains(&feature)) { | |||
There was a problem hiding this comment.
This is doing a second linear lookup through the tied_features list (in addition to the one that will happen later in the check_tied_features.
Ignoring entirely the fact that what's going on here is an exercise in micro-optimization, and the overall performance of the compiler is likely going to stay pretty much the same, I feel like such double lookup is probably defeating any improvements gained here by all the other changes.
It would seem to me that we'd be better off by just not checking this here (while leaving everything else intact):
| if diagnostics && tied_features.iter().any(|tied| tied.contains(&feature)) { | |
| if diagnostics { |
There was a problem hiding this comment.
Thanks for pointing out! That makes sense and I think we can keep the FIXME here
nagisa
added
S-waiting-on-author
SparrowLii
force-pushed
the
target_features
branch
from
c10ae82
to
b3cd892
Compare
|
Thanks for reviewing! I made the corresponding corrections |
|
@bors r+ thanks! |
|
📌 Commit b3cd892 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - rust-lang#97312 (Compute lifetimes in scope at diagnostic time) - rust-lang#97495 (Add E0788 for improper #[no_coverage] usage) - rust-lang#97579 (Avoid creating `SmallVec`s in `global_llvm_features`) - rust-lang#97767 (interpret: do not claim UB until we looked more into variadic functions) - rust-lang#97787 (E0432: rust 2018 -> rust 2018 or later in --explain message) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This PR made a simple optimization to avoid creating extra
SmallVecs by adjusting the use of iterator statements.Also, given the very small size of
tied_target_features, there is no need to insert each feature into the FxHashMap.