Skip to content

Conversation

@folkertdev
Copy link
Contributor

Could be a fix for a potential performance regression reported here #144529 (comment). Apparently the regression later disappeared. Nevertheless, this seems like a decent refactor.

r? @JonathanBrouwer (vaguely attribute-related, maybe there are other optimizations to that PR that we're missing)

@rustbot 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 Nov 6, 2025
@workingjubilee
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Nov 6, 2025
re-use `self.get_all_attrs` result for pass indirectly attribute
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 6, 2025
@JonathanBrouwer
Copy link
Contributor

JonathanBrouwer commented Nov 6, 2025

get_all_attrs is pretty fast so I can't really imagine this being a performance improvement, but I think this improves the code anyways so lets merge it
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 6, 2025

📌 Commit 1a9cc78 has been approved by JonathanBrouwer

It is now in the queue for this repository.

@bors 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 Nov 6, 2025
@folkertdev
Copy link
Contributor Author

Does anything else in #144529 jump out to you?

@JonathanBrouwer
Copy link
Contributor

Hmmm we do need to call pass_indirectly_in_non_rustic_abis on every argument, which does need to look through the flags, but I can't see that being significant and I can't see why it would specifically affect this benchmark.

I just took a look at the implementation of get_all_attrs, and it's just a btreemap lookup so it should not be that slow. I (and humans in general) are not good at predicting performance tho so lets let the numbers speak. We call get_all_attrs a ton of times throughout the compiler though so if that's slow we are leaving a lot of performance on the table.

Alternatively it could've just been noise, since the benchmark did just bounce back. I think it being noise is the most likely answer.

@rust-bors
Copy link

rust-bors bot commented Nov 6, 2025

☀️ Try build successful (CI)
Build commit: 1869732 (18697328f3073a12811e91ab3af2d58b27d9689f, parent: c90bcb9571b7aab0d8beaa2ce8a998ffaf079d38)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1869732): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible 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)

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

Cycles

Results (primary -2.4%, secondary -3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)
-3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) -2.4% [-2.4%, -2.4%] 1

Binary size

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

Bootstrap: 475.185s -> 475.633s (0.09%)
Artifact size: 390.81 MiB -> 390.80 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 7, 2025
@JonathanBrouwer
Copy link
Contributor

@bors rollup

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2025
…ttrs, r=JonathanBrouwer

re-use `self.get_all_attrs` result for pass indirectly attribute

Could be a fix for a potential performance regression reported here rust-lang#144529 (comment). Apparently the regression later disappeared. Nevertheless, this seems like a decent refactor.

r? `@JonathanBrouwer` (vaguely attribute-related, maybe there are other optimizations to that PR that we're missing)
bors added a commit that referenced this pull request Nov 7, 2025
Rollup of 7 pull requests

Successful merges:

 - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro)
 - #148204 (Modify contributor email entries in .mailmap)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)
 - #148612 (Add note for identifier with attempted hygiene violation)
 - #148613 (Switch hexagon targets to rust-lld)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Nov 7, 2025
Rollup of 7 pull requests

Successful merges:

 - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro)
 - #148204 (Modify contributor email entries in .mailmap)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)
 - #148612 (Add note for identifier with attempted hygiene violation)
 - #148613 (Switch hexagon targets to rust-lld)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Nov 8, 2025
Rollup of 7 pull requests

Successful merges:

 - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro)
 - #148204 (Modify contributor email entries in .mailmap)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)
 - #148612 (Add note for identifier with attempted hygiene violation)
 - #148613 (Switch hexagon targets to rust-lld)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 8, 2025
…ttrs, r=JonathanBrouwer

re-use `self.get_all_attrs` result for pass indirectly attribute

Could be a fix for a potential performance regression reported here rust-lang#144529 (comment). Apparently the regression later disappeared. Nevertheless, this seems like a decent refactor.

r? ``@JonathanBrouwer`` (vaguely attribute-related, maybe there are other optimizations to that PR that we're missing)
bors added a commit that referenced this pull request Nov 8, 2025
Rollup of 15 pull requests

Successful merges:

 - #147404 (Fix issue with callsite inline attribute not being applied sometimes.)
 - #147534 (Implement SIMD funnel shifts in const-eval/Miri)
 - #147686 (update isolate_highest_one for NonZero<T>)
 - #148020 (Show backtrace on allocation failures when possible)
 - #148204 (Modify contributor email entries in .mailmap)
 - #148230 (rustdoc: Properly highlight shebang, frontmatter & weak keywords in source code pages and code blocks)
 - #148555 (Fix rust-by-example spanish translation)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)
 - #148612 (Add note for identifier with attempted hygiene violation)
 - #148613 (Switch hexagon targets to rust-lld)
 - #148644 ([bootstrap] Make `--open` option work with `doc src/tools/error_index_generator`)
 - #148649 (don't completely reset `HeadUsages`)
 - #148675 (Remove eslint-js from npm dependencies)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 8, 2025
…ttrs, r=JonathanBrouwer

re-use `self.get_all_attrs` result for pass indirectly attribute

Could be a fix for a potential performance regression reported here rust-lang#144529 (comment). Apparently the regression later disappeared. Nevertheless, this seems like a decent refactor.

r? ```@JonathanBrouwer``` (vaguely attribute-related, maybe there are other optimizations to that PR that we're missing)
bors added a commit that referenced this pull request Nov 8, 2025
Rollup of 16 pull requests

Successful merges:

 - #147534 (Implement SIMD funnel shifts in const-eval/Miri)
 - #147686 (update isolate_highest_one for NonZero<T>)
 - #148020 (Show backtrace on allocation failures when possible)
 - #148204 (Modify contributor email entries in .mailmap)
 - #148230 (rustdoc: Properly highlight shebang, frontmatter & weak keywords in source code pages and code blocks)
 - #148279 (rustc_builtin_macros: rename bench parameter to avoid collisions with user-defined function names)
 - #148555 (Fix rust-by-example spanish translation)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)
 - #148612 (Add note for identifier with attempted hygiene violation)
 - #148613 (Switch hexagon targets to rust-lld)
 - #148619 (Enable std locking functions on AIX)
 - #148644 ([bootstrap] Make `--open` option work with `doc src/tools/error_index_generator`)
 - #148649 (don't completely reset `HeadUsages`)
 - #148675 (Remove eslint-js from npm dependencies)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Nov 8, 2025
Rollup of 10 pull requests

Successful merges:

 - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro)
 - #147024 (std_detect: Support run-time detection on OpenBSD using elf_aux_info)
 - #147534 (Implement SIMD funnel shifts in const-eval/Miri)
 - #147540 (Stabilise `as_array` in `[_]` and `*const [_]`; stabilise `as_mut_array` in `[_]` and `*mut [_]`.)
 - #147686 (update isolate_highest_one for NonZero<T>)
 - #148230 (rustdoc: Properly highlight shebang, frontmatter & weak keywords in source code pages and code blocks)
 - #148555 (Fix rust-by-example spanish translation)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Nov 8, 2025
Rollup of 10 pull requests

Successful merges:

 - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro)
 - #147024 (std_detect: Support run-time detection on OpenBSD using elf_aux_info)
 - #147534 (Implement SIMD funnel shifts in const-eval/Miri)
 - #147540 (Stabilise `as_array` in `[_]` and `*const [_]`; stabilise `as_mut_array` in `[_]` and `*mut [_]`.)
 - #147686 (update isolate_highest_one for NonZero<T>)
 - #148230 (rustdoc: Properly highlight shebang, frontmatter & weak keywords in source code pages and code blocks)
 - #148555 (Fix rust-by-example spanish translation)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Nov 9, 2025
Rollup of 10 pull requests

Successful merges:

 - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro)
 - #147024 (std_detect: Support run-time detection on OpenBSD using elf_aux_info)
 - #147534 (Implement SIMD funnel shifts in const-eval/Miri)
 - #147540 (Stabilise `as_array` in `[_]` and `*const [_]`; stabilise `as_mut_array` in `[_]` and `*mut [_]`.)
 - #147686 (update isolate_highest_one for NonZero<T>)
 - #148230 (rustdoc: Properly highlight shebang, frontmatter & weak keywords in source code pages and code blocks)
 - #148555 (Fix rust-by-example spanish translation)
 - #148556 (Fix suggestion for returning async closures)
 - #148585 ([rustdoc] Replace `print` methods with functions to improve code readability)
 - #148600 (re-use `self.get_all_attrs` result for pass indirectly attribute)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bbd3ab8 into rust-lang:master Nov 9, 2025
12 checks passed
rust-timer added a commit that referenced this pull request Nov 9, 2025
Rollup merge of #148600 - folkertdev:pass-indirectly-reuse-attrs, r=JonathanBrouwer

re-use `self.get_all_attrs` result for pass indirectly attribute

Could be a fix for a potential performance regression reported here #144529 (comment). Apparently the regression later disappeared. Nevertheless, this seems like a decent refactor.

r? ````@JonathanBrouwer```` (vaguely attribute-related, maybe there are other optimizations to that PR that we're missing)
@rustbot rustbot added this to the 1.93.0 milestone Nov 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants