Fix performance issue in liveness checking

[chenyukang]

r? @ghost

from #150955 (comment)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[chenyukang]

@bors try @rust-timer queue

[lqd]

Cool. r=me with green perf

View changes since this review

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3a8fd6c (3a8fd6ccca553ed0605e839cc9eac17a3aa80166, parent: d940e56841ddcc05671ead99290e35ff2e98369f)

[rust-timer]

Finished benchmarking commit (3a8fd6c): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@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.2%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (secondary -2.5%)

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

Binary size

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

Bootstrap: 473.058s -> 474.35s (0.27%)
Artifact size: 383.29 MiB -> 383.32 MiB (0.01%)

[chenyukang]

@bors r=lqd

[rust-bors]

📌 Commit 54f9a44 has been approved by lqd

It is now in the queue for this repository.

[lqd]

It didn’t fix the issue you were trying to fix, right?

[chenyukang]

It didn’t fix the issue you were trying to fix, right?

seems that PR #150955 don't have that big performance regression:
#150955 (comment)

[lqd]

It doesn't indeed.

[lqd]

In any case, this new code is in a good spot now (but it's also not clear that it was causing much in the previous place) so let's land it.

[panstromek]

This is inside the loop, so this lookup can be done multiple times after this change, right? Could that be the issue?

For positive cases we can also just break, too.

Suggested change

	// By convention, underscore-prefixed bindings are explicitly allowed to be unused.
	if name.as_str().starts_with('_') {
	continue;
	}
	// By convention, underscore-prefixed bindings are explicitly allowed to be unused.
	if name.as_str().starts_with('_') {
	break;
	}

[lqd]

I saw that but the only way this would matter is if the benchmarks code emits unused warnings today, and we'd be thus benchmarking diagnostics?

[panstromek]

Hmm, right. I guess I'm just perplexed by the results. For code that never emitted these warnings, this PR should be strictly less work, because they never reach this branch anyway, but instead it's a regression. That doesn't make any sense to me. At least it should undo the previous regression in unicode-normalization, but those benchmarks didn't move above the treshold, or slightly regressed, if anything (I'm assuming unicode-normalization doesn't emit unused-assignemnts).

[lqd]

were you able to check @chenyukang? break makes sense to us, but the results are also strange: not knowing whether they impact benchmarks unexpectedly due to diagnostics.

[chenyukang]

yeah, seems better, updated the code.

[lqd]

what did you learn, are we unexpectedly looking at codepaths that are only exercised through diagnostics emitted by the benchmarks?

[chenyukang]

break seems more efficient in theory, but no performance improvement in practice, i think diagnostics emitting is general not in hotpath, don't understand why #150955 (comment) happened, this PR should make sure name.as_str().starts_with('_') only executed when there is an error, but #151376 (comment) makes no change.

[rust-bors]

💔 Test for 399f6ba failed: CI. Failed job:

• auto - x86_64-gnu-aux (web logs, enhanced plaintext logs)

[chenyukang]

There error is cannot open file descriptor 3 from the jobserver environment variable value: Bad file descriptor (os error 9), seems unrelated to my change.

@bors try jobs=x86_64-gnu-aux

[chenyukang]

anyway, let's run a perf again

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 98437b3 (98437b3d28233c3a5d7a8d07f1459ed8ea5b1068, parent: ef2657cbaf6885b71e5ffd277ffe72a2616c0a7c)

[rust-timer]

Finished benchmarking commit (98437b3): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 545.612s -> 476.413s (-12.68%)
Artifact size: 397.86 MiB -> 397.86 MiB (0.00%)

[lqd]

Great, no noise, @bors r+ rollup

[rust-bors]

📌 Commit d2aa64f has been approved by lqd

It is now in the queue for this repository.