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

Fix rustdoc duplicated blanket impls #96091

Merged
merged 2 commits into from
Apr 17, 2022
Merged

Fix rustdoc duplicated blanket impls #96091

merged 2 commits into from
Apr 17, 2022

Conversation

GuillaumeGomez
Copy link
Member

Fixes #96036.

I think it'll not be great performance-wise but I couldn't find another way to prevent that unfortunately...

r? @notriddle

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 15, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2022
@GuillaumeGomez
Copy link
Member 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 15, 2022
@bors
Copy link
Contributor

bors commented Apr 15, 2022

⌛ Trying commit 0ef6967560e9c07aa81ddcda731e16eb8e5db8ed with merge 423efbc30a40067e7710e64b2381817f976e764c...

@vacuus
Copy link
Contributor

vacuus commented Apr 15, 2022 •
edited
Loading

I don't actually know that this works, but can't you use a FxHashSet<DefId> for impl_ids? You don't seem to check which set did maps to, just whether it's contained within that set, so the nesting seems unnecessary.

Unrelated, but it would be nice to use the stabilized format_args_capture feature (having the identifier in the braces) for the panic invocation.

@GuillaumeGomez
Copy link
Member Author

Unfortunately, no. The DefId in the FxHashMap is the item and it lists all its implementations. So we cannot only have one level, otherwise we'd have much less implementations.

For the fun fact: I also thought the same as you first. The tests disagreed strongly. 😆

notriddle
notriddle requested changes Apr 15, 2022
View reviewed changes
src/librustdoc/formats/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved
@notriddle
Copy link
Contributor

Overall the approach is okay to me. It's just nitpicking about specifics: why so much of the moral equivalent of unwrap?

@GuillaumeGomez
Copy link
Member Author

It's not that I don't want unwrap equivalents but more than I actually thought it would turn out better this way. Let's see how it'll go in the other way around. :)

@bors
Copy link
Contributor

bors commented Apr 15, 2022

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

@rust-timer
Copy link
Collaborator

Queued 423efbc30a40067e7710e64b2381817f976e764c with parent 1e6fe58, future comparison URL.

@GuillaumeGomez GuillaumeGomez force-pushed the duplicated-blanket-impls branch from 0ef6967 to 26e905b Compare April 15, 2022 22:22
@GuillaumeGomez
Copy link
Member Author

Applied @notriddle's suggestions. :)

notriddle added a commit to notriddle/rust that referenced this pull request Apr 15, 2022
@notriddle
rustdoc: fix duplicate blanket impls
0e853d1 
Fixes rust-lang#96036

This is a different approach than rust-lang#96091, an approach that should have less
performance impact.
@notriddle notriddle mentioned this pull request Apr 15, 2022
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (423efbc30a40067e7710e64b2381817f976e764c): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 1 1 6 1
mean2 N/A 0.3% -0.8% -1.0% -0.8%
max N/A 0.3% -0.8% -1.2% -0.8%

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.

@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 16, 2022
@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 16, 2022

📌 Commit 26e905b9f2c8fd76a4e3e885c37fcf6a8acd8632 has been approved by notriddle

@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 Apr 16, 2022
@GuillaumeGomez GuillaumeGomez mentioned this pull request Apr 16, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 16, 2022
@Dylan-DPC
Rollup merge of rust-lang#96118 - GuillaumeGomez:cleanup-def-id-item-…
10e0db5 
…id, r=notriddle

rustdoc: Rename `def_id` into `item_id` when the type is `ItemId` for readability

As `@notriddle` mentioned in rust-lang#96091, the field name is inaccurate. This PR fixes it by renaming it accordingly to its real type.

r? `@notriddle`
@bors
Copy link
Contributor

bors commented Apr 17, 2022

⌛ Testing commit 26e905b9f2c8fd76a4e3e885c37fcf6a8acd8632 with merge bccc58c250e6d7f5676aca060726ba12e64c969e...

@bors
Copy link
Contributor

bors commented Apr 17, 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 Apr 17, 2022
@GuillaumeGomez
Copy link
Member Author

Needs to be rebased.

@GuillaumeGomez GuillaumeGomez force-pushed the duplicated-blanket-impls branch from 26e905b to 6d10fd0 Compare April 17, 2022 16:04
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

@bors r=notriddle

@bors
Copy link
Contributor

bors commented Apr 17, 2022

📌 Commit 6d10fd0 has been approved by notriddle

@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 Apr 17, 2022
@bors
Copy link
Contributor

bors commented Apr 17, 2022

⌛ Testing commit 6d10fd0 with merge ad4e98e...

@bors
Copy link
Contributor

bors commented Apr 17, 2022

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing ad4e98e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2022
@bors bors merged commit ad4e98e into rust-lang:master Apr 17, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 17, 2022
@GuillaumeGomez GuillaumeGomez deleted the duplicated-blanket-impls branch April 17, 2022 20:33
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ad4e98e): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 3 0 3 0
mean2 N/A 1.1% N/A -1.1% N/A
max N/A 1.2% N/A -1.1% N/A

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. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added the perf-regression Performance regression. label Apr 17, 2022
@pnkfelix
Copy link
Member

Visiting for weekly performance triage.

  • The main hit is to externs, which I'm rejecting as ignorable this week due to extant noise.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 21, 2022
@rylev rylev mentioned this pull request Apr 25, 2022
Merged
@dwrensha dwrensha mentioned this pull request Oct 6, 2022
Closed
@fmease fmease mentioned this pull request Jan 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle requested changes

Assignees

@notriddle notriddle

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-rustdoc Relevant to the rustdoc 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.

rustdoc: Many times the same From<!> for T impl for the ! type
9 participants
@GuillaumeGomez @rust-timer @bors @vacuus @notriddle @rust-log-analyzer @pnkfelix @rust-highfive @rustbot