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

Improve JS performance by storing length before comparing to it in loops #80515

Merged
merged 2 commits into from
Jan 16, 2021

Conversation

GuillaumeGomez
Copy link
Member

Since #79052 is quite complicated to review, I suggested to split into smaller parts. This first part is mostly about saving the array length into a variable (I tried to not change anything else as much as possible 😃 ).

r? @jyn514

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020
@jyn514 jyn514 added A-rustdoc-ui Area: Rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 30, 2020
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 31, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2021
More js cleanup

Part of rust-lang#79052 (Same kind as rust-lang#80515).

This one is about some small fixes:
 * Replacing some loops with `onEachLazy`.
 * Removing unused function arguments.
 * Turn `buildHelperPopup` into a variable so it can be "replaced" once the function has been called once so it's not called again.

r? `@jyn514`
@bors
Copy link
Contributor

bors commented Jan 4, 2021

☔ The latest upstream changes (presumably #80554) made this pull request unmergeable. Please resolve the merge conflicts.

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.

I think the new code isn't any easier to read, and it creates a lot of churn.

src/librustdoc/html/static/main.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/main.js Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 7, 2021
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

@Nemo157 Updated!

Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

The rest of the changes all look correct 👍

@@ -1710,7 +1706,7 @@ function defocusSearchBar() {
if (smallest === null) {
break;
}
for (x = 0; x < arrays.length && ret.length < MAX_RESULTS; ++x) {
for (x = 0; x < arrays_len && ret.length < MAX_RESULTS; ++x) {
Copy link
Member

Choose a reason for hiding this comment

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

This is using the non-lexical variable from line 1700, right? JS is fun 😭.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It's a syntax nightmare. ^^'

@jyn514 jyn514 removed their assignment Jan 15, 2021
@GuillaumeGomez
Copy link
Member Author

Thanks @Nemo157 !

@bors: r=Nemo157,jyn514 rollup

@bors
Copy link
Contributor

bors commented Jan 15, 2021

📌 Commit e727600 has been approved by Nemo157,jyn514

@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 Jan 15, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2021
…laumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#77693 (Add test for rust-lang#59352)
 - rust-lang#80515 (Improve JS performance by storing length before comparing to it in loops)
 - rust-lang#81030 (Update mdbook)
 - rust-lang#81033 (Remove useless `clean::Variant` struct)
 - rust-lang#81049 (inline: Round word-size cost estimates up)
 - rust-lang#81054 (Drop a few unneeded borrows)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 033faf9 into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
@GuillaumeGomez GuillaumeGomez deleted the js-for-loop-perf branch January 16, 2021 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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
Development

Successfully merging this pull request may close these issues.

6 participants