Streamline size estimates #113684

nnethercote · 2023-07-14T10:05:28Z

Makes things nicer and a tiny bit faster.

r? @wesleywiser

rustbot · 2023-07-14T10:05:36Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

nnethercote · 2023-07-14T10:06:03Z

@bors try @rust-timer queue

bors · 2023-07-14T10:06:13Z

⌛ Trying commit adcfd93f0aa9b387f91a10e9dc49c5dc189572a0 with merge eb0596b60545ce2c3ac92dd2aafb5713910aeca4...

bors · 2023-07-14T11:21:25Z

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

nnethercote · 2023-07-14T12:07:58Z

@bors try @rust-timer queue

bors · 2023-07-14T12:08:08Z

⌛ Trying commit f2d28cc15b1a20c5a5b93031eb5d3cf110c61497 with merge a861e61c1dc3fcf632860ee8946626d8945d9bd9...

bors · 2023-07-14T13:23:28Z

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

compiler/rustc_middle/src/mir/mono.rs

wesleywiser · 2023-07-14T22:27:01Z

compiler/rustc_middle/src/mir/mono.rs

+                    // "Normal" functions size estimate: the number of
+                    // statements, plus one for the terminator.


Oh, seeing this code again reminded me of something that occurred to me while reading your recent blog post: should we have different size estimates for codegen with and without optimizations enabled?

I could believe this current estimate is relatively accurate for debug builds but many optimization passes in LLVM are non-linear with regards to the number of basic blocks. Perhaps it makes sense to use different estimation strategies when we are optimizing code and not optimizing code?

I have tried a variety of alternative estimation functions. #113047 and #113649 are two that I filed PRs for, and I've tried a bunch more locally. I can't say that I ever really saw meaningful differences between release and debug builds. Cleverer estimation functions have gotten me exactly zero wins at the moment, so I'm inclined to not spend more effort there :)

It replaces `(Linkage, Visibility)`, making the code nicer. Plus the next commit will add another field.

This means we call `MonoItem::size_estimate` (which involves a query) less often: just once per mono item, and then once more per inline item placement. After that we can reuse the stored value as necessary. This means `CodegenUnit::compute_size_estimate` is cheaper.

They're quite rare, and ignoring them simplifies things quite a bit, and further reduces the number of calls to `MonoItem::size_estimate` to the number of placed items (one per root item, and one or more per reachable inlined item).

It doesn't seem worthwhile now that `MonoItem::size_estimate` is called much less often.

nnethercote · 2023-07-14T23:37:31Z

@bors try @rust-timer queue

bors · 2023-07-14T23:37:41Z

⌛ Trying commit f4c4602 with merge bef6ff6...

rust-timer · 2023-07-14T23:56:22Z

Finished benchmarking commit (eb0596b60545ce2c3ac92dd2aafb5713910aeca4): comparison URL.

Overall result: no relevant changes - no action needed

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-perf -perf-regression

Instruction count

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

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

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

Binary size

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

Bootstrap: 657.803s -> 657.535s (-0.04%)

nnethercote · 2023-07-15T00:06:55Z

@bors r=wesleywiser

bors · 2023-07-15T00:06:56Z

📌 Commit f4c4602 has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-07-15T00:53:22Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing bef6ff6 to master...

rust-timer · 2023-07-15T11:34:09Z

Finished benchmarking commit (bef6ff6): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-6.1%, -1.8%]	3
All ❌✅ (primary)	-	-	0

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: 657.64s -> 657.713s (0.01%)

the8472 · 2023-07-15T16:09:58Z

There has been a bors issue that lead to the merge commit of this PR getting purged from master.
You'll have to make a new PR to reapply it.

…s-2, r=wesleywiser Streamline size estimates (take 2) This was merged in rust-lang#113684 but then [something happened](rust-lang#113684 (comment)): > There has been a bors issue that lead to the merge commit of this PR getting purged from master. > You'll have to make a new PR to reapply it. So this is exactly the same changes. `@bors` r=wesleywiser

rustbot assigned wesleywiser Jul 14, 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 Jul 14, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 14, 2023

This comment has been minimized.

Sign in to view

nnethercote force-pushed the streamline-size-estimates branch from adcfd93 to f2d28cc Compare July 14, 2023 12:07

This comment has been minimized.

Sign in to view

wesleywiser approved these changes Jul 14, 2023

View reviewed changes

nnethercote added 4 commits July 15, 2023 09:24

Introduce MonoItemData.

54b5eae

It replaces `(Linkage, Visibility)`, making the code nicer. Plus the next commit will add another field.

Ignore unreachable inlined items in debug_dump.

71bfc57

They're quite rare, and ignoring them simplifies things quite a bit, and further reduces the number of calls to `MonoItem::size_estimate` to the number of placed items (one per root item, and one or more per reachable inlined item).

Remove instance_def_size_estimate query.

f4c4602

It doesn't seem worthwhile now that `MonoItem::size_estimate` is called much less often.

nnethercote force-pushed the streamline-size-estimates branch from f2d28cc to f4c4602 Compare July 14, 2023 23:37

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 14, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2023

bors merged commit bef6ff6 into rust-lang:master Jul 15, 2023

rustbot added this to the 1.73.0 milestone Jul 15, 2023

nnethercote deleted the streamline-size-estimates branch July 15, 2023 01:48

This was referenced Jul 15, 2023

miri: fail when calling a function that requires an unavailable target feature #113720

Merged

Add the no-builtins attribute to functions when no_builtins is applied at the crate level. #113716

Merged

nnethercote mentioned this pull request Jul 16, 2023

Streamline size estimates (take 2) #113772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamline size estimates #113684

Streamline size estimates #113684

nnethercote commented Jul 14, 2023

rustbot commented Jul 14, 2023

nnethercote commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

bors commented Jul 14, 2023

This comment has been minimized.

nnethercote commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

bors commented Jul 14, 2023

wesleywiser Jul 14, 2023 •

edited

Loading

nnethercote Jul 14, 2023

nnethercote commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

rust-timer commented Jul 14, 2023

nnethercote commented Jul 15, 2023

bors commented Jul 15, 2023

bors commented Jul 15, 2023

rust-timer commented Jul 15, 2023

the8472 commented Jul 15, 2023

		// "Normal" functions size estimate: the number of
		// statements, plus one for the terminator.

Streamline size estimates #113684

Streamline size estimates #113684

Conversation

nnethercote commented Jul 14, 2023

rustbot commented Jul 14, 2023

nnethercote commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

bors commented Jul 14, 2023

This comment has been minimized.

nnethercote commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

bors commented Jul 14, 2023

wesleywiser Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Jul 14, 2023

This comment has been minimized.

bors commented Jul 14, 2023

rust-timer commented Jul 14, 2023

Overall result: no relevant changes - no action needed

nnethercote commented Jul 15, 2023

bors commented Jul 15, 2023

bors commented Jul 15, 2023

rust-timer commented Jul 15, 2023

Overall result: no relevant changes - no action needed

the8472 commented Jul 15, 2023

wesleywiser Jul 14, 2023 •

edited

Loading