Make some traversable types generic over interner #108214

eggyal · 2023-02-18T17:13:28Z

Implementations of the traversable traits (TypeFoldable and TypeVisitable) that specify a concrete interner (TyCtxt) can be defined in the crate that defines that interner (rustc_middle) irrespective of whether the type for which the traits are being implemented are local or foreign to that crate—and indeed this is where many such implementations have historically been defined, even for simple types that otherwise have no dependency on the type context.

In particular, the rustc_middle::ty::structural_impls module (and a few others) utilised the rustc_middle::macros::TrivialTypeTraversalImpls macro-by-example to define traversable trait implementations for many such "atomic" types. Making such implementations generic over the interner where those types are foreign to rustc_middle requires those implementations to move into either the type library or the crate that defines the type. In this PR I have opted for the latter approach, which creates some new dependencies on the type library (for access to the traits); the former would either require the type library to further depend on other parts of the compiler (undesirable, I think?) or for those types themselves to move into it (as previously happened for some basic types and is potentially desirable for further types in a future PR).

Because these implementations are all rather trivial, they are provided by (modified versions of) the existing TypeFoldable and TypeVisitable derive macros. These macros now produce implementations that either specify the TyCtxt interner or are generic over the interner depending on whether the type for which they are derived is defined with a 'tcx lifetime parameter or not.

r? @oli-obk

rustbot · 2023-02-18T17:13:35Z

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured in abstract_const.rs

cc @BoxyUwU

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

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

compiler/rustc_middle/src/mir/interpret/value.rs

bors · 2023-02-22T22:13:33Z

☔ The latest upstream changes (presumably #108340) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2023-02-26T14:25:19Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

eggyal · 2023-02-26T14:40:04Z

I've reworked this PR to use (in a8cd76c9d450abd0b78f0fbcf9268677fd07d591) the genius suggestion made by @lcnr in the Zulip topic for my (now closed) associated MCP of an auto-trait together with auto-deref specialisation. This ensures that:

any incorrect application of the #[skip_traversal] attribute will result in an inapplicable impl (modulo an inappropriate implementation of the SkipTraversalAutoImplOnly auto-trait, which documents that such implementations should never be made);
types that do not contain anything of interest to folders/visitors mostly no longer need to implement the traits even if they appear within types that do: the auto-deref specialisation used in the impls generated by the derive macros will automatically treat them as no-ops. Consequently, many previously required impls have been removed. I have further refactored some other impls to use the derive macros where possible.

I think a perf run would now be wise.

Maybe given the amount of thought @lcnr has put into this already, they're now the most suitable reviewer? I wanted to get your nod on that first though @oli-obk?

oli-obk · 2023-02-26T17:36:22Z

@bors try @rust-timer queue

I'll happily let lcnr take over this PR, but will review next week otherwise

bors · 2023-02-26T17:36:31Z

⌛ Trying commit 784870c3d73ee21f3d730a1406010f77620ea0d0 with merge 516caeb9588d175d929b03ef6889f0b43055f136...

eggyal · 2023-02-26T18:30:20Z

Thanks Oli. Unfortunately, I hadn't noticed your comment before I pushed a minor change to comments that I think cancelled the perf run?

bjorn3 · 2023-02-27T11:25:59Z

@bors try @rust-timer queue

bors · 2023-02-27T11:26:09Z

⌛ Trying commit e1f6096f7118020b355bdff7b4e2dd48d910825c with merge 3c471c0bbf60528fb31cc524fbb5a51c124d6fae...

bors · 2023-02-27T14:06:32Z

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

rust-timer · 2023-02-27T15:24:44Z

Finished benchmarking commit (3c471c0bbf60528fb31cc524fbb5a51c124d6fae): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.8%	[0.3%, 2.2%]	40
Regressions ❌ (secondary)	1.4%	[0.4%, 4.7%]	43
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.3%, 2.2%]	40

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
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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
Regressions ❌ (secondary)	2.1%	[1.7%, 2.7%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

eggyal · 2023-02-27T15:27:10Z

I have a hunch the regressions will be down to some function calls that are now cross-crate no longer being inlined. Will dig deeper.

oli-obk · 2023-03-21T12:24:06Z

@bors try @rust-timer queue

bors · 2023-03-21T12:24:16Z

⌛ Trying commit bf618ac with merge 1950270ad663a766750d9890629a5cd5abca197a...

bors · 2023-03-21T14:19:47Z

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

rust-timer · 2023-03-21T16:40:50Z

Finished benchmarking commit (1950270ad663a766750d9890629a5cd5abca197a): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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)	2.0%	[0.8%, 3.9%]	8
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.5%, -0.4%]	3

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.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.0%, -0.8%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-1.0%, 1.0%]	6

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-12.7%	[-13.4%, -12.3%]	6
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	1

eggyal · 2023-03-22T00:24:23Z

Okay, so we're definitely moving in the right direction. Of the regressions that remain, 6/8 arose on the deeply_nested_multi crate. In all of those cases, the cgdiff most materially include something like:

6,221,122  ???:<rustc_middle::ty::Ty as rustc_type_ir::fold::TypeSuperFoldable<rustc_middle::ty::context::TyCtxt>>::super_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
1,805,758  ???:<rustc_middle::ty::Ty as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::try_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
1,565,584  ???:<rustc_infer::infer::ShallowResolver>::fold_infer_ty
  847,898  ???:<core::option::Option<alloc::rc::Rc<rustc_middle::traits::ObligationCauseCode>> as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::try_fold_with::<rustc_infer::infer::resolve::OpportunisticVarResolver>
  839,194  ???:<rustc_trait_selection::traits::fulfill::FulfillProcessor as rustc_data_structures::obligation_forest::ObligationProcessor>::process_obligation
 -837,429  ???:rustc_trait_selection::traits::wf::obligations
  620,831  ???:<rustc_trait_selection::traits::wf::WfPredicates>::nominal_obligations_inner
 -460,969  ???:<rustc_infer::infer::resolve::OpportunisticVarResolver as rustc_type_ir::fold::FallibleTypeFolder<rustc_middle::ty::context::TyCtxt>>::try_fold_ty

I'm pretty sure something here isn't getting inlined as it was before, but I'm really not sure what... I'll get a cross-compilation of a debug build going tomorrow so that I can hopefully glean something useful from the cachegrind line counts.

nnethercote · 2023-03-22T02:20:57Z

Look in and around OpportunisticVarResolver::fold_ty.

eggyal · 2023-03-22T11:36:54Z

Okay, it's not due to inlining: OpportunisticVarResolver is being invoked the same number of times as before, but folds are recursing deeper. I would imagine this is most likely due to 408ba98 and/or ef97776, which introduced traversals on fields of (respectively) BindingForm and Obligation that had previously been omitted (I think accidentally?).

That being the case, and these being minor regressions in secondary benchmarks, are we okay to mark this regression as triaged?

oli-obk · 2023-03-22T11:58:49Z

we're not doing ef97776 on purpose. @estebank tried doing something there iirc and it had performance problems.

Either way, I think we should leave that commit out of this PR and land it afterwards as a separate PR where we can discuss whether that needs to be done.

eggyal · 2023-03-22T12:14:21Z

Perhaps it'd be sensible to use an attribute on the cause field, eg #[skip_traversal(despite_interesting_type_because = "...")]? That way any field changes in future will be caught by the derive macro, and the reason for the cause being skipped is mandatorily documented?

oli-obk · 2023-03-22T12:21:52Z

I like that, but if it is a lot of implementation effort, maybe just hand-roll the impl

JohnCSimon · 2023-05-28T13:51:38Z

@eggyal
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Dylan-DPC · 2023-08-06T10:09:46Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

eggyal · 2023-11-02T13:23:11Z

Apologies for the lack of attention on this—had a host of other things demanding my attention over these past months. I'm back on it now. I don't appear to have the option to reopen the PR—happy to create a new one, whatever is preferred?

lcnr · 2023-11-03T10:53:39Z

creating a new one is fine 👍

…pes_noop-traversable, r=<try> Automatically make trivial types noop-traversable This is a second reincarnation of rust-lang#108214 (or at least, what that PR ultimately became), the previous reincarnation in rust-lang#117620 having been closed accidentally. It may be easier to review by commit, albeit there are quite a few of them. ## Terminology I refer to: * folds and visits as "traversals" * the types that are folded or visited as "traversables" * the folders and visitors as "traversers" * traversables on which traversers can act as "interesting" (there are currently only five: binders, types, consts, regions and predicates) and all others as "trivial" (traversals of which are no-ops) * the `TypeFoldable` and `TypeVisitable` traits (and only these traits) as the "traversable traits" ## Content This PR: * introduces a macro, `rustc_type_ir::noop_if_trivially_traversable` that uses [auto-deref specialisation](http://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html) to: * perform a no-op traversal on values of type `T` if the interner implements `rustc_type_ir::TriviallyTraverses<T>` (thereby obviating all need for most trivial types to implement the traversable traits—and indeed such implementations are removed) or * delegate to the relevant traversable trait implementation on `T` otherwise; * introduces an auto-trait, `rustc_middle::ty::TriviallyTraversable`, that is then "unimplemented" on the interesting types (and thereby remains implemented only on, but on all, trivial types); * implements `rustc_type_ir::TriviallyTraverses<T>` on the `TyCtxt<'tcx>` interner for all `T: rustc_middle::ty::TriviallyTraversable` (thus ensuring that, for that interner at least, trivial types do not require implementations of the traversable traits); * updates the traversable traits' derive macros to: * skip fields that reference neither any generic type parameters nor, if present, the `'tcx` lifetime parameter * use the above specialisation macro when traversing fields * if the traversable is not parameterised by a `'tcx` lifetime, generate implementations that are generic over the interner; * replaces those derive macros' distinct helper attributes (introduced in rust-lang#108040) with a unified `#[skip_traversal]` helper attribute that requires a justifying reason to be provided (this does mean that the different types of traversal can no longer be independently skipped, but nowhere is that currently required or ever expected to be); * the derive macros by default refuse to be implemented on trivial types as specialisation usually negates any need—but this can be overridden with a `#[skip_traversal(impl_despite_trivial_because = "<reason>")]` attribute; * uses those derive macros in place of remaining usages of the `TrivialTypeTraversal` macros and some explicit implementations; and * a few other minor relevant refactorings. ## Commentary One limitation of auto-deref specialisation is that it cannot work for unconstrained generic types. Therefore, absent real specialisation, implementations of the traversable traits must constrain their generics: * by default, the derive macros constrain every field type `T` that references one or more generic type parameters to implementors of the relevant traversable trait—however this can be modified on a field (or variant) basis with a `#[skip_traversal(because_trivial)]` attribute so that instead the interner is constrained to implement `TriviallyTraverses<T>` (and the field is not traversed) * the constraints are applied to the field types rather than the generic type parameters to achieve a "[perfect derive](https://smallcultfollowing.com/babysteps/blog/2022/04/12/implied-bounds-and-perfect-derive/)" that not only avoids having to propagate `#[skip_traversal]` annotations into wrapping types but also works with associated types and other esoteric situations—however this could result in trait solver cycles if interesting generic field types are one day involved in recursive type definitions; * for exceptionally rare cases where traversal of an item/variant/field should be a no-op despite it being (potentially, if generic) interesting, it can be annotated with the explicit and deliberately cumbersome `#[skip_traversal(despite_potential_miscompilation_because = "<reason>")]` (which produces a no-op without adding any constraint). * (the few remaining) explicit implementations of the traversable traits over generic types are similarly constrained * indeed, for generic tuples *all element types* must implement the trait—however, since (most) trivial types no longer do, this unfortunately means that the implementations are not applicable to tuples with trivial elements and hence some such tuples have been replaced with more concrete newtypes that have appropriate constraints: c5e9447 is a particularly hefty example of this (where tuples containing `Span` are replaced with `Spanned<T>` instead). r? `@lcnr` cc `@oli-obk` `@rustbot` label A-query-system T-compiler WG-trait-system-refactor

rustbot assigned oli-obk Feb 18, 2023

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 Feb 18, 2023

RalfJung reviewed Feb 18, 2023

View reviewed changes

compiler/rustc_middle/src/mir/interpret/value.rs Outdated Show resolved Hide resolved

eggyal force-pushed the generify_traversables branch 2 times, most recently from dc58d23 to 0330754 Compare February 19, 2023 13:11

eggyal mentioned this pull request Feb 20, 2023

Derive noop impls of TypeFoldable/Visitable rust-lang/compiler-team#593

Closed

3 tasks

eggyal force-pushed the generify_traversables branch from 0330754 to 784870c Compare February 26, 2023 14:25

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Feb 26, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 26, 2023

eggyal force-pushed the generify_traversables branch from 784870c to e1f6096 Compare February 26, 2023 17:40

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 27, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 21, 2023

bors mentioned this pull request Mar 21, 2023

a general type system cleanup #109119

Merged

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 21, 2023

lcnr 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2023

lcnr mentioned this pull request Apr 26, 2023

More Typefoldable/TypeVisitable cleanups #110838

Merged

Dylan-DPC closed this Aug 6, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2023

This was referenced Nov 5, 2023

Automatically make "boring" types noop-traversable #117620

Closed

Automatically make trivial types noop-traversable #117726

Closed

eggyal deleted the generify_traversables branch April 16, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make some traversable types generic over interner #108214

Make some traversable types generic over interner #108214

eggyal commented Feb 18, 2023

rustbot commented Feb 18, 2023

bors commented Feb 22, 2023

rustbot commented Feb 26, 2023

eggyal commented Feb 26, 2023 •

edited

Loading

oli-obk commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

eggyal commented Feb 26, 2023

bjorn3 commented Feb 27, 2023

This comment has been minimized.

bors commented Feb 27, 2023

bors commented Feb 27, 2023

This comment has been minimized.

rust-timer commented Feb 27, 2023

eggyal commented Feb 27, 2023

oli-obk commented Mar 21, 2023

This comment has been minimized.

bors commented Mar 21, 2023

bors commented Mar 21, 2023

This comment has been minimized.

rust-timer commented Mar 21, 2023

eggyal commented Mar 22, 2023 •

edited

Loading

nnethercote commented Mar 22, 2023

eggyal commented Mar 22, 2023 •

edited

Loading

oli-obk commented Mar 22, 2023

eggyal commented Mar 22, 2023

oli-obk commented Mar 22, 2023

JohnCSimon commented May 28, 2023

Dylan-DPC commented Aug 6, 2023

eggyal commented Nov 2, 2023

lcnr commented Nov 3, 2023

Make some traversable types generic over interner #108214

Make some traversable types generic over interner #108214

Conversation

eggyal commented Feb 18, 2023

rustbot commented Feb 18, 2023

bors commented Feb 22, 2023

rustbot commented Feb 26, 2023

eggyal commented Feb 26, 2023 • edited Loading

oli-obk commented Feb 26, 2023

This comment has been minimized.

bors commented Feb 26, 2023

eggyal commented Feb 26, 2023

bjorn3 commented Feb 27, 2023

This comment has been minimized.

bors commented Feb 27, 2023

bors commented Feb 27, 2023

This comment has been minimized.

rust-timer commented Feb 27, 2023

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

eggyal commented Feb 27, 2023

oli-obk commented Mar 21, 2023

This comment has been minimized.

bors commented Mar 21, 2023

bors commented Mar 21, 2023

This comment has been minimized.

rust-timer commented Mar 21, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

eggyal commented Mar 22, 2023 • edited Loading

nnethercote commented Mar 22, 2023

eggyal commented Mar 22, 2023 • edited Loading

oli-obk commented Mar 22, 2023

eggyal commented Mar 22, 2023

oli-obk commented Mar 22, 2023

JohnCSimon commented May 28, 2023

Dylan-DPC commented Aug 6, 2023

eggyal commented Nov 2, 2023

lcnr commented Nov 3, 2023

eggyal commented Feb 26, 2023 •

edited

Loading

eggyal commented Mar 22, 2023 •

edited

Loading

eggyal commented Mar 22, 2023 •

edited

Loading