Clarify iterator interners #108112

nnethercote · 2023-02-16T03:40:55Z

I found the iterator interners very confusing. This PR clarifies things.

r? @compiler-errors

rustbot · 2023-02-16T03:41:03Z

Some changes occured in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

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

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

nnethercote · 2023-02-16T03:41:26Z

Note: the first four commits are carried over from #108020, please ignore them.

The remaining commits are best reviewed one commit at a time.

compiler-errors

Yes, I agree that the old traits were super confusing. Honestly also probably they were making rust-analyzer choke -- I never seemed to get good inference around them.

Just a few questions and comments.

If I understand things correctly -- I wonder if there would be a way to discourage users from doing something like mk_tuple([ty, ty].into_iter()) when they can do intern_tup(&[ty, ty])? Presumably the latter is more efficient, and there are two choices in many cases.

compiler/rustc_hir_typeck/src/check.rs

compiler/rustc_middle/src/ty/context.rs

nnethercote · 2023-02-16T04:30:39Z

If I understand things correctly -- I wonder if there would be a way to discourage users from doing something like mk_tuple([ty, ty].into_iter()) when they can do intern_tup(&[ty, ty])? Presumably the latter is more efficient, and there are two choices in many cases.

Aha! I have a half-written follow-up to this PR that will change every intern_foo/mk_foo pairing to mk_foo and mk_foo_from_iter. That way the simpler case gets the normal name, and the more complex/unusual case gets a more complex name.

compiler-errors · 2023-02-16T04:36:26Z

Fair enough. r=me then once the dependency PR lands.

rustbot · 2023-02-16T05:43:53Z

The Miri subtree was changed

cc @rust-lang/miri

nnethercote · 2023-02-16T05:45:33Z

I have added three new commits, one using IntoIterator for mk_fn_sig, and two other minor cleanups.

oli-obk · 2023-02-16T10:04:41Z

@bors try @rust-timer queue

bors · 2023-02-16T10:04:50Z

⌛ Trying commit 72935cbf0c538a4ff1961b6504d5df42098e9d9d with merge 31ed7ba148878b77b2fa85c45efbd2330feea940...

bors · 2023-02-16T12:50:16Z

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

bors · 2023-02-16T12:50:16Z

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

rust-timer · 2023-02-16T14:06:51Z

Finished benchmarking commit (31ed7ba148878b77b2fa85c45efbd2330feea940): comparison URL.

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

@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)	0.3%	[0.2%, 0.3%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	3.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	2.5%	[1.0%, 3.7%]	9
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-4.7%	[-6.5%, -2.9%]	2
All ❌✅ (primary)	1.1%	[-1.3%, 3.4%]	2

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)	-1.1%	[-1.4%, -0.9%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.4%, -0.9%]	5

oli-obk

r=me after a rebase eliminating the commits that already landed on master

compiler/rustc_trait_selection/src/solve/trait_goals.rs

nnethercote · 2023-02-16T21:31:04Z

Thanks for the reviews! I'm really happy with how this one turned out.

@bors r=oli-obk rollup=iffy

bors · 2023-02-16T21:31:06Z

📌 Commit 9922e085add3032037bb49f1cf994e0243e99da9 has been approved by oli-obk

It is now in the queue for this repository.

oli-obk · 2023-02-17T08:15:49Z

@bors r-
@bors r=oli-obk,compiler-errors

There are several `mk_foo`/`intern_foo` pairs, where the former takes an iterator and the latter takes a slice. (This naming convention is bad, but that's a fix for another PR.) This commit changes several `mk_foo` occurrences into `intern_foo`, avoiding the need for some `.iter()`/`.into_iter()` calls. Affected cases: - mk_type_list - mk_tup - mk_substs - mk_const_list

Giving the item type a name `T` avoids duplication.

`InternIteratorElement` is a trait used to intern values produces by iterators. There are three impls, corresponding to iterators that produce different types: - One for `T`, which operates straightforwardly. - One for `Result<T, E>`, which is fallible, and will fail early with an error result if any of the iterator elements are errors. - One for `&'a T`, which clones the items as it iterates. That last one is bad: it's extremely easy to use it without realizing that it clones, which goes against Rust's normal "explicit is better" approach to cloning. So this commit just removes it. In practice, there weren't many use sites. For all but one of them `into_iter()` could be used, which avoids the need for cloning. And for the one remaining case `copied()` is used.

There are two traits, `InternAs` and `InternIteratorElement`. I found them confusing to use, particularly this: ``` pub fn mk_tup<I: InternAs<Ty<'tcx>, Ty<'tcx>>>(self, iter: I) -> I::Output { iter.intern_with(|ts| self.intern_tup(ts)) } ``` where I thought there might have been two levels of interning going on (there isn't) due to the `intern_with`/`InternAs` + `intern_tup` naming. And then I found the actual traits and impls themselves *very* confusing. - `InternAs` has a single impl, for iterators, with four type variables. - `InternAs` is only implemented for iterators because it wouldn't really make sense to implement for any other type. And you can't really understand the trait without seeing that single impl, which is suspicious. - `InternAs` is basically just a wrapper for `InternIteratorElement` which does all the actual work. - Neither trait actually does any interning. They just have `Intern` in their name because they are used *by* interning code. - There are no comments. So this commit improves things. - It removes `InternAs` completely. This makes the `mk_*` function signatures slightly more verbose -- two trait bounds instead of one -- but much easier to read, because you only need to understand one trait instead of two. - It renames `InternIteratorElement` as `CollectAndApply`. Likewise, it renames its method `intern_with` as `collect_and_apply`. These names describe better what's going on: we collect the iterator elements into a slice and then apply a function to the slice. - It adds comments, making clear that all this is all there just to provide an optimized version of `f(&iter.collect::<Vec<_>>())`. It took me a couple of attempts to come up with this commit. My initial attempt kept `InternAs` around, but renamed things and added comments, and I wasn't happy with it. I think this version is much better. The resulting code is shorter, despite the addition of the comments.

This makes a lot of call sites nicer.

This function has this line twice: ``` let bound_vars = tcx.intern_bound_variable_kinds(&bound_vars); ``` The second occurrence is effectively a no-op, because the first occurrence interned any that needed it.

nnethercote · 2023-02-17T11:37:26Z

I rebased.

@bors r=oli-obk,compiler-errors

bors · 2023-02-17T11:37:27Z

📌 Commit af32411 has been approved by oli-obk,compiler-errors

It is now in the queue for this repository.

bors · 2023-02-18T00:20:55Z

⌛ Testing commit af32411 with merge a9842c7...

bors · 2023-02-18T03:05:04Z

☀️ Test successful - checks-actions
Approved by: oli-obk,compiler-errors
Pushing a9842c7 to master...

rust-timer · 2023-02-18T04:58:03Z

Finished benchmarking commit (a9842c7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	3.3%	[2.1%, 4.6%]	2
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

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

…rs, r=oli-obk,compiler-errors Clarify iterator interners I found the iterator interners very confusing. This PR clarifies things. r? `@compiler-errors`

Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 Unify validity checks into a single query rust-lang/rust#108364 s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 Switch to EarlyBinder for type_of query rust-lang/rust#107753 Rename interner funcs rust-lang/rust#108250 Optimize mk_region rust-lang/rust#108020 Clarify iterator interners rust-lang/rust#108112

- Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 - Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112

- Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112

rustbot assigned compiler-errors Feb 16, 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Feb 16, 2023

compiler-errors reviewed Feb 16, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/check.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the clarify-iterator-interners branch from 42297c8 to 72935cb Compare February 16, 2023 05:43

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 16, 2023

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

oli-obk approved these changes Feb 16, 2023

View reviewed changes

compiler/rustc_trait_selection/src/solve/trait_goals.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the clarify-iterator-interners branch from 72935cb to 9922e08 Compare February 16, 2023 21:29

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2023

ckxkexing mentioned this pull request Feb 17, 2023

Rollup of 3 pull requests #108153

Closed

nnethercote added 7 commits February 17, 2023 22:24

Clarify mk_fn_sig signature.

28184e7

Giving the item type a name `T` avoids duplication.

Use IntoIterator for mk_fn_sig.

2017aef

This makes a lot of call sites nicer.

Replace more mk_foo calls with infer_foo.

107f14d

Avoid double-interning some BoundVariableKinds.

af32411

This function has this line twice: ``` let bound_vars = tcx.intern_bound_variable_kinds(&bound_vars); ``` The second occurrence is effectively a no-op, because the first occurrence interned any that needed it.

nnethercote force-pushed the clarify-iterator-interners branch from 9922e08 to af32411 Compare February 17, 2023 11:26

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 18, 2023

bors merged commit a9842c7 into rust-lang:master Feb 18, 2023

rustbot added this to the 1.69.0 milestone Feb 18, 2023

bors mentioned this pull request Feb 18, 2023

Add cross-language LLVM CFI support to the Rust compiler #105452

Merged

nnethercote deleted the clarify-iterator-interners branch February 19, 2023 21:39

bjorn3 mentioned this pull request Feb 20, 2023

Rename interner funcs #108250

Merged

qinheping mentioned this pull request Mar 10, 2023

Upgrade toolchain to nightly-2023-03-09 model-checking/kani#2293

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify iterator interners #108112

Clarify iterator interners #108112

nnethercote commented Feb 16, 2023

rustbot commented Feb 16, 2023

nnethercote commented Feb 16, 2023

compiler-errors left a comment

nnethercote commented Feb 16, 2023

compiler-errors commented Feb 16, 2023

rustbot commented Feb 16, 2023

nnethercote commented Feb 16, 2023

oli-obk commented Feb 16, 2023

This comment has been minimized.

bors commented Feb 16, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

This comment has been minimized.

rust-timer commented Feb 16, 2023

oli-obk left a comment

nnethercote commented Feb 16, 2023

bors commented Feb 16, 2023

oli-obk commented Feb 17, 2023

nnethercote commented Feb 17, 2023

bors commented Feb 17, 2023

bors commented Feb 18, 2023

bors commented Feb 18, 2023

rust-timer commented Feb 18, 2023

Clarify iterator interners #108112

Clarify iterator interners #108112

Conversation

nnethercote commented Feb 16, 2023

rustbot commented Feb 16, 2023

nnethercote commented Feb 16, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

nnethercote commented Feb 16, 2023

compiler-errors commented Feb 16, 2023

rustbot commented Feb 16, 2023

nnethercote commented Feb 16, 2023

oli-obk commented Feb 16, 2023

This comment has been minimized.

bors commented Feb 16, 2023

bors commented Feb 16, 2023

bors commented Feb 16, 2023

This comment has been minimized.

rust-timer commented Feb 16, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

oli-obk left a comment

Choose a reason for hiding this comment

nnethercote commented Feb 16, 2023

bors commented Feb 16, 2023

oli-obk commented Feb 17, 2023

nnethercote commented Feb 17, 2023

bors commented Feb 17, 2023

bors commented Feb 18, 2023

bors commented Feb 18, 2023

rust-timer commented Feb 18, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles