Rollup of 8 pull requests #120843

matthiaskrgr · 2024-02-09T13:41:53Z

Successful merges:

Make privacy visitor use types more (instead of HIR) #113671 (Make privacy visitor use types more (instead of HIR))
core/time: avoid divisions in Duration::new #120308 (core/time: avoid divisions in Duration::new)
Invert diagnostic lints. #120693 (Invert diagnostic lints.)
A drive-by rewrite of give_region_a_name() #120704 (A drive-by rewrite of give_region_a_name())
Use transmute_unchecked in NonZero::new. #120809 (Use transmute_unchecked in NonZero::new.)
Fix more ty::Error ICEs in MIR passes #120817 (Fix more ty::Error ICEs in MIR passes)
Fix ErrorGuaranteed unsoundness with stash/steal. #120828 (Fix ErrorGuaranteed unsoundness with stash/steal.)
Startup objects disappearing from sysroot #120831 (Startup objects disappearing from sysroot)

r? @ghost
@rustbot modify labels: rollup

That is, change `diagnostic_outside_of_impl` and `untranslatable_diagnostic` from `allow` to `deny`, because more than half of the compiler has be converted to use translated diagnostics. This commit removes more `deny` attributes than it adds `allow` attributes, which proves that this change is warranted.

This rewrite makes the cache-updating nature of the function slightly clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows the map once, but in this context the performance impact is almost certainly completely negligible.

…visit This prevents ICEs from happening in the future when this code path is actually used

…ariants

When you stash an error, the error count is incremented. You can then use the non-zero error count to get an `ErrorGuaranteed`. You can then steal the error, which decrements the error count. You can then cancel the error. Example code: ``` fn unsound(dcx: &DiagCtxt) -> ErrorGuaranteed { let sp = rustc_span::DUMMY_SP; let k = rustc_errors::StashKey::Cycle; dcx.struct_err("bogus").stash(sp, k); // increment error count on stash let guar = dcx.has_errors().unwrap(); // ErrorGuaranteed from error count > 0 let err = dcx.steal_diagnostic(sp, k).unwrap(); // decrement error count on steal err.cancel(); // cancel error guar // ErrorGuaranteed with no error emitted! } ``` This commit fixes the problem in the simplest way: by not counting stashed errors in `DiagCtxt::{err_count,has_errors}`. However, just doing this without any other changes leads to over 40 ui test failures. Mostly because of uninteresting extra errors (many saying "type annotations needed" when type inference fails), and in a few cases, due to delayed bugs causing ICEs when no normal errors are printed. To fix these, this commit adds `DiagCtxt::stashed_err_count`, and uses it in three places alongside `DiagCtxt::{has_errors,err_count}`. It's dodgy to rely on it, because unlike `DiagCtxt::err_count` it can go up and down. But it's needed to preserve existing behaviour, and at least the three places that need it are now obvious.

When launching tests with --keep-stage option, startup objects such as rsbegin.o an rsend.o may disappear from the corresponding stageN compiler. Fix issue rust-lang#120784

…ochenkov Make privacy visitor use types more (instead of HIR) r? ``@petrochenkov`` This is a prerequisite to normalizing projections, as otherwise we have too many invalid bound vars (hir_ty_to_ty is creating types that have bound vars, but no binder). The commits are still chaotic, I'm gonna clean them up, but I just wanted to let you know about the general direction and wondering if we could land this before adding normalization, as normalization is where behavioral changes happen, and I'd like to keep that part as minimal as possible. [context can be found on zulip](https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/weak.20type.20aliases.20and.20privacy)

…ou-se core/time: avoid divisions in Duration::new In our (decently large) code base, we use `SystemTime::UNIX_EPOCH.elapsed()` in a lot of places & often in a loop or in the hot path. On [Unix](https://github.com/rust-lang/rust/blob/1.75.0/library/std/src/sys/unix/time.rs#L153-L162) at least, it seems we do calculations before hand to ensure that nanos is within the valid range, yet `Duration::new()` still checks it again, using 2 divisions. It seems like adding a branch can make this function 33% faster on ARM64 in the cases where nanos is already in the valid range & seems to have no effect in the other case. Benchmarks: M1 Pro (14-inch base model): ``` duration/current/checked time: [1.5945 ns 1.6167 ns 1.6407 ns] Found 5 outliers among 100 measurements (5.00%) 2 (2.00%) high mild 3 (3.00%) high severe duration/current/unchecked time: [1.5941 ns 1.6051 ns 1.6179 ns] Found 2 outliers among 100 measurements (2.00%) 1 (1.00%) high mild 1 (1.00%) high severe duration/branched/checked time: [1.1997 ns 1.2048 ns 1.2104 ns] Found 8 outliers among 100 measurements (8.00%) 4 (4.00%) high mild 4 (4.00%) high severe duration/branched/unchecked time: [1.5881 ns 1.5957 ns 1.6039 ns] Found 6 outliers among 100 measurements (6.00%) 3 (3.00%) high mild 3 (3.00%) high severe ``` EC2 c7gd.16xlarge (Graviton 3): ``` duration/current/checked time: [2.7996 ns 2.8000 ns 2.8003 ns] Found 5 outliers among 100 measurements (5.00%) 2 (2.00%) low severe 3 (3.00%) low mild duration/current/unchecked time: [2.9922 ns 2.9925 ns 2.9928 ns] Found 7 outliers among 100 measurements (7.00%) 4 (4.00%) low severe 1 (1.00%) low mild 2 (2.00%) high mild duration/branched/checked time: [2.0830 ns 2.0843 ns 2.0857 ns] Found 3 outliers among 100 measurements (3.00%) 1 (1.00%) low severe 1 (1.00%) low mild 1 (1.00%) high mild duration/branched/unchecked time: [2.9879 ns 2.9886 ns 2.9893 ns] Found 5 outliers among 100 measurements (5.00%) 3 (3.00%) low severe 2 (2.00%) low mild ``` EC2 r7iz.16xlarge (Intel Xeon Scalable-based (Sapphire Rapids)): ``` duration/current/checked time: [980.60 ps 980.79 ps 980.99 ps] Found 10 outliers among 100 measurements (10.00%) 4 (4.00%) low severe 2 (2.00%) low mild 3 (3.00%) high mild 1 (1.00%) high severe duration/current/unchecked time: [979.53 ps 979.74 ps 979.96 ps] Found 6 outliers among 100 measurements (6.00%) 2 (2.00%) low severe 1 (1.00%) low mild 2 (2.00%) high mild 1 (1.00%) high severe duration/branched/checked time: [938.72 ps 938.96 ps 939.22 ps] Found 4 outliers among 100 measurements (4.00%) 1 (1.00%) low mild 1 (1.00%) high mild 2 (2.00%) high severe duration/branched/unchecked time: [1.0103 ns 1.0110 ns 1.0118 ns] Found 10 outliers among 100 measurements (10.00%) 2 (2.00%) low mild 7 (7.00%) high mild 1 (1.00%) high severe ``` Bench code (ran using stable 1.75.0 & criterion latest 0.5.1): I couldn't find any benches for `Duration` in this repo, so I just copied the relevant types & recreated it. ```rust use criterion::{black_box, criterion_group, criterion_main, Criterion}; pub fn duration_bench(c: &mut Criterion) { const NANOS_PER_SEC: u32 = 1_000_000_000; #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] #[repr(transparent)] struct Nanoseconds(u32); impl Default for Nanoseconds { #[inline] fn default() -> Self { // SAFETY: 0 is within the valid range unsafe { Nanoseconds(0) } } } #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] pub struct Duration { secs: u64, nanos: Nanoseconds, // Always 0 <= nanos < NANOS_PER_SEC } impl Duration { #[inline] pub const fn new_current(secs: u64, nanos: u32) -> Duration { let secs = match secs.checked_add((nanos / NANOS_PER_SEC) as u64) { Some(secs) => secs, None => panic!("overflow in Duration::new"), }; let nanos = nanos % NANOS_PER_SEC; // SAFETY: nanos % NANOS_PER_SEC < NANOS_PER_SEC, therefore nanos is within the valid range Duration { secs, nanos: unsafe { Nanoseconds(nanos) } } } #[inline] pub const fn new_branched(secs: u64, nanos: u32) -> Duration { if nanos < NANOS_PER_SEC { // SAFETY: nanos < NANOS_PER_SEC, therefore nanos is within the valid range Duration { secs, nanos: unsafe { Nanoseconds(nanos) } } } else { let secs = match secs.checked_add((nanos / NANOS_PER_SEC) as u64) { Some(secs) => secs, None => panic!("overflow in Duration::new"), }; let nanos = nanos % NANOS_PER_SEC; // SAFETY: nanos % NANOS_PER_SEC < NANOS_PER_SEC, therefore nanos is within the valid range Duration { secs, nanos: unsafe { Nanoseconds(nanos) } } } } } let mut group = c.benchmark_group("duration/current"); group.bench_function("checked", |b| { b.iter(|| black_box(Duration::new_current(black_box(1_000_000_000), black_box(1_000_000)))); }); group.bench_function("unchecked", |b| { b.iter(|| { black_box(Duration::new_current(black_box(1_000_000_000), black_box(2_000_000_000))) }); }); drop(group); let mut group = c.benchmark_group("duration/branched"); group.bench_function("checked", |b| { b.iter(|| { black_box(Duration::new_branched(black_box(1_000_000_000), black_box(1_000_000))) }); }); group.bench_function("unchecked", |b| { b.iter(|| { black_box(Duration::new_branched(black_box(1_000_000_000), black_box(2_000_000_000))) }); }); } criterion_group!(duration_benches, duration_bench); criterion_main!(duration_benches); ```

…s, r=davidtwco Invert diagnostic lints. That is, change `diagnostic_outside_of_impl` and `untranslatable_diagnostic` from `allow` to `deny`, because more than half of the compiler has been converted to use translated diagnostics. This commit removes more `deny` attributes than it adds `allow` attributes, which proves that this change is warranted. r? ````@davidtwco````

…write, r=compiler-errors A drive-by rewrite of `give_region_a_name()` This drive-by rewrite makes the cache-updating nature of the method clearer, using the Entry API into the hash table for region names to capture the update-insert nature of the method. May be marginally more efficient since it only runtime-borrows and indexes the map once, but in this context the performance impact is almost certainly completely negligible. Note that this commit should preserve all externally visible behaviour. Notably, it preserves the debug logging: 1. printing even in the case of a `None` for the new computed name, and 2. only printing on new values, begin silent on reused values

…ructors, r=Nilstrieb Use `transmute_unchecked` in `NonZero::new`. Tracking issue: rust-lang#120257 See rust-lang#120521 (comment).

…=oli-obk Fix more `ty::Error` ICEs in MIR passes Fixes rust-lang#120791 - Add a check for `ty::Error` in the `ByMove` coroutine pass Fixes rust-lang#120816 - Add a check for `ty::Error` in the MIR validator Also a drive-by fix for a FIXME I had asked oli to add r? oli-obk

…-obk Fix `ErrorGuaranteed` unsoundness with stash/steal. When you stash an error, the error count is incremented. You can then use the non-zero error count to get an `ErrorGuaranteed`. You can then steal the error, which decrements the error count. You can then cancel the error. Example code: ``` fn unsound(dcx: &DiagCtxt) -> ErrorGuaranteed { let sp = rustc_span::DUMMY_SP; let k = rustc_errors::StashKey::Cycle; dcx.struct_err("bogus").stash(sp, k); // increment error count on stash let guar = dcx.has_errors().unwrap(); // ErrorGuaranteed from error count > 0 let err = dcx.steal_diagnostic(sp, k).unwrap(); // decrement error count on steal err.cancel(); // cancel error guar // ErrorGuaranteed with no error emitted! } ``` This commit fixes the problem in the simplest way: by not counting stashed errors in `DiagCtxt::{err_count,has_errors}`. However, just doing this without any other changes leads to over 40 ui test failures. Mostly because of uninteresting extra errors (many saying "type annotations needed" when type inference fails), and in a few cases, due to delayed bugs causing ICEs when no normal errors are printed. To fix these, this commit adds `DiagCtxt::stashed_err_count`, and uses it in three places alongside `DiagCtxt::{has_errors,err_count}`. It's dodgy to rely on it, because unlike `DiagCtxt::err_count` it can go up and down. But it's needed to preserve existing behaviour, and at least the three places that need it are now obvious. r? oli-obk

…objects, r=onur-ozkan Startup objects disappearing from sysroot When launching tests with --keep-stage option, startup objects such as rsbegin.o an rsend.o may disappear from the corresponding stageN compiler. Fix issue rust-lang#120784

matthiaskrgr · 2024-02-09T13:42:06Z

@bors r+ rollup=never p=8

bors · 2024-02-09T13:42:08Z

📌 Commit 4a46914 has been approved by matthiaskrgr

It is now in the queue for this repository.

bors · 2024-02-09T15:34:52Z

⌛ Testing commit 4a46914 with merge e28fae5...

bors · 2024-02-09T18:07:55Z

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing e28fae5 to master...

rust-timer · 2024-02-09T18:10:09Z

📌 Perf builds for each rolled up PR:

PR#	Message	Perf Build Sha
#113671	Make privacy visitor use types more (instead of HIR)	`32747815c40aa3d165e7b0fa070a24b2066edf40` (link)
#120308	core/time: avoid divisions in Duration::new	`a804e9e32051107246434fc395611a740d9361b9` (link)
#120693	Invert diagnostic lints.	`e657b0c549128ea1ab4ff86582bbf2fe4f347c72` (link)
#120704	A drive-by rewrite of `give_region_a_name()`	`76c0538511496d8a0ceaf17c5ac53a70b559344b` (link)
#120809	Use `transmute_unchecked` in `NonZero::new`.	`14762173dd90ee8541780c279b95eaf0ab370cb3` (link)
#120817	Fix more `ty::Error` ICEs in MIR passes	`929e98308d967fed2bb6ef56a61c17f9a33c7e63` (link)
#120828	Fix `ErrorGuaranteed` unsoundness with stash/steal.	`d2ca6c5833e51dcc58a2206051435061b70c98c5` (link)
#120831	Startup objects disappearing from sysroot	`59f826fca85d104adca4bd534a8875dc7c0f5cd9` (link)

previous master: 8fb67fb37f

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

rust-timer · 2024-02-09T19:34:38Z

Finished benchmarking commit (e28fae5): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.6%	[0.2%, 1.0%]	11
Improvements ✅ (primary)	-0.5%	[-1.0%, -0.2%]	41
Improvements ✅ (secondary)	-0.8%	[-2.4%, -0.2%]	17
All ❌✅ (primary)	-0.5%	[-1.0%, 0.2%]	42

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)	5.7%	[0.8%, 10.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-9.1%	[-14.5%, -5.7%]	3
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	-3.1%	[-14.5%, 10.7%]	5

Cycles

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

Binary size

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.1%	[0.0%, 0.4%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.5%, -0.0%]	25
Improvements ✅ (secondary)	-0.9%	[-1.3%, -0.1%]	4
All ❌✅ (primary)	-0.0%	[-0.5%, 0.4%]	31

Bootstrap: 663.818s -> 666.509s (0.41%)
Artifact size: 308.01 MiB -> 307.99 MiB (-0.01%)

scottmcm · 2024-02-09T20:41:09Z

Kicked a perf run on #120809 (comment)

Kobzol · 2024-02-13T07:50:23Z

Wins outweight the regressions here.

@rustbot label: +perf-regression-triaged

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113671 (Make privacy visitor use types more (instead of HIR)) - rust-lang#120308 (core/time: avoid divisions in Duration::new) - rust-lang#120693 (Invert diagnostic lints.) - rust-lang#120704 (A drive-by rewrite of `give_region_a_name()`) - rust-lang#120809 (Use `transmute_unchecked` in `NonZero::new`.) - rust-lang#120817 (Fix more `ty::Error` ICEs in MIR passes) - rust-lang#120828 (Fix `ErrorGuaranteed` unsoundness with stash/steal.) - rust-lang#120831 (Startup objects disappearing from sysroot) r? `@ghost` `@rustbot` modify labels: rollup

utkarshgupta137 and others added 24 commits January 24, 2024 11:10

std/time: avoid divisions in Duration::new

8a850cd

Associated types in traits don't necessarily have a type that we can …

0f323b2

…visit This prevents ICEs from happening in the future when this code path is actually used

extern types don't have any types to visit

0b97d18

Actually walk fields of Adt definitions

b998b51

Stop using hir_ty_to_ty in rustc_privacy

4389a1c

Remove now-useless method override

5c25de6

Remove dead code

e867886

Make RegionName Copy by (transitively) interning the few string v…

795be51

…ariants

Add some tests for associated type normalization edge cases

d80d7ea

Use transmute_unchecked in NonZero::new.

d70d320

Don't ICE in ByMoveBody when coroutine is tainted

698a3c7

Don't ice in validation when error body is created

e32c1dd

Startup objects disappearing from sysroot

575e0aa

When launching tests with --keep-stage option, startup objects such as rsbegin.o an rsend.o may disappear from the corresponding stageN compiler. Fix issue rust-lang#120784

Rollup merge of rust-lang#120809 - reitermarkus:generic-nonzero-const…

475c47a

…ructors, r=Nilstrieb Use `transmute_unchecked` in `NonZero::new`. Tracking issue: rust-lang#120257 See rust-lang#120521 (comment).

rustbot added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Feb 9, 2024

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 9, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2024

bors merged commit e28fae5 into rust-lang:master Feb 9, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 9, 2024

rustbot added the perf-regression Performance regression. label Feb 9, 2024

rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 13, 2024

matthiaskrgr deleted the rollup-med37z5 branch March 16, 2024 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup of 8 pull requests #120843

Rollup of 8 pull requests #120843

matthiaskrgr commented Feb 9, 2024

matthiaskrgr commented Feb 9, 2024

bors commented Feb 9, 2024

bors commented Feb 9, 2024

bors commented Feb 9, 2024

rust-timer commented Feb 9, 2024

rust-timer commented Feb 9, 2024

scottmcm commented Feb 9, 2024

Kobzol commented Feb 13, 2024

Rollup of 8 pull requests #120843

Rollup of 8 pull requests #120843

Conversation

matthiaskrgr commented Feb 9, 2024

matthiaskrgr commented Feb 9, 2024

bors commented Feb 9, 2024

bors commented Feb 9, 2024

bors commented Feb 9, 2024

rust-timer commented Feb 9, 2024

rust-timer commented Feb 9, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

scottmcm commented Feb 9, 2024

Kobzol commented Feb 13, 2024