Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

store codegen_fn_attrs in crate metadata #96473

Merged
merged 5 commits into from
May 9, 2022
Merged

store codegen_fn_attrs in crate metadata #96473

merged 5 commits into from
May 9, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Apr 27, 2022

extracted from #95562 because the change isn't trivial.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 27, 2022
@rust-highfive
Copy link
Collaborator

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Apr 27, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2022
@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Trying commit 00da420d86e9c4c61da6aa3f22f3d853aa971794 with merge 8bda55c77d599ffd1ee91eba6c0dfa8d31734149...

@bors
Copy link
Contributor

bors commented Apr 27, 2022

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

@rust-timer
Copy link
Collaborator

Queued 8bda55c77d599ffd1ee91eba6c0dfa8d31734149 with parent bb85bca, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8bda55c77d599ffd1ee91eba6c0dfa8d31734149): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 37 32 0 2 37
mean2 0.6% 1.0% N/A -0.2% 0.6%
max 1.4% 4.6% N/A -0.2% 1.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 27, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Apr 27, 2022 •
edited
Loading

maybe caching it on disk if its already simply pulled from the metadata isn't too useful

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2022
@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Trying commit de08cbe2840ae1761852a1a984627c82640b605e with merge 95de7c6d4d85ca601c2838decc62bacf341ca265...

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Apr 27, 2022

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

@rust-timer
Copy link
Collaborator

Queued 95de7c6d4d85ca601c2838decc62bacf341ca265 with parent bb85bca, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (95de7c6d4d85ca601c2838decc62bacf341ca265): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 38 28 1 2 39
mean2 0.6% 1.0% -0.3% -0.2% 0.5%
max 1.5% 4.6% -0.3% -0.2% 1.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Apr 27, 2022

alright, this performance decrease seems acceptable, considering that with #95562 we can now remove inline and some other somewhat frequent attributes from the crate metadata which should offset this regression

@lcnr
Copy link
Contributor Author

lcnr commented Apr 29, 2022

r? rust-lang/compiler

@rust-highfive rust-highfive assigned cjgillot and unassigned fee1-dead Apr 29, 2022
@cjgillot
Copy link
Contributor

Normally, I'd be all for more queries, but the perf result is a bit too large for it's own good. It looks like it is dominated by metadata encoding.

It may be more interesting to land the refactoring in #95562 that makes it possible to omit attributes.
Once this is done, we will be able to identify the cost of the tradeoff in #96473 (comment).

@lcnr lcnr force-pushed the querify-codegen-fn-attrs branch from 9fa5d1e to 9dc70c0 Compare May 9, 2022 15:47
@lcnr
Copy link
Contributor Author

lcnr commented May 9, 2022 •
edited
Loading

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2022
@klensy
Copy link
Contributor

klensy commented May 9, 2022

Finished benchmarking commit (1ceb86c): comparison url.

Improved all, except doc builds (if uncheck all profiles except doc, it can be clearly visible), which regressed.

@cjgillot
Copy link
Contributor

cjgillot commented May 9, 2022

r=me with the warning removed.

@lcnr lcnr force-pushed the querify-codegen-fn-attrs branch from 9dc70c0 to 32b13ac Compare May 9, 2022 16:40
@lcnr
Copy link
Contributor Author

lcnr commented May 9, 2022

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented May 9, 2022

📌 Commit 32b13ac has been approved by cjgillot

@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 May 9, 2022
@bors
Copy link
Contributor

bors commented May 9, 2022

⌛ Testing commit 32b13ac with merge 88860d5...

@bors
Copy link
Contributor

bors commented May 9, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 88860d5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 9, 2022
@bors bors merged commit 88860d5 into rust-lang:master May 9, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 9, 2022
This was referenced May 9, 2022
Closed
Merged
Merged
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (88860d5): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 6 28 89 39 95
mean2 0.3% 0.6% -0.5% -1.6% -0.4%
max 0.6% 2.2% -1.4% -4.8% -1.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@lcnr lcnr deleted the querify-codegen-fn-attrs branch May 10, 2022 06:00
@Alexendoo Alexendoo mentioned this pull request May 10, 2022
Closed
@Aaron1011 Aaron1011 mentioned this pull request May 11, 2022
Merged
@nbdd0121 nbdd0121 mentioned this pull request Jun 30, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2023
@bors
Auto merge of rust-lang#113213 - nbdd0121:unwind, r=<try>
8b98c37 
Fix outdated comment of `fn_can_unwind`

The first part is outdated since rust-lang#96473, and the second part is outdated since rust-lang#97235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
9 participants
@lcnr @rust-highfive @rust-timer @bors @cjgillot @rust-log-analyzer @klensy @fee1-dead @rustbot