Implement struct_target_features. #129881

veluca93 · 2024-09-02T08:35:11Z

This PR implements a first version of RFC 3525.

This is a roll-up of #129764, #129783 and #129764, which will hopefully result in a PR that does not introduce perf regressions in the first place.

This PR also includes code to handle generics, unlike the original PR, since doing so influenced the design of the original PR significantly.

r? Kobzol

Tracking issue: #129107

rustbot · 2024-09-02T08:35:14Z

Could not assign reviewer from: Kobzol.
User(s) Kobzol are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

rustbot · 2024-09-02T08:35:20Z

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

veluca93 · 2024-09-02T08:35:54Z

r? compiler-errors

veluca93 · 2024-09-02T08:36:48Z

@Kobzol please do a perf run once the instability issues are resolved!

compiler-errors · 2024-09-02T08:37:42Z

@bors try @rust-timer queue

bors · 2024-09-02T08:38:52Z

⌛ Trying commit 8bc1c85 with merge 15965d5...

Implement struct_target_features for non-generic functions. This PR implements a first version of RFC 3525. In particular, the current code does not handle structs with target features being passed to generic functions correctly. This is a roll-up of rust-lang#129764, rust-lang#129783 and rust-lang#129764, which will hopefully result in a PR that does not introduce perf regressions in the first place. r? Kobzol Tracking issue: rust-lang#129107

bors · 2024-09-02T10:32:12Z

☀️ Try build successful - checks-actions
Build commit: 15965d5 (15965d5e1fe3d8f875c379474067d15652ebfbb6)

rust-timer · 2024-09-02T12:57:11Z

Finished benchmarking commit (15965d5): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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.5%	[0.3%, 1.2%]	66
Regressions ❌ (secondary)	1.0%	[0.3%, 6.6%]	22
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.8%, -0.3%]	4
All ❌✅ (primary)	0.5%	[0.3%, 1.2%]	66

Max RSS (memory usage)

Results (primary 0.4%, secondary 0.1%)

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.8%	[0.4%, 3.2%]	93
Regressions ❌ (secondary)	0.8%	[0.4%, 3.6%]	71
Improvements ✅ (primary)	-1.1%	[-3.6%, -0.4%]	28
Improvements ✅ (secondary)	-0.9%	[-3.1%, -0.4%]	49
All ❌✅ (primary)	0.4%	[-3.6%, 3.2%]	121

Cycles

Results (primary 0.5%, secondary 0.3%)

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.8%	[0.4%, 4.5%]	62
Regressions ❌ (secondary)	1.1%	[0.4%, 3.3%]	82
Improvements ✅ (primary)	-0.7%	[-1.2%, -0.4%]	14
Improvements ✅ (secondary)	-1.2%	[-5.6%, -0.4%]	45
All ❌✅ (primary)	0.5%	[-1.2%, 4.5%]	76

Binary size

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

Bootstrap: 751.868s -> 783.738s (4.24%)
Artifact size: 338.28 MiB -> 338.31 MiB (0.01%)

Kobzol · 2024-09-02T20:30:36Z

@bors try @rust-timer queue

(Now with fixed perfbot)

bors · 2024-09-02T20:31:45Z

⌛ Trying commit 8bc1c85 with merge 41e69d8...

Implement struct_target_features for non-generic functions. This PR implements a first version of RFC 3525. In particular, the current code does not handle structs with target features being passed to generic functions correctly. This is a roll-up of rust-lang#129764, rust-lang#129783 and rust-lang#129764, which will hopefully result in a PR that does not introduce perf regressions in the first place. r? Kobzol Tracking issue: rust-lang#129107

bors · 2024-09-02T22:24:50Z

☀️ Try build successful - checks-actions
Build commit: 41e69d8 (41e69d8824046f172331c7223f530eeb949af727)

rust-timer · 2024-09-02T23:39:37Z

Finished benchmarking commit (41e69d8): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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.5%	[0.2%, 1.2%]	84
Regressions ❌ (secondary)	0.7%	[0.2%, 2.3%]	22
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.5%	[0.2%, 1.2%]	84

Max RSS (memory usage)

Results (primary 1.0%, secondary -1.1%)

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

Cycles

Results (primary 1.2%, secondary 3.0%)

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)	1.2%	[1.0%, 1.4%]	5
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.0%, 1.4%]	5

Binary size

Results (secondary -0.0%)

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

Bootstrap: 750.044s -> 785.469s (4.72%)
Artifact size: 338.29 MiB -> 338.36 MiB (0.02%)

veluca93 · 2024-09-09T20:42:19Z

After some discussions, I applied some changes wrt the previous design / the RFC:

target features are no longer inherited: if T has features, then only passing T, &T, &&T (etc) to a function enables features for the function. This is likely less confusing, and should probably fix a good chunk of the perf regression. A possible future extension can add #[target_feature(inherit)] or similar if needed.
feature-carrying structs no longer need to be empty (makes the restriction at the above point less limiting)
constructing a feature-carrying struct is no longer unsafe if the function already has the required features (a la target_feature_11)

@compiler-errors / @Kobzol : would you be able to trigger another perf run? Would be useful to know if this version fixes the regression...

Kobzol · 2024-10-05T19:47:14Z

@bors try

bors · 2024-10-05T19:48:25Z

⌛ Trying commit d752ae8 with merge 446b363...

Implement struct_target_features. This PR implements a first version of RFC 3525. This is a roll-up of rust-lang#129764, rust-lang#129783 and rust-lang#129764, which will hopefully result in a PR that does not introduce perf regressions in the first place. This PR also includes code to handle generics, unlike the original PR, since doing so influenced the design of the original PR significantly. r? Kobzol Tracking issue: rust-lang#129107

bors · 2024-10-05T21:34:50Z

☀️ Try build successful - checks-actions
Build commit: 446b363 (446b3634d8038ae8fdcfc73c9a79bb90b516e042)

rust-timer · 2024-10-05T23:46:45Z

Finished benchmarking commit (446b363): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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.2%	[0.2%, 0.2%]	2
Regressions ❌ (secondary)	0.8%	[0.1%, 3.0%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	2

Max RSS (memory usage)

Results (primary 1.1%, secondary -0.7%)

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)	1.1%	[0.4%, 3.9%]	7
Regressions ❌ (secondary)	1.7%	[1.5%, 2.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.2%]	3
All ❌✅ (primary)	1.1%	[0.4%, 3.9%]	7

Cycles

Results (secondary 2.4%)

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.4%	[2.4%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.2%)

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.0%	[0.0%, 0.1%]	40
Regressions ❌ (secondary)	0.2%	[0.0%, 0.4%]	21
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	40

Bootstrap: 773.933s -> 774.549s (0.08%)
Artifact size: 329.48 MiB -> 329.47 MiB (-0.00%)

veluca93 · 2024-10-06T05:56:06Z

@Kobzol I believe the perf results look similar enough to before, and in particular still reasonable for a new feature, but if you could confirm and/or mark the regression as triaged I'd be grateful ;-) (I assume I can't do the latter)

Kobzol · 2024-10-06T06:22:52Z

I agree that it's reasonable. There's nothing to mark as triaged yet though, that only happens after a PR is merged. (the post-merge run can sometimes also have different perf. characteristics).

veluca93 · 2024-10-06T06:39:52Z

I agree that it's reasonable. There's nothing to mark as triaged yet though, that only happens after a PR is merged. (the post-merge run can sometimes also have different perf. characteristics).

Ah, I thought otherwise from the rust-timer report. Then nevermind ;)

Allow using struct-tf with functions with non-Rust ABI. Also allow converting struct-tf functions to function pointers / let them implement function traits.

bors · 2024-10-21T09:17:11Z

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

alex-semenyuk · 2024-11-22T15:12:04Z

@veluca93
From wg-triage. Any updates on this? Could you please rebase meantime

veluca93 · 2024-11-22T15:15:35Z

@veluca93 From wg-triage. Any updates on this? Could you please rebase meantime

There's been a lot of discussion on Zulip on whether this is the correct way forward, so I'm not sure merging this PR as-is is a good idea.

Also nobody has reviewed this PR since September, and after a while I have not kept up with rebasing as that felt somewhat pointless.

rustbot assigned TaKO8Ki Sep 2, 2024

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 Sep 2, 2024

rustbot assigned compiler-errors and unassigned TaKO8Ki Sep 2, 2024

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 Sep 2, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 2, 2024

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 Sep 2, 2024

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 Sep 2, 2024

veluca93 force-pushed the struct_tf branch from 8bc1c85 to 7e763db Compare September 9, 2024 20:42

veluca93 force-pushed the struct_tf branch from 89fbc27 to f55da6b Compare October 5, 2024 17:24

This comment has been minimized.

Sign in to view

veluca93 force-pushed the struct_tf branch from f55da6b to 1cf622f Compare October 5, 2024 17:48

This comment has been minimized.

Sign in to view

veluca93 force-pushed the struct_tf branch from 1cf622f to d752ae8 Compare October 5, 2024 18:13

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 Oct 5, 2024

veluca93 force-pushed the struct_tf branch 2 times, most recently from b670318 to d7ec26e Compare October 14, 2024 09:31

veluca93 added 4 commits October 14, 2024 11:33

Implement struct_target_features for non-generic functions.

69a19f4

Un-querify struct_reachable_target_features

605a9d9

Fix unnecessary restrictions in struct-target-features.

c9b01f0

Allow using struct-tf with functions with non-Rust ABI. Also allow converting struct-tf functions to function pointers / let them implement function traits.

Require annotating struct-tf functions explicitly.

e47b5ae

veluca93 force-pushed the struct_tf branch from d7ec26e to 25e6f44 Compare October 14, 2024 09:35

This comment has been minimized.

Sign in to view

veluca93 force-pushed the struct_tf branch from 25e6f44 to fba90ce Compare October 14, 2024 09:50

This comment has been minimized.

Sign in to view

Implement struct_target_features for generic functions.

b3628ae

veluca93 force-pushed the struct_tf branch from fba90ce to b3628ae Compare October 14, 2024 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement struct_target_features. #129881

Implement struct_target_features. #129881

veluca93 commented Sep 2, 2024 •

edited

Loading

rustbot commented Sep 2, 2024

rustbot commented Sep 2, 2024

veluca93 commented Sep 2, 2024

veluca93 commented Sep 2, 2024

compiler-errors commented Sep 2, 2024

This comment has been minimized.

bors commented Sep 2, 2024

bors commented Sep 2, 2024

This comment has been minimized.

rust-timer commented Sep 2, 2024

Kobzol commented Sep 2, 2024

This comment has been minimized.

bors commented Sep 2, 2024

bors commented Sep 2, 2024

This comment has been minimized.

rust-timer commented Sep 2, 2024

veluca93 commented Sep 9, 2024

This comment has been minimized.

This comment has been minimized.

Kobzol commented Oct 5, 2024

bors commented Oct 5, 2024

bors commented Oct 5, 2024

This comment has been minimized.

rust-timer commented Oct 5, 2024

veluca93 commented Oct 6, 2024

Kobzol commented Oct 6, 2024

veluca93 commented Oct 6, 2024

This comment has been minimized.

This comment has been minimized.

bors commented Oct 21, 2024

alex-semenyuk commented Nov 22, 2024 •

edited

Loading

veluca93 commented Nov 22, 2024

Implement struct_target_features. #129881

Are you sure you want to change the base?

Implement struct_target_features. #129881

Conversation

veluca93 commented Sep 2, 2024 • edited Loading

rustbot commented Sep 2, 2024

rustbot commented Sep 2, 2024

veluca93 commented Sep 2, 2024

veluca93 commented Sep 2, 2024

compiler-errors commented Sep 2, 2024

This comment has been minimized.

bors commented Sep 2, 2024

bors commented Sep 2, 2024

This comment has been minimized.

rust-timer commented Sep 2, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Kobzol commented Sep 2, 2024

This comment has been minimized.

bors commented Sep 2, 2024

bors commented Sep 2, 2024

This comment has been minimized.

rust-timer commented Sep 2, 2024

Overall result: ❌ regressions - ACTION NEEDED

veluca93 commented Sep 9, 2024

This comment has been minimized.

This comment has been minimized.

Kobzol commented Oct 5, 2024

bors commented Oct 5, 2024

bors commented Oct 5, 2024

This comment has been minimized.

rust-timer commented Oct 5, 2024

Overall result: ❌ regressions - ACTION NEEDED

veluca93 commented Oct 6, 2024

Kobzol commented Oct 6, 2024

veluca93 commented Oct 6, 2024

This comment has been minimized.

This comment has been minimized.

bors commented Oct 21, 2024

alex-semenyuk commented Nov 22, 2024 • edited Loading

veluca93 commented Nov 22, 2024

veluca93 commented Sep 2, 2024 •

edited

Loading

alex-semenyuk commented Nov 22, 2024 •

edited

Loading