Fix perf regression caused by tracing

[Stypox]

See #143334, this is another alternative that may be worth benchmarking as suggested in #143334 (comment).

r? @RalfJung

[rustbot]

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[Kobzol]

@bors2 try @rust-timer queue

[rust-bors]

⌛ Trying commit 57aa88e with merge 3ab9e25…

To cancel the try build, run the command @bors2 try cancel.

[RalfJung]

No, this is not the closure I meant. I don't think this one will help.

What I meant is an API of the form

fn with_trace_span<R>(_span: tracing::Span, f: impl FnOnce() -> R) -> R { f() }

I.e., the span ends when the closure returns.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3ab9e25 (3ab9e2528f3d5f3f77694dc747bed001f44bf378, parent: febb10d0a2d29278135676783f6a22eb83295981)

[rust-timer]

Finished benchmarking commit (3ab9e25): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.4%	[0.1%, 0.8%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-1.1%, -0.2%]	15
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.4%)

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

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-1.6%, 0.9%]	2

Cycles

Results (primary -1.3%)

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

Binary size

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

Bootstrap: 460.756s -> 460.961s (0.04%)
Artifact size: 372.14 MiB -> 372.24 MiB (0.03%)

[RalfJung]

Ah, nice find! My prediction was wrong. :)
This does seem quite a bit better than the other approach.

[bors]

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

[RalfJung]

@Stypox Do you think it is worth trying this API? I assume not, so I'd be fine landing this as-is once the conflicts are resolved.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[Stypox]

Yeah I also think it's not worth it, since it takes the span by value I would expect it to behave like point 4 in my analysis. I rebased and implemented enter_trace_span for MiriMachine because in the meantime the Miri tree was updated.

@rustbot ready

[RalfJung]

I meant a variant of that that takes the span in a closure, since you demonstrated that that is indeed necessary.

But, I doubt it'll help so let's just
@bors r+ rollup

[bors]

📌 Commit e5f7d4d has been approved by RalfJung

It is now in the queue for this repository.

[Kobzol]

@rust-timer build 45ab0f5

Checking that the small regression from #143667 is caused by this.

[RalfJung]

Ah damn I shouldn't have marked this as rollup, sorry for that.

[rust-timer]

Finished benchmarking commit (45ab0f5): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.0%, 0.2%]	20
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.9%, -0.0%]	5
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.0%, secondary 3.3%)

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

	mean	range	count
Regressions ❌ (primary)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	4.2%	[1.2%, 7.0%]	14
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.4%]	2
All ❌✅ (primary)	-1.0%	[-3.4%, 1.4%]	2

Cycles

Results (primary -2.6%, secondary -3.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)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-2.6%	[-3.0%, -2.1%]	4
Improvements ✅ (secondary)	-4.3%	[-10.3%, -2.0%]	7
All ❌✅ (primary)	-2.6%	[-3.0%, -2.1%]	4

Binary size

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

Bootstrap: 465.667s -> 464.844s (-0.18%)
Artifact size: 372.26 MiB -> 374.50 MiB (0.60%)

[RalfJung]

Hm, that looks somewhat different from the last perf run we did.

I also have no idea how anything can be slowed down by this, so it might just be a bit of random inliner noise...

[Kobzol]

The regression isn't that bad, but since this PR calimed to "fix perf regression", and it seemingly is either the same or slightly worse, maybe we should revert it?

[RalfJung]

It helps with const-stress, and arguably it makes a lot more sense to put this span logic into a closure. Previous measurements came back slightly green in the median, this one is slightly red. I am inclined to keep it.

[Kobzol]

Sorry, I missed the CTFE benchmark. Ok, let's keep it, marking the rollup as triaged.

[Kobzol]

@rustbot label: +perf-regression-triaged