nightly feature tracking: get rid of the per-feature bool fields #132027

RalfJung · 2024-10-22T07:33:26Z

The struct Features that tracks which features are enabled has a ton of public bool-typed fields that are basically caching the result of looking up the corresponding feature in enabled_lang_features. Having public fields with an invariant is not great, so at least they should be made private. However, it turns out caching these lookups is actually not worth it, so this PR just entirely gets rid of these fields. (The alternative would be to make them private and have a method for each of them to expose them in a read-only way. Most of the diff of this PR would be the same in that case.)

r? @nnethercote

rustbot · 2024-10-22T07:33:38Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

HIR ty lowering was modified

cc @fmease

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in match checking

cc @Nadrieril

changes to the core type system

cc @compiler-errors, @lcnr

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

nnethercote

A nice simplification. Just a few minor comments.

nnethercote · 2024-10-22T21:35:18Z

compiler/rustc_query_system/src/ich/impls_syntax.rs

@@ -115,7 +115,7 @@ impl<'tcx> HashStable<StableHashingContext<'tcx>> for rustc_feature::Features {
 self.enabled_lang_features().hash_stable(hcx, hasher);
 self.enabled_lib_features().hash_stable(hcx, hasher);

- self.all_lang_features()[..].hash_stable(hcx, hasher);
+ // FIXME: why do we hash something that is a compile-time constant?


This is a good question. But I suggest undoing this change and then redoing it in a follow-up PR. That way if any problems occur they will bisect back to a small and distinct PR.

nnethercote · 2024-10-22T21:43:32Z

compiler/rustc_feature/src/builtin_attrs.rs

@@ -14,7 +14,7 @@ type GateFn = fn(&Features) -> bool;

 macro_rules! cfg_fn {
 ($field: ident) => {
- (|features| features.$field) as GateFn
+ Features::$field as GateFn


Is the as GateFn still needed? If not, it might be worth removing this macro and writing Features::foo directly in all the relevant places.

nnethercote · 2024-10-22T21:45:29Z

compiler/rustc_feature/src/unstable.rs

@@ -8,7 +8,6 @@ use super::{Feature, to_nonzero};

 pub struct UnstableFeature {


This struct is now a trivial wrapper around Feature. Maybe remove it in a second commit?

nnethercote · 2024-10-22T21:52:18Z

compiler/rustc_feature/src/unstable.rs

@@ -30,6 +29,46 @@ macro_rules! status_to_enum {
 };
 }

+/// A set of features to be used by later passes.


A comment like this would be useful here:

There are two ways to check if a feature `foo` is enabled. - Directly with the `foo` method, e.g. `if tcx.features().foo() { ... }`. - With the `enabled` method, e.g. `if tcx.features.enabled(sym::foo) { ... }`. The former is preferred. `enabled` should only be used when the feature symbol is not a constant, e.g. a parameter.

nnethercote · 2024-10-22T21:53:37Z

compiler/rustc_feature/src/unstable.rs

+ enabled_features: FxHashSet<Symbol>,
+}
+
+impl Features {


This impl block is much nicer now 👍

nnethercote · 2024-10-22T21:58:13Z

compiler/rustc_codegen_ssa/src/target_features.rs

- Some(sym::x86_amx_intrinsics) => rust_features.x86_amx_intrinsics,
- Some(sym::xop_target_feature) => rust_features.xop_target_feature,
- Some(sym::s390x_target_feature) => rust_features.s390x_target_feature,
+ Some(sym::arm_target_feature) => rust_features.arm_target_feature(),


This could be changed to:

Some(sym) => { match sym { sym::arm_target_feature | sym::hexagon_target_feature | ... // all the other possibilities => rust_features.enabled(sym) } }

It has more indentation, but would avoid the feature name being repeated on every line, and make it easier to add new lines in the future. What do you think?

nnethercote · 2024-10-22T22:01:09Z

Let's do another perf run, just to be sure:

@bors try @rust-timer queue

rust-timer · 2024-10-22T22:01:11Z

Awaiting bors try build completion.

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

…=<try> nightly feature tracking: get rid of the per-feature bool fields The `struct Features` that tracks which features are enabled has a ton of public `bool`-typed fields that are basically caching the result of looking up the corresponding feature in `enabled_lang_features`. Having public fields with an invariant is not great, so at least they should be made private. However, it turns out caching these lookups is actually [not worth it](rust-lang#131321 (comment)), so this PR just entirely gets rid of these fields. (The alternative would be to make them private and have a method for each of them to expose them in a read-only way. Most of the diff of this PR would be the same in that case.) r? `@nnethercote`

bors · 2024-10-22T22:02:21Z

⌛ Trying commit dd8850b with merge 740f801...

bors · 2024-10-22T23:55:14Z

☀️ Try build successful - checks-actions
Build commit: 740f801 (740f801072bbece1059248b3173d1ea8d7da6435)

rust-timer · 2024-10-22T23:55:17Z

Queued 740f801 with parent 86d69c7, future comparison URL.
There is currently 1 preceding artifact in the queue.
It will probably take at least ~2.5 hours until the benchmark run finishes.

rustbot assigned nnethercote Oct 22, 2024

nightly feature tracking: get rid of the per-feature bool fields

dd8850b

RalfJung force-pushed the lang-feature-bool-fields branch from 16e611f to dd8850b Compare October 22, 2024 14:40

nnethercote reviewed Oct 22, 2024

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nightly feature tracking: get rid of the per-feature bool fields #132027

nightly feature tracking: get rid of the per-feature bool fields #132027

RalfJung commented Oct 22, 2024 •

edited

Loading

rustbot commented Oct 22, 2024

nnethercote left a comment

nnethercote Oct 22, 2024

nnethercote Oct 22, 2024

nnethercote Oct 22, 2024

nnethercote Oct 22, 2024

nnethercote Oct 22, 2024

nnethercote Oct 22, 2024

nnethercote commented Oct 22, 2024

rust-timer commented Oct 22, 2024

bors commented Oct 22, 2024

bors commented Oct 22, 2024

rust-timer commented Oct 22, 2024

		@@ -8,7 +8,6 @@ use super::{Feature, to_nonzero};

		pub struct UnstableFeature {

nightly feature tracking: get rid of the per-feature bool fields #132027

Are you sure you want to change the base?

nightly feature tracking: get rid of the per-feature bool fields #132027

Conversation

RalfJung commented Oct 22, 2024 • edited Loading

rustbot commented Oct 22, 2024

nnethercote left a comment

Choose a reason for hiding this comment

nnethercote Oct 22, 2024

Choose a reason for hiding this comment

nnethercote Oct 22, 2024

Choose a reason for hiding this comment

nnethercote Oct 22, 2024

Choose a reason for hiding this comment

nnethercote Oct 22, 2024

Choose a reason for hiding this comment

nnethercote Oct 22, 2024

Choose a reason for hiding this comment

nnethercote Oct 22, 2024

Choose a reason for hiding this comment

nnethercote commented Oct 22, 2024

rust-timer commented Oct 22, 2024

bors commented Oct 22, 2024

bors commented Oct 22, 2024

rust-timer commented Oct 22, 2024

RalfJung commented Oct 22, 2024 •

edited

Loading