Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Dec 11, 2022

@rustbot author

Fixes #105168

@rustbot
Copy link
Collaborator

rustbot commented Dec 11, 2022

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2022
@Noratrieb Noratrieb removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2022
@ghost
Copy link
Author

ghost commented Dec 11, 2022

r? @Nilstrieb

@rustbot rustbot assigned Noratrieb and unassigned nagisa Dec 11, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ghost ghost closed this Dec 18, 2022
@ghost ghost reopened this Dec 18, 2022
@ghost
Copy link
Author

ghost commented Dec 18, 2022

(GitHub... 💢)

@ghost ghost marked this pull request as ready for review December 18, 2022 16:50
@ghost ghost changed the title [Borked] Use DepKind instead of &'static str in QueryStackFrame Use DepKind instead of &'static str in QueryStackFrame Dec 18, 2022
@ghost
Copy link
Author

ghost commented Dec 18, 2022

@rustbot review

@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 Dec 18, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 18, 2022

r=me unless you want another review from nils :)

@bors delegate=gimbles rollup=never (perf sensitive)

@bors
Copy link
Collaborator

bors commented Dec 18, 2022

✌️ @gimbles can now approve this pull request

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, a few small improvements, r=me with them (or r=jyn514, i leave that to you).

One other thing that could be changed is to use K instead of D for the generic because that's we use already but this would conflict with a "key" generic in one place so it's fine if you leave it like this imo, I (or you if you want) can change it as a follow-up.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ghost
Copy link
Author

ghost commented Dec 23, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 23, 2022

📌 Commit f8b3008 has been approved by gimbles

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 Dec 23, 2022
@Noratrieb
Copy link
Member

@bors r=Nilstrieb

@bors
Copy link
Collaborator

bors commented Dec 23, 2022

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Collaborator

bors commented Dec 23, 2022

📌 Commit f8b3008 has been approved by Nilstrieb

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Dec 23, 2022

⌛ Testing commit f8b3008 with merge c2ff8ad...

@bors
Copy link
Collaborator

bors commented Dec 23, 2022

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing c2ff8ad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 23, 2022
@bors bors merged commit c2ff8ad into rust-lang:master Dec 23, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 23, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c2ff8ad): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

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%] 4
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
-0.2% [-0.2%, -0.2%] 2
Improvements ✅
(secondary)
-0.5% [-0.7%, -0.3%] 5
All ❌✅ (primary) 0.1% [-0.2%, 0.2%] 6

Max RSS (memory usage)

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

Cycles

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

@rustbot rustbot added the perf-regression Performance regression. label Dec 23, 2022
@Noratrieb
Copy link
Member

Filtering out rustc_query_system::query::plumbing::get_query from the cachegrind diff (which shows up with the same + and - because of the added generic param) it's pretty noisy, probably because of slighly different inlining decisions. Most changes are in encoding/decoding.

One interesting case is that DepNode::construct::<_, DefId> got slower and I can't find where the extra instructions could be coming from, as all related functions don't show up in the diff.

@pnkfelix
Copy link
Contributor

  • The primary effects here are to various incremental scenarios, by relatively small amounts in either direction.
  • The PR reviewer already has posted some notes summarizing their investigation into the performance delta.
  • This PR is adding a type parameter to several methods that did not have one before, which I expect to change code layout of the compiler itself.
  • i.e. I would expect this kind of change to have broad but shallow effects on rustc performance, which is consistent with the timer report.
  • Marking as triaged.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 27, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Use `DepKind` instead of `&'static str` in `QueryStackFrame`

`@rustbot` author

Fixes rust-lang#105168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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
Development

Successfully merging this pull request may close these issues.

Use DepKind instead of &'static str in QueryStackFrame
8 participants