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.

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

Use index newtyping for TyVid #88710

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Conversation

Mark-Simulacrum
Copy link
Member

This is useful for using TyVid in types like VecGraph, and just otherwise seems like a small win.

@rust-highfive
Copy link
Collaborator

r? @nagisa

(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 Sep 7, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

One small nit, but r=me otherwise

compiler/rustc_infer/src/infer/type_variable.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member

jackh726 commented Sep 7, 2021

r? @jackh726

@rust-highfive rust-highfive assigned jackh726 and unassigned nagisa Sep 7, 2021
@Mark-Simulacrum
Copy link
Member Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Sep 7, 2021

📌 Commit 2eac09d has been approved by jackh726

@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 Sep 7, 2021
@jackh726
Copy link
Member

jackh726 commented Sep 7, 2021

@bors rollup=never

It's possible this is perf sensitive, given TyVids are used practically everywhere.

@bors
Copy link
Contributor

bors commented Sep 7, 2021

⌛ Testing commit 2eac09d with merge 99db26bb785497c59593b01985d8983e640229d1...

@bors
Copy link
Contributor

bors commented Sep 7, 2021

💥 Test timed out

@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 Sep 7, 2021
@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)

@Mark-Simulacrum
Copy link
Member Author

Majority of the builders finished, with distcheck being the odd one out - I'm guessing this is spurious.

@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 Sep 7, 2021
@bors
Copy link
Contributor

bors commented Sep 7, 2021

⌛ Testing commit 2eac09d with merge 69c4aa2...

@bors
Copy link
Contributor

bors commented Sep 7, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 69c4aa2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 7, 2021
@bors bors merged commit 69c4aa2 into rust-lang:master Sep 7, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 7, 2021
@Mark-Simulacrum Mark-Simulacrum deleted the tyvid-idx branch September 7, 2021 19:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (69c4aa2): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

  • Moderate regression in instruction counts (up to 1.7% on full builds of keccak)

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

@rustbot rustbot added the perf-regression Performance regression. label Sep 7, 2021
@Mark-Simulacrum
Copy link
Member Author

Hm, interesting. My guess is the extra checks for from_u32/from_usize being in-range (now that we have some niche bits) are causing the worse performance -- that said, cycle times seem more improved than not (https://perf.rust-lang.org/compare.html?start=73641cd23ba470c6b4dcd72b8d5f62d27c735254&end=69c4aa2901ffadf69deaf91b2f90604bcbc2eb36&stat=cycles%3Au) and generally keccak is more of a stress test than real world code.

I'll try to spend some time with cachegrind investigating this.

@Mark-Simulacrum
Copy link
Member Author

Cachegrind seems to confirm that the index manipulation is the likely cause of the added overhead. I don't think that's worth fixing with unsafe code (and would be difficult to justify).

@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants