fix: Avoid repeated String#slice calls in stringDistance #1095
Conversation
The `stringDistance` function calls ```js strA.slice(0, -1) ``` and ```js strB.slice(0, -1) ``` multiple times, which is a bit of a waste of time here. JavaScript engines cannot generally eliminate the duplicated calls easily, so it's better to avoid the redundant calls altogether. This improves the chai test on the [web-tooling-benchmark](https://github.com/v8/web-tooling-benchmark) by around 8% when run with upcoming V8 6.4.
Codecov Report
@@ Coverage Diff @@
## master #1095 +/- ##
==========================================
+ Coverage 93.6% 93.61% +<.01%
==========================================
Files 32 32
Lines 1626 1628 +2
Branches 393 393
==========================================
+ Hits 1522 1524 +2
Misses 104 104
Continue to review full report at Codecov.
|
bmeurer
changed the title
|
The code LGTM! Btw, I've taken a look at the failing checks:
|
bmeurer
changed the title
|
Sorry - the appveyor stuff is a red herring in this project, I accidentally set it up for this rather than another project. I'll try to disable it. |
|
So what about the validate-commit-msg? Where can I see why it fails? |
|
Hi @bmeurer, You can see the accepted prefixes on this link: https://conventionalcommits.org/ I think that using |
|
BTW |
|
Great! This LGTM too. If you want to rebase it I'll be waiting to merge, otherwise just leave as comment and we'll merge as soon as we see it. Thanks! |
|
Please just merge the change and make the appropriate changes to the commit message. |
|
Does this even need to use slice at all? It's easy to convert this code to just pass around lengths – but better yet, it can just be a loop without recursion which I just wrote: function stringDistance(strA, strB, memo) {
var memo = [];
// `memo` is a two-dimensional array containing distances.
// memo[i][j] is the distance between strA.slice(0, i) and
// strB.slice(0, j).
for (var i = 0; i <= strA.length; i++) {
memo[i] = Array(strB.length + 1).fill(0);
}
for (var i = 0; i <= strA.length; i++) {
for (var j = 0; j <= strB.length; j++) {
if (i === 0) {
memo[i][j] = j;
} else if (j === 0) {
memo[i][j] = i;
} else {
memo[i][j] = Math.min(
memo[i - 1][j] + 1,
memo[i][j - 1] + 1,
memo[i - 1][j - 1] +
(strA.charCodeAt(i - 1) === strB.charCodeAt(j - 1) ? 0 : 1)
);
}
}
}
return memo[strA.length][strB.length];
}I don't have the benchmark on my machine so it's hard for me to compare easily. I verified it does seem to give the same results though which should not be surprising. @bmeurer Do you want to run the benchmark? |
|
@sophiebits Running a benchmark would be awesome! If it works out faster I'd be very happy with a new PR being made to reflect those changes. |
|
I actually see some more opportunities for improvement here so I'll try to get the benchmark set up locally. |
|
(Posted #1098.) |
The
stringDistancefunction callsstrA.slice(0, -1)andstrB.slice(0, -1)multiple times, which is a bit of a waste of time here. JavaScript
engines cannot generally eliminate the duplicated calls easily, so
it's better to avoid the redundant calls altogether.
This improves the chai test on the web-tooling-benchmark by
around 8% when run with upcoming V8 6.4.