[Experiment] Box diagnostic_metadata field

[TaKO8Ki]

closes #97954

r? @estebank

[TaKO8Ki]

@bors try @rust-timer queue

[rust-timer]

Insufficient permissions to issue commands to rust-timer.

[bors]

@TaKO8Ki: 🔑 Insufficient privileges: not in try users

[TaKO8Ki]

I don't have permission to run perf.

[eggyal]

This is exactly what I had thought the issue was saying, yet it surely only adds the cost of additional heap allocs? I'm sure I must be missing something, as I can't see how there could be any benefit from it?

[joshtriplett]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 5ece481 with merge e5e645fd772153e58de155a806285c5cef5d3428...

[bors]

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

[rust-timer]

Queued e5e645fd772153e58de155a806285c5cef5d3428 with parent 2d1e075, future comparison URL.

[rust-timer]

Finished benchmarking commit (e5e645fd772153e58de155a806285c5cef5d3428): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-5.4%	-5.4%	1
Improvements 🎉 (secondary)	-3.4%	-4.5%	2
All 😿🎉 (primary)	-5.4%	-5.4%	1

Cycles

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

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩

2. number of relevant changes ↩

[joshtriplett]

@TaKO8Ki Very nice!

[eggyal]

Really surprised by this... can someone explain it?

[Kobzol]

In general, boxing an attribute moves it to the heap, so you pay the cost for allocating (both in executed instructions and extra memory usage). But on the other hand, it can reduce the size of the structure that contains the attribute (e.g. if a field has 64 bytes and you box it, the size gets reduced to 8 bytes), which in turn can improve performance (less data loaded from memory, better cache utilization etc.).

That being said, I'm not really seeing any great improvements in the perf. results here. Instruction counts and cycles are a wash and Max RSS was improved only in three cases with quite low significance factor (on average it's just -0.24% improvement). So I'm not sure if this is worth it.

[eggyal]

But on the other hand, it can reduce the size of the structure that contains the attribute (e.g. if a field has 64 bytes and you box it, the size gets reduced to 8 bytes), which in turn can improve performance (less data loaded from memory, better cache utilization etc.).

Indeed, but this particular structure is quite large already so the reduction in its stack size shouldn't have a material impact. Moreover, the identified change is to total memory consumption: yet the heap allocs are made on object creation and maintained until object destruction so total memory should in fact have increased (by the size of the pointer).

Anyway, thank you for your thoughts! I'll leave it at that; I just found the idea of this experiment, and it's outcome, rather surprising/unintuitive.

[estebank]

this particular structure is quite large already

Is it? The only things that stand out to me as "potentially big" are ParentScope and PerNS, all other fields are in the heap, in one way or another (Vec, &, HashMap).

rust/compiler/rustc_resolve/src/late.rs

Lines 521 to 552 in 5ece481

	struct LateResolutionVisitor<'a, 'b, 'ast> {
	r: &'b mut Resolver<'a>,
	/// The module that represents the current item scope.
	parent_scope: ParentScope<'a>,
	/// The current set of local scopes for types and values.
	/// FIXME #4948: Reuse ribs to avoid allocation.
	ribs: PerNS<Vec<Rib<'a>>>,
	/// The current set of local scopes, for labels.
	label_ribs: Vec<Rib<'a, NodeId>>,
	/// The current set of local scopes for lifetimes.
	lifetime_ribs: Vec<LifetimeRib>,
	/// The trait that the current context can refer to.
	current_trait_ref: Option<(Module<'a>, TraitRef)>,
	/// Fields used to add information to diagnostic errors.
	diagnostic_metadata: Box<DiagnosticMetadata<'ast>>,
	/// State used to know whether to ignore resolution errors for function bodies.
	///
	/// In particular, rustdoc uses this to avoid giving errors for `cfg()` items.
	/// In most cases this will be `None`, in which case errors will always be reported.
	/// If it is `true`, then it will be updated when entering a nested function or trait body.
	in_func_body: bool,
	/// Count the number of places a lifetime is used.
	lifetime_uses: FxHashMap<LocalDefId, LifetimeUseSet>,
	}

That being said, given how resolution behaves, without needing to clone or be moved too much, I guess it's not surprising this didn't have significant perf impacts, beyond the margins. That being said, I can go either on whether we should merge this or not, particularly given the patch size :)

[estebank]

@bors r+

[bors]

📌 Commit 5ece481 has been approved by estebank

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5ece481 with merge 96c2df8...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 96c2df8 to master...

[rust-timer]

Finished benchmarking commit (96c2df8): comparison url.

Instruction count

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.4%	0.4%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	3.0%	3.0%	1
Regressions 😿 (secondary)	3.0%	3.5%	3
Improvements 🎉 (primary)	-2.5%	-2.5%	1
Improvements 🎉 (secondary)	-3.3%	-3.6%	2
All 😿🎉 (primary)	0.2%	3.0%	2

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

	mean1	max	count2
Regressions 😿 (primary)	2.9%	3.5%	2
Regressions 😿 (secondary)	2.9%	3.0%	2
Improvements 🎉 (primary)	-2.1%	-2.1%	1
Improvements 🎉 (secondary)	-4.2%	-4.2%	1
All 😿🎉 (primary)	1.2%	3.5%	3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³