Improve JS performance #79052

GuillaumeGomez · 2020-11-14T19:35:13Z

Along my random journey on the internet, I came upon an interesting fact when looking at some code: they were always setting the length of the data they were iterating on in a variable instead of just accessing it at every turn.

Based on this strange code, I decided to check why they were doing so and discovered that you could get up to 3% of performance improvements (which is big enough for me to consider it).

So to sum it up:

var some_array = [];

for (var i = 0, len = some_array.length; i < len; ++i) {}
// is faster than:
for (var i = 0; i < some_array.length; ++i) {}

Of course, it only makes sense to use this when the data we're iterating on doesn't have its length updated while iterating.

The second commit is a little cleanup after I ran "eslint".

r? @jyn514

jyn514

How did you measure the performance change? I don't see why this would have anything to do with performace 😕

src/librustdoc/html/static/main.js

jyn514 · 2020-11-14T22:06:00Z

src/librustdoc/html/static/main.js

+            for (var z = split.length - 1; z >= 0; --z) {
                if (split[z] === "") {
                    split.splice(z, 1);
-                    z -= 1;
                }
            }


Suggested change

for (var z = split.length - 1; z >= 0; --z) {

if (split[z] === "") {

split.splice(z, 1);

z -= 1;

}

}

split = split.filter(function(segment) { return segment !== ""; });

That is a huge speed gain. (84% apparently). Great catch!

jyn514 · 2020-11-14T22:10:18Z

src/librustdoc/html/static/main.js

+                    name = result.item.name.toLowerCase();
+                    path = result.item.path.toLowerCase();
+                    parent = result.item.parent;


Does this really have a performance difference?

No, it's more about cleanup. Recreating the variable at each iteration isn't "clean" (and invalid too, but js doesn't really care apparently).

javascript variables defined with var are hoisted to the top of the function you're in, so declaring them earlier doesn't make a difference. so it won't recreate the variable each iteration, so it's valid. are you referencing something when you say "clean"?

https://developer.mozilla.org/en-US/docs/Glossary/Hoisting

jyn514 · 2020-11-14T22:17:47Z

src/librustdoc/html/static/main.js

+                                    for (x = 0; allFound === false && x < elen; ++x) {
                                        allFound = getObjectFromId(elems[x]).name === firstGeneric;
                                    }
                                    if (allFound === true) {


Suggested change

for (x = 0; allFound === false && x < elen; ++x) {

allFound = getObjectFromId(elems[x]).name === firstGeneric;

}

if (allFound === true) {

var allFound = elems.every(function(id) { return getObjectFromId(id).name == firstGeneric; });

if (allFound === true) {

elems.pop();

}

If you don't like the suggestion, you have a typo: elen -> elems.

It's not a typo, it's meant for "elements length". But it's not that great so I'll update it a bit. Also, the code you suggested might be a worth a try, but I'll delay it for another PR until I fully get how it works.

jyn514 · 2020-11-14T22:19:20Z

src/librustdoc/html/static/main.js

+                        for (x = 0, len = obj[GENERICS_DATA].length; x < len; ++x) {
                            if (obj[GENERICS_DATA][x] === val.name) {
                                return true;
                            }


Suggested change

for (x = 0, len = obj[GENERICS_DATA].length; x < len; ++x) {

if (obj[GENERICS_DATA][x] === val.name) {

return true;

}

if (obj[GENERICS_DATA].some(function(name) { return name === val.name; })) {

return true;

Actually, we could even use our own "onEach" function. It basically works the same after all.

jyn514 · 2020-11-14T22:21:25Z

src/librustdoc/html/static/main.js

@@ -927,8 +924,7 @@ function defocusSearchBar() {
                    lev_distance = Math.ceil((checkGenerics(obj, val) + lev_distance) / 2);
                } else if (obj.length > GENERICS_DATA && obj[GENERICS_DATA].length > 0) {
                    // We can check if the type we're looking for is inside the generics!
-                    var olength = obj[GENERICS_DATA].length;
-                    for (x = 0; x < olength; ++x) {
+                    for (x = 0, len = obj[GENERICS_DATA].length; x < len; ++x) {


Why this change?

I removed a useless extra variable.

jyn514 · 2020-11-14T22:21:43Z

src/librustdoc/html/static/main.js

@@ -942,8 +938,7 @@ function defocusSearchBar() {
                var lev_distance = MAX_LEV_DISTANCE + 1;

                if (obj && obj.type && obj.type[INPUTS_DATA] && obj.type[INPUTS_DATA].length > 0) {
-                    var length = obj.type[INPUTS_DATA].length;
-                    for (var i = 0; i < length; i++) {
+                    for (var i = 0, len = obj.type[INPUTS_DATA].length; i < len; i++) {


Why this change?

More consistent with the rest.

Lonami · 2020-11-14T23:05:18Z

don't see why this would have anything to do with performace

Just a guess, but by having a local, the JIT compiler probably can do a better job with it. After all the original way always accesses the .length attribute of an arbitrary object which is probably more expensive than a local.

For the other changes where a length was declared before and is now also inside the for, I'd argue it makes it more consistent with the rest of these changes :P

GuillaumeGomez · 2020-11-15T14:02:50Z

@jyn514: I was running the performance comparison using https://jsbench.me/ for another project and came across this one. The 3% improvement is what came out after making the median out of the runs.

jyn514 · 2020-11-17T17:17:05Z

src/librustdoc/html/static/main.js

@@ -569,7 +570,7 @@ function defocusSearchBar() {
                    len = rootPath.match(/\.\.\//g).length + 1;

                for (i = 0; i < len; ++i) {
-                    match = url.match(/\/[^\/]*$/);
+                    match = url.match(/\/[^/]*$/);


I think this was easier to read before - unless it matched a literal backslash?

The linter complained about it so I removed it.

jyn514 · 2020-11-29T16:47:25Z

src/librustdoc/html/static/main.js

-    function buildHelperPopup() {
+    var buildHelperPopup = function() {


Why this change?

Because we call this function every time we want to show the help popup instead of creating the popup every time. However, since it only requires to be built once, we then set buildHelperPopup to an empty function so that calling it doesn't do anything anymore.

bors · 2020-12-01T12:34:44Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

frewsxcv · 2020-12-11T16:26:37Z

src/librustdoc/html/static/main.js

@@ -927,8 +920,7 @@ function defocusSearchBar() {
                    lev_distance = Math.ceil((checkGenerics(obj, val) + lev_distance) / 2);
                } else if (obj.length > GENERICS_DATA && obj[GENERICS_DATA].length > 0) {
                    // We can check if the type we're looking for is inside the generics!
-                    var olength = obj[GENERICS_DATA].length;
-                    for (x = 0; x < olength; ++x) {
+                    for (x = 0, len = obj[GENERICS_DATA].length; x < len; ++x) {


where is x declared?

jyn514 · 2020-12-11T17:33:39Z

I'll be honest, I am not comfortable making changes of this magnitude without tests. Maybe we could just keep the filter improvement (#79052 (comment)) and leave the rest for later?

One way to verify this would be use strict, right? I don't think we have that on currently.

GuillaumeGomez · 2020-12-15T14:19:18Z

Actually, we use "use strict";. ;)

But if the amount of changes is problematic, I can make smaller PRs if you prefer?

Part of rust-lang#79052, originally suggested in rust-lang#79052 (comment) Co-authored-by: Joshua Nelson <[email protected]>

Use Array.prototype.filter instead of open-coding Part of rust-lang#79052, originally suggested in rust-lang#79052 (comment) by `@jyn514` Besides making main.js smaller (always a plus), this also performs better by using the optimized filter implementation in your browser's JavaScript engine (according to `@GuillaumeGomez,` an 84% performance improvement).

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`

…Nemo157,jyn514 Improve JS performance by storing length before comparing to it in loops Since rust-lang#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`

GuillaumeGomez · 2021-01-19T13:54:17Z

Can be closed now!

GuillaumeGomez added 2 commits November 14, 2020 20:24

JS performance improvements

96e5e5c

Fix JS lints

ae0b37a

rust-highfive assigned jyn514 Nov 14, 2020

This comment has been minimized.

Sign in to view

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2020

jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 14, 2020

jyn514 reviewed Nov 14, 2020

View reviewed changes

Improve JS a bit

1c26969

jyn514 reviewed Nov 29, 2020

View reviewed changes

frewsxcv reviewed Dec 11, 2020

View reviewed changes

This was referenced Dec 30, 2020

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

Merged

More js cleanup #80554

Merged

notriddle added a commit to notriddle/rust that referenced this pull request Jan 1, 2021

Use Array.prototype.filter instead of open-coding

a3958d0

Part of rust-lang#79052, originally suggested in rust-lang#79052 (comment) Co-authored-by: Joshua Nelson <[email protected]>

notriddle mentioned this pull request Jan 1, 2021

Use Array.prototype.filter instead of open-coding #80569

Merged

JohnCSimon 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 11, 2021

GuillaumeGomez closed this Jan 19, 2021

GuillaumeGomez deleted the js-perf branch January 19, 2021 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve JS performance #79052

Improve JS performance #79052

GuillaumeGomez commented Nov 14, 2020

This comment has been minimized.

jyn514 left a comment

jyn514 Nov 14, 2020

GuillaumeGomez Nov 15, 2020

jyn514 Nov 14, 2020

GuillaumeGomez Nov 15, 2020

frewsxcv Dec 11, 2020 •

edited

Loading

frewsxcv Dec 11, 2020

jyn514 Nov 14, 2020

GuillaumeGomez Nov 15, 2020

jyn514 Nov 14, 2020

GuillaumeGomez Nov 15, 2020

jyn514 Nov 14, 2020

GuillaumeGomez Nov 15, 2020

jyn514 Nov 14, 2020

GuillaumeGomez Nov 15, 2020

Lonami commented Nov 14, 2020

GuillaumeGomez commented Nov 15, 2020

jyn514 Nov 17, 2020

GuillaumeGomez Nov 29, 2020

jyn514 Nov 29, 2020

GuillaumeGomez Nov 29, 2020

bors commented Dec 1, 2020

frewsxcv Dec 11, 2020

jyn514 commented Dec 11, 2020

GuillaumeGomez commented Dec 15, 2020

GuillaumeGomez commented Jan 19, 2021

		function buildHelperPopup() {
		var buildHelperPopup = function() {

Improve JS performance #79052

Improve JS performance #79052

Conversation

GuillaumeGomez commented Nov 14, 2020

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lonami commented Nov 14, 2020

GuillaumeGomez commented Nov 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 1, 2020

Choose a reason for hiding this comment

jyn514 commented Dec 11, 2020

GuillaumeGomez commented Dec 15, 2020

GuillaumeGomez commented Jan 19, 2021

frewsxcv Dec 11, 2020 •

edited

Loading