Collect VTable stats & add `-Zprint-vtable-sizes` by WaffleLapkin · Pull Request #112400 · rust-lang/rust

WaffleLapkin · 2023-06-07T18:50:42Z

This is a bit hacky/buggy, but I'm not entirely sure how to fix it, so I want to ask reviewers for help...

To try this, use either of those:

cargo clean && RUSTFLAGS="-Zprint-vtable-sizes" cargo +toolchain b
cargo clean && cargo rustc +toolchain -Zprint-vtable-sizes
rustc +toolchain -Zprint-vtable-sizes ./file.rs

rustbot · 2023-06-07T18:50:47Z

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

WaffleLapkin · 2023-06-07T18:53:20Z

r? @compiler-errors

…=Nilstrieb Don't `use compile_error as print` I've spent **1.5 hours** debugging this while trying to compile rust-lang#112400, if we use `compile_error!`, we should not just forward user input to it, but issue a reasonable error message. The better solution would be to use a lint like `clippy::print_stdout`, but since we don't have clippy in CI, let's at least make the macro error better. Also note that some functions called here actually do use `println` (see for example `print_type_sizes` function).

WaffleLapkin · 2023-06-12T16:17:41Z

So, status update:

I've fixed both problematic examples I noted above
I've also removed vtable_entries_no_resolve in favor of counting entries by hand, as suggested by @compiler-errors
I've added a skip of object-unsafe traits
I've also squashed all the fixes together

With this I think this is ready for review.

Some things we might want to fix:

I've made some things pub which maybe shouldn't be pub
Naming is somewhat off, for example size_words_without_upcasting is ehhh
There are no tests. There probably should be at least some tests.

compiler-errors · 2023-06-12T16:35:33Z

+ let existential_trait_ref = trait_ref.map_bound(|trait_ref| {
+ ty::ExistentialTraitRef::erase_self_ty(tcx, trait_ref)
+ });
+
+ // Lookup the shape of vtable for the trait.
+ let own_existential_entries =
+ tcx.own_existential_vtable_entries(existential_trait_ref.def_id());


Suggested change
let existential_trait_ref = trait_ref.map_bound(|trait_ref| {
ty::ExistentialTraitRef::erase_self_ty(tcx, trait_ref)
});
// Lookup the shape of vtable for the trait.
let own_existential_entries =
tcx.own_existential_vtable_entries(existential_trait_ref.def_id());
// Lookup the shape of vtable for the trait.
let own_existential_entries =
tcx.own_existential_vtable_entries(trait_ref.def_id());

compiler-errors · 2023-06-12T16:37:31Z

+ size_words_without_upcasting: unupcasted_cost,
+ size_words_with_upcasting: unupcasted_cost + upcast_cost,
+ difference_words: upcast_cost,
+ difference_percent: upcast_cost as f64 / unupcasted_cost as f64 * 100.,


why isnt this (size_words_without_upcasting + size_words_with_upcasting) / size_words_without_upcasting... not sure if i totally understand the math here, that could use some explanation i think 😅

nvm, I think I get it. This creates a "how much % larger is the new vtable compared to the old table". could use doc tho

compiler-errors · 2023-06-12T16:38:48Z

+ let own_entries = own_existential_entries.iter().copied().map(|_def_id| {
+ // The original code here ignores the method if its predicates are impossible.
+ // We can't really do that as, for example, all not trivial bounds on generic
+ // parameters are impossible (since we don't know the parameters...),
+ // see the comment above.
+
+ 1
+ });
+
+ unupcasted_cost += own_entries.sum::<usize>();


Can you uplift this comment and just use count 😅

WaffleLapkin · 2023-06-13T16:37:34Z

@compiler-errors I've simplified the code, added some more docs, renamed some variables to make sense and finally added a test. I think this should be ready

compiler-errors · 2023-06-13T17:55:43Z

@bors r+

bors · 2023-06-13T17:55:47Z

📌 Commit af4631a has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2023-06-14T11:24:45Z

⌛ Testing commit af4631a with merge 7b0eac4...

bors · 2023-06-14T14:20:52Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 7b0eac4 to master...

rust-timer · 2023-06-14T16:35:14Z

Finished benchmarking commit (7b0eac4): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

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
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	2

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)	3.7%	[3.7%, 3.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-0.1%, 3.7%]	2

Cycles

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
Regressions ❌ (secondary)	5.4%	[5.2%, 5.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 648.428s -> 648.095s (-0.05%)

lqd · 2023-06-14T18:59:41Z

I'm pretty sure the perf changes are noise 😅

kornelski · 2023-06-22T02:57:44Z

Is this meant to display sizes for traits that are never used with dyn? I have a crate-private trait that is only ever used with a monomorphic implementation, but I still see print-vtable-sizes line for it.

WaffleLapkin · 2023-06-22T07:46:17Z

@kornelski yes, this is expected. theoretically it'd be possible to filter out private & not used with dyn traits, but it's not trivial...

rustbot assigned b-naber Jun 7, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 7, 2023

WaffleLapkin commented Jun 7, 2023
View reviewed changes

Comment threadcompiler/rustc_interface/src/passes.rs Outdated

WaffleLapkin commented Jun 7, 2023
View reviewed changes

Comment threadcompiler/rustc_session/src/code_stats.rs Outdated

rustbot assigned compiler-errors and unassigned b-naber Jun 7, 2023

This was referenced Jun 7, 2023
add feature gate to permit measuring memory usage of dyn upcasting #112355
Closed
Don't use compile_error as print#112401
Merged

compiler-errors reviewed Jun 7, 2023
View reviewed changes

Comment threadcompiler/rustc_interface/src/passes.rs Outdated
Comment threadcompiler/rustc_interface/src/passes.rs Outdated

rust-cloud-vmsBotforce-pushed the vtable_stats branch from ff566a0 to 88095f5Compare June 12, 2023 15:49

Collect VTable stats & add -Zprint-vtable-sizes
f2545fb

rust-cloud-vmsBotforce-pushed the vtable_stats branch from 88095f5 to f2545fbCompare June 12, 2023 15:58

compiler-errors reviewed Jun 12, 2023
View reviewed changes

Comment threadcompiler/rustc_interface/src/passes.rs Outdated

compiler-errors reviewed Jun 12, 2023
View reviewed changes

WaffleLapkin added 3 commits June 13, 2023 11:46

Simplify code as suggested by the review
5008a08

Tweak names and docs for vtable stats
8e6a193

Tweak the sort of vtable sizes
dc0fba0

This comment has been minimized.
Sign in to view

Add a test for -Zprint-vtable-sizes
af4631a

rust-cloud-vmsBotforce-pushed the vtable_stats branch from 2bef9e9 to af4631aCompare June 13, 2023 15:17

compiler-errors approved these changes Jun 13, 2023
View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 13, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 14, 2023

bors merged commit 7b0eac4 into rust-lang:masterJun 14, 2023

rustbot added this to the 1.72.0 milestone Jun 14, 2023

lqd mentioned this pull request Jun 14, 2023
Add support for targets without unwinding in mir-opt, and improve --bless for it #112418
Merged

WaffleLapkin deleted the vtable_stats branch June 15, 2023 10:46

Uh oh!

Uh oh!

Conversation

WaffleLapkin commented Jun 7, 2023

Uh oh!

rustbot commented Jun 7, 2023

Uh oh!

Uh oh!

Uh oh!

WaffleLapkin commented Jun 7, 2023

Uh oh!

Uh oh!

Uh oh!

WaffleLapkin commented Jun 12, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

WaffleLapkin commented Jun 13, 2023

Uh oh!

compiler-errors commented Jun 13, 2023

Uh oh!

bors commented Jun 13, 2023

Uh oh!

bors commented Jun 14, 2023

Uh oh!

bors commented Jun 14, 2023

Uh oh!

rust-timer commented Jun 14, 2023

Overall result: ❌✅ regressions and improvements - no action needed

Uh oh!

lqd commented Jun 14, 2023

Uh oh!

kornelski commented Jun 22, 2023

Uh oh!

WaffleLapkin commented Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants