rustdoc: Separate filter-empty-string out into its own function

[notriddle]

No description provided.

[rust-highfive]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

[rust-highfive]

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

[GuillaumeGomez]

When comparing performance between the two, the "manual" filter is much faster. Results say that your proposition is 17.61% slower. I tested it on jsbench.me with the following array:

var x = ["a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","","a", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq", "hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq","hbqsdq",""];

Then the two following codes:

x = x.filter(function(f) { return f !== ""; });

and:

for (var i = 0, len = x.length; i < len; ++i) {
    if (x[i] === "") {
        x.splice(i, 1);
        i -= 1;
    }
}

If you confirm the result, I think it can be closed. However, could you add a comment saying that the current code is faster please?

[notriddle]

Immediately below the code that I changed, it loops through every item in the search index, and then runs checkPath on it. checkPath itself loops over every segment of that path. This makes the algorithmic complexity of that loop O(nSearchWords * paths.length). The only case I can think of where it makes sense to sacrifice readability of this search query parsing code in favor of performance, since it's O(paths.length) either way, is if you have a small crate, and apply a large search query to it.

Honestly, I wasn't really trying to make this code faster, because I assume that nSearchWords will be much, much larger than paths.length. I just thought this code was more readable (it's certainly shorter).

[GuillaumeGomez]

Performance in the search is important. But you can move the code into a filter function instead if you prefer (but still not use the official filter function, I think it's the callback which slows things down...). A bit of readability there would definitely be appreciated.

[notriddle]

@GuillaumeGomez

Okay, I've separated the empty string filter out into its own function, and also used a version that's about 10% faster according to jsbench.me (I designed it thinking that Array.prototype.splice had to renumber everything, so it would be better to only call it once).

[GuillaumeGomez]

About the CI failure, it's because you need to update the rustdoc-js script by adding the name of your function here.

[GuillaumeGomez]

Looks all good to me now. Thanks a lot for both performance and code readability improvements!

[notriddle]

Okay, I fixed the test case, and also did further tweaks to the function... there's an annoying amount of engine-specific behaviour here.

Given these two functions:

function removeEmptyStringsFromArray_A(x) {
    for (var i = 0, len = x.length; i < len; ++i) {
        if (x[i] === "") {
            x.splice(i, 1);
            i -= 1;
        }
    }
}
function removeEmptyStringsFromArray_B(x) {
    for (var i = x.length - 1, j = x.length - 1; j >= 0; j -= 1) {
        if (x[j] !== "") {
            x[i] = x[j];
            i -= 1;
        }
    }
    x.splice(0, i + 1);
}

In Firefox, version A is reported to be 43% slower than version B, but in Chromium, version B is 12.5% slower than version A, using your original test data.

What's really annoying is that there's never a point where B becomes faster. I would've expected a splice nested inside of a loop to have O(n²) runtime, since it ought to renumber the array, but given the apparent worst-case of an array containing nothing but empty strings, both engines perform better with version A. Does anyone have any idea what's going on?

[GuillaumeGomez]

Might need to ask to the people who actually write the JS engine at this point... If you know any?

[GuillaumeGomez]

For now, let's go forward with this already.

@bors: r+ rollup

[bors]

📌 Commit 227f5ed has been approved by GuillaumeGomez