Skip to content

Cut code size for feature hashing #118348

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

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Cut code size for feature hashing #118348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions compiler/rustc_feature/src/unstable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ macro_rules! declare_features {
}),+
];

const NUM_FEATURES: usize = UNSTABLE_FEATURES.len();

/// A set of features to be used by later passes.
#[derive(Clone, Default, Debug)]
pub struct Features {
Expand Down Expand Up @@ -82,8 +84,14 @@ macro_rules! declare_features {
self.declared_features.insert(symbol);
}

pub fn walk_feature_fields(&self, mut f: impl FnMut(&str, bool)) {
$(f(stringify!($feature), self.$feature);)+
/// This is intended for hashing the set of active features.
///
/// The expectation is that this produces much smaller code than other alternatives.
///
/// Note that the total feature count is pretty small, so this is not a huge array.
#[inline]
pub fn all_features(&self) -> [u8; NUM_FEATURES] {
Copy link
Member

@compiler-errors compiler-errors Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why is it better to turn it into a u8 from a bool? Better HashStable impl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe we should add a specialization there as well but [u8] operates in blocks while [bool] will operate one by one. In practice since this is compiling to a loop vs. fully unrolling (what we were doing before this PR afaict) it's not that big a difference but seems like an easy win. Also discourages usage of this array for anything but hashing.

[$(self.$feature as u8),+]
Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't like this I might look into seeing how painful/annoying it would be to make the tail of the struct be a repr(C) struct that we can cast into &[u8] directly. Theoretically if we're not reordering fields that's basically already what should happen here. In any case, there's only ~250 bytes here so it's not a big copy anyway.

}

/// Is the given feature explicitly declared, i.e. named in a
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_query_system/src/ich/impls_syntax.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,9 @@ impl<'tcx> HashStable<StableHashingContext<'tcx>> for rustc_feature::Features {
self.declared_lang_features.hash_stable(hcx, hasher);
self.declared_lib_features.hash_stable(hcx, hasher);

self.walk_feature_fields(|feature_name, value| {
feature_name.hash_stable(hcx, hasher);
value.hash_stable(hcx, hasher);
});
self.all_features()[..].hash_stable(hcx, hasher);
for feature in rustc_feature::UNSTABLE_FEATURES.iter() {
feature.feature.name.hash_stable(hcx, hasher);
}
}
}