Implement struct_target_features. by veluca93 · Pull Request #129881 · rust-lang/rust (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation71 Commits5 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
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.
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
@Kobzol please do a perf run once the instability issues are resolved!
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
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
☀️ Try build successful - checks-actions
Build commit: 15965d5 (15965d5e1fe3d8f875c379474067d15652ebfbb6)
This comment has been minimized.
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%)
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
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
☀️ Try build successful - checks-actions
Build commit: 41e69d8 (41e69d8824046f172331c7223f530eeb949af727)
This comment has been minimized.
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%)
After some discussions, I applied some changes wrt the previous design / the RFC:
- target features are no longer inherited: if
Thas features, then only passingT,&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...
This comment has been minimized.
bors added a commit to rust-lang-ci/rust that referenced this pull request
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
☀️ Try build successful - checks-actions
Build commit: 446b363 (446b3634d8038ae8fdcfc73c9a79bb90b516e042)
This comment has been minimized.
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%)
@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)
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).
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.
This comment has been minimized.
This comment has been minimized.
@veluca93
From wg-triage. Any updates on this? Could you please rebase meantime
@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.
Status: Blocked on something else such as an RFC or other implementation work.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Labels
Performance regression.
Status: Blocked on something else such as an RFC or other implementation work.
Relevant to the compiler team, which will review and decide on the PR/issue.