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

Only build help popup when it's really needed #79986

Merged
merged 2 commits into from
Mar 20, 2021
Merged

Only build help popup when it's really needed #79986

merged 2 commits into from
Mar 20, 2021

Conversation

GuillaumeGomez
Copy link
Member

When working on #79985, I realized that the help popup was built even when it wasn't needed. This PR only makes the help popup to be built when required.

r? @jyn514

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2020
@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 12, 2020
@jyn514 jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Dec 12, 2020
jyn514
jyn514 reviewed Dec 12, 2020
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

There's already caching here so it doesn't do work twice: https://github.com/rust-lang/rust/blob/692ad8332eeaf5de113575ff1490a050ce7d4586/src/librustdoc/html/static/main.js#L2942. Do we need to add additional caching on top? That seems redundant (and a little prone to failure, I'm not 100% the caching logic is right).

src/librustdoc/html/static/main.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

There's already caching here so it doesn't do work twice:

It's not caching so-to-speak. Currently it's built only when it's "needed", meaning whenever you want to show/hide the help popup. Only issue: if you want to hide the help popup, you need to check its classes, and to do so, you need to build it first, which is completely unneeded since you want to hide it anyway. This PR actually enforce to only build it when needed (so when we want to display it).

@GuillaumeGomez
Copy link
Member Author

Updated!

@jyn514
Copy link
Member

jyn514 commented Dec 14, 2020

I guess my question is how much work that actually saves? The new logic is a little tricky and I'm not sure the benefit is worth it.

@GuillaumeGomez
Copy link
Member Author

Put in a straightforward way, we added a check in case we wanted to hide the popup that it actually existed beforehand to not create it needlessly. And as the line diff shows, it doesn't increase the code complexity much.

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2021
@jyn514
Copy link
Member

jyn514 commented Feb 1, 2021

r? @Nemo157

@rust-highfive rust-highfive assigned Nemo157 and unassigned jyn514 Feb 1, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2021
@Nemo157
Copy link
Member

Nemo157 commented Mar 18, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 18, 2021

📌 Commit ab154fd has been approved by Nemo157

@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 Mar 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 18, 2021
@Dylan-DPC
Rollup merge of rust-lang#79986 - GuillaumeGomez:build-help-when-need…
c28d8d2 
…ed, r=Nemo157

Only build help popup when it's really needed

When working on rust-lang#79985, I realized that the help popup was built even when it wasn't needed. This PR only makes the help popup to be built when required.

r? `@jyn514`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 18, 2021
Closed
@Dylan-DPC-zz
Copy link

failed in rollup

@bors r- (needs tidy)

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 18, 2021
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 18, 2021
@GuillaumeGomez
Copy link
Member Author

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Mar 19, 2021

📌 Commit e2c70f7 has been approved by GuillaumeGomez

@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 Mar 19, 2021
@GuillaumeGomez
Copy link
Member Author

Woups.

@bors: r=Nemo157

@bors
Copy link
Contributor

bors commented Mar 19, 2021

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

@bors
Copy link
Contributor

bors commented Mar 19, 2021

📌 Commit e2c70f7 has been approved by Nemo157

@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 19, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2021
@bors
Auto merge of rust-lang#83308 - Dylan-DPC:rollup-p2j6sy8, r=Dylan-DPC
6bfbf0c 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#79986 (Only build help popup when it's really needed)
 - rust-lang#82570 (Add `as_str` method for split whitespace str iterators)
 - rust-lang#83244 (Fix overflowing length in Vec<ZST> to VecDeque)
 - rust-lang#83254 (Include output stream in `panic!()` documentation)
 - rust-lang#83269 (Revert the second deprecation of collections::Bound)
 - rust-lang#83277 (Mark early otherwise optimization unsound)
 - rust-lang#83285 (Update LLVM to bring in SIMD updates for WebAssembly)
 - rust-lang#83297 (Do not ICE on ty::Error as an error must already have been reported)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ae1a2df into rust-lang:master Mar 20, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 20, 2021
@GuillaumeGomez GuillaumeGomez deleted the build-help-when-needed branch March 20, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@Nemo157 Nemo157

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.52.0
Development

Successfully merging this pull request may close these issues.
10 participants
@GuillaumeGomez @rust-highfive @jyn514 @Nemo157 @bors @Dylan-DPC-zz @crlf0710 @JohnCSimon @camelid @rustbot