Avoid TrustedLen specialization that optimize poorly in collect_remaining_errors

[panstromek]

View all comments

see #160048

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4bd0be6 (4bd0be6d9bb9580aab29eed8cfcd3ba5e095d874)
Base parent: e19d321 (e19d321c06479c6fd77533582b0d5a86651f1be3)

[rust-timer]

Finished benchmarking commit (4bd0be6): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-2.6%, -0.1%]	19
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.1%, secondary -2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-2.9%	[-5.7%, -0.8%]	3
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Cycles

Results (primary -1.5%, secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.3%	[3.0%, 3.6%]	2
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-4.1%	[-6.2%, -2.4%]	7
All ❌✅ (primary)	-1.5%	[-1.5%, -1.5%]	1

Binary size

Results (secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 490.165s -> 488.452s (-0.35%)
Artifact size: 388.34 MiB -> 390.43 MiB (0.54%)

[panstromek]

Yea, the issue is still there even after #160005.

r? @nnethercote

Atm I don't know if we will be able to fix the TrustedLen specialization problem in general some time soon. This workaround is a bit funky, but it works for this case and it doesn't seem like we hit this issue in many other places in the compiler anyway.

[rustbot]

Some changes occurred to the core trait solver

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

[lqd]

Is this an error codepath? Why are we hitting it that much in the benchmarks?

[Kobzol]

Probably because our benchmarks are relatively old crates (anything gets old quickly in Rust unless you regularly update it), and they are generating thousands of warnings, FCWs, etc.

Edit: wait, this looks like an actual trait error codepath? Not sure if that is relevant to what I wrote above; if not, then nevermind.

[lqd]

It seems that way although I haven't checked. But if there's anything in there it looks like a bad time heh. I'm mostly pointing this out so we know what we're optimizing, if it's worth it, and the opposite (the regressions we could sometimes ignore in the other PRs working with ThinVec).

[panstromek]

I'm not sure, I assumed it's one of those things where we try to solve something multiple ways until it succeeds, so the error happens as a part of normal execution path.

But if the list is always empty, then it'd kinda make sense that an iterator that doesn't look at size_hint and just calls next a single time and return empty vec would maybe optimize better?

[panstromek]

It is indeed almost always empty, but sometimes not:

$ cat results/eprintln-Id-syn-2.0.101-new-solver-Check-Full | sort | uniq -c | sort -n 
    182 collect_remaining_errors: len=1
1057538 collect_remaining_errors: len=0

[Kobzol]

Maybe the size hint is set based on capacity or something? That is weird, I would expect that if the input Vec is empty, then the size hint will also be set to 0.

Anyway, maybe instead of the skip(0) magic, creating an empty vec and then iterating through the input obligations and pushing to the Vec would be easier to understand? With a comment that says that we don't want to allocate the vector there unnecesarily, because it is usually empty.

[the8472]

But if the list is always empty, then it'd kinda make sense that an iterator that doesn't look at size_hint and just calls next a single time and return empty vec would maybe optimize better?

Maybe. Trustedlen is, well, trusted, so perhaps we can shortcircuit on 0 there too. OTOH that's an extra branch, so it'll need benchmarking.

[panstromek]

Maybe the size hint is set based on capacity or something

It should be exactly the length of the iterator (and therefore vec.len()), that's the contract of TrustedLen.

But to clarify what I was talking about, the non-specialized impl first calls next() and early returns Vec::new() for None case, while the specialized impl first calls size_hint, then Vec::with_capacity(..), then SpecExtend::extend, which calls the size_hint() again and then a bunch more setup before it delegates to iter.for_each(). That's a bunch more code to churn through in the empty case (at least by vočko).

They are right next to each other here: https://github.com/rust-lang/rust/blob/main/library/alloc/src/vec/spec_from_iter_nested.rs

But yea, I might actually just add if len == 0 check at the start of collect_remaining_errors and outline the rest, that might be better than doing anything fancy.

[the8472]

They are right next to each other here:

Heh, yes and no. Those are the FromIterator entrypoints, which then delegates two its another maze of Extend specializations, which bottom out here, also next to each other:

rust/library/alloc/src/vec/mod.rs

Lines 4099 to 4155 in e19d321

	fn extend_desugared<I: Iterator<Item = T>>(&mut self, mut iterator: I) {
	// This is the case for a general iterator.
	//
	// This function should be the moral equivalent of:
	//
	// for item in iterator {
	// self.push(item);
	// }
	while let Some(element) = iterator.next() {
	let len = self.len();
	if len == self.capacity() {
	let (lower, _) = iterator.size_hint();
	self.reserve(lower.saturating_add(1));
	}
	unsafe {
	ptr::write(self.as_mut_ptr().add(len), element);
	// Since next() executes user code which can panic we have to bump the length
	// after each step.
	// NB can't overflow since we would have had to alloc the address space
	self.set_len(len + 1);
	}
	}
	}
	// specific extend for `TrustedLen` iterators, called both by the specializations
	// and internal places where resolving specialization makes compilation slower
	#[cfg(not(no_global_oom_handling))]
	fn extend_trusted(&mut self, iterator: impl iter::TrustedLen<Item = T>) {
	let (low, high) = iterator.size_hint();
	if let Some(additional) = high {
	debug_assert_eq!(
	low,
	additional,
	"TrustedLen iterator's size hint is not exact: {:?}",
	(low, high)
	);
	self.reserve(additional);
	unsafe {
	let ptr = self.as_mut_ptr();
	let mut local_len = SetLenOnDrop::new(&mut self.len);
	iterator.for_each(move |element| {
	ptr::write(ptr.add(local_len.current_len()), element);
	// Since the loop executes user code which can panic we have to update
	// the length every step to correctly drop what we've written.
	// NB can't overflow since we would have had to alloc the address space
	local_len.increment_len(1);
	});
	}
	} else {
	// Per TrustedLen contract a `None` upper bound means that the iterator length
	// truly exceeds usize::MAX, which would eventually lead to a capacity overflow anyway.
	// Since the other branch already panics eagerly (via `reserve()`) we do the same here.
	// This avoids additional codegen for a fallback code path which would eventually
	// panic anyway.
	panic!("capacity overflow");
	}
	}

[nnethercote]

I'm fine with the skip(0), it's very simple and works well and the comment explains it.

r=me with the comment changes below.

View changes since this review

[nnethercote]

(at least by vočko)

What does this mean? Google didn't give me any answers that seem relevant :)

[panstromek]

Sorry, it means something like "guess from cursory look." or a "guesstimate." Vočko means something like a squinting eye.

[panstromek]

Thanks. I addressed the nits but I don't have r+ rights.

[nnethercote]

@bors r+

[rust-bors]

📌 Commit 51752f0 has been approved by nnethercote

It is now in the queue for this repository.