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

Handle def_ident_span like def_span. #95880

Merged
merged 6 commits into from
Jun 11, 2022
Merged

Handle def_ident_span like def_span. #95880

merged 6 commits into from
Jun 11, 2022

Conversation

cjgillot
Copy link
Contributor

def_ident_span had an ad-hoc status in the compiler.

This PR refactors it to be a first-class citizen like def_span:

  • it gets encoded in the main metadata loop, instead of the visitor;
  • its implementation is updated to mirror the one of def_span.

We do not remove the Option in the return type, since some items do not have an ident, AnonConsts for instance.

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

r? @lcnr

(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 10, 2022
@cjgillot
Copy link
Contributor Author

@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 10, 2022
@bors
Copy link
Collaborator

bors commented Apr 10, 2022

⌛ Trying commit c57e96dc969cbec2730bf3ab84025789c10532bb with merge f3871fbc9a3c49fbaa3fb76744cb534d40ecbeb3...

@bors
Copy link
Collaborator

bors commented Apr 10, 2022

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

@rust-timer
Copy link
Collaborator

Queued f3871fbc9a3c49fbaa3fb76744cb534d40ecbeb3 with parent 341883d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f3871fbc9a3c49fbaa3fb76744cb534d40ecbeb3): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 48 23 0 0 48
mean2 0.4% 0.7% N/A N/A 0.4%
max 1.1% 1.6% N/A N/A 1.1%

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 10, 2022
@petrochenkov petrochenkov mentioned this pull request Apr 10, 2022
Merged
@petrochenkov
Copy link
Contributor

Can this PR wait for #95899? It should remove the extra span decoding causing syntax context shift and the diffs in ui tests.

@lcnr
Copy link
Contributor

lcnr commented Apr 12, 2022

I won't be available next week and it seems like you already have some thoughts here

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned lcnr Apr 12, 2022
@petrochenkov
Copy link
Contributor

Ok, marking as blocked on #95899 then.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked 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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2022
@bors
Auto merge of rust-lang#95899 - petrochenkov:modchild2, r=cjgillot
563ef23 
rustc_metadata: Do not encode unnecessary module children

This should remove the syntax context shift and the special case for `ExternCrate` in decoder in rust-lang#95880.

This PR also shifts some work from decoding to encoding, which is typically useful for performance (but probably not much in this case).
r? `@cjgillot`
@petrochenkov
Copy link
Contributor

#95899 has landed.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 17, 2022
@cjgillot
Copy link
Contributor Author

Perf analysis in #95880 (comment): we encode more spans into metadata and invoke more queries, so the slight regression is to be expected.
@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Jun 10, 2022

📌 Commit 1a881a4 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2022
@bors
Copy link
Collaborator

bors commented Jun 10, 2022

⌛ Testing commit 1a881a4 with merge 1a74efc900c1d81c0b847227351d221cbcb4939b...

@cjgillot cjgillot added the perf-regression-triaged The performance regression has been triaged. label Jun 10, 2022
@bors
Copy link
Collaborator

bors commented Jun 10, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 10, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@cjgillot
Copy link
Contributor Author

@bors retry

@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 Jun 11, 2022
@bors
Copy link
Collaborator

bors commented Jun 11, 2022

⌛ Testing commit 1a881a4 with merge 4e4b3f53906f88781b35d7cf137c260ffcab22b8...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Dylan-DPC
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented Jun 11, 2022

⌛ Testing commit 1a881a4 with merge 99930ac...

@bors
Copy link
Collaborator

bors commented Jun 11, 2022

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 99930ac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2022
@bors bors merged commit 99930ac into rust-lang:master Jun 11, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 11, 2022
@cjgillot cjgillot deleted the def-ident-span branch June 11, 2022 22:49
@bors bors mentioned this pull request Jun 11, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (99930ac): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 0.6% 1.6% 92
Regressions 😿
(secondary)		 0.8% 1.9% 28
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 0.6% 1.6% 92

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 4.2% 11.8% 22
Regressions 😿
(secondary)		 2.4% 3.0% 6
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 4.2% 11.8% 22

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 2.6% 3.5% 13
Regressions 😿
(secondary)		 3.0% 3.0% 2
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 2.6% 3.5% 13

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.63.0
Development

Successfully merging this pull request may close these issues.
9 participants
@cjgillot @rust-highfive @rust-timer @bors @petrochenkov @lcnr @rust-log-analyzer @Dylan-DPC @rustbot