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

fix: Avoid repeated String#slice calls in stringDistance #1095

Merged
merged 1 commit into from
Dec 2, 2017

Conversation

bmeurer
Copy link
Contributor

@bmeurer bmeurer commented Nov 29, 2017

The stringDistance function calls strA.slice(0, -1) and 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 by
around 8% when run with upcoming V8 6.4.

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.
@bmeurer bmeurer requested a review from a team as a code owner November 29, 2017 09:48
@codecov
Copy link

codecov bot commented Nov 29, 2017

Codecov Report

Merging #1095 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/chai/utils/proxify.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a6610e...b0b7b05. Read the comment docs.

@bmeurer bmeurer changed the title Avoid repeated String#slice calls in stringDistance. Avoid repeated String#slice calls in stringDistance Nov 29, 2017
@lucasfcosta
Copy link
Member

The code LGTM!
Thanks for your insight into this, it's great to have this kind of input in Chai's codebase 😄

Btw, I've taken a look at the failing checks:

  • validate-commit-msg - Fails because the commit message does not follow the rules specified here: https://conventionalcommits.org/. For this case I think we can use the fix: prefix
  • continuous-integration/appveyor/pr - Fails because the build would run as a Visual Studio project. Maybe setting this build as Script as suggested would be enough, but since we already run a reliable build on TravisCI, I think this is redundant. Is there any significant change between running it on AppVeyor and TravisCI?

@bmeurer bmeurer changed the title Avoid repeated String#slice calls in stringDistance fix: Avoid repeated String#slice calls in stringDistance Nov 29, 2017
@keithamus
Copy link
Member

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.

@bmeurer
Copy link
Contributor Author

bmeurer commented Nov 30, 2017

So what about the validate-commit-msg? Where can I see why it fails?

@lucasfcosta
Copy link
Member

Hi @bmeurer,

You can see the accepted prefixes on this link: https://conventionalcommits.org/

I think that using perf: for this case would be perfect.

@keithamus
Copy link
Member

BTW validate-commit-msg shouldn't block the merge. If someone reviews this code we can squash-and-merge.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

So consider this approval. @bmeurer feel free to rebase your commit if you'd like to - otherwise we can hit the squash-and-merge button to refactor the commit message.

@lucasfcosta
Copy link
Member

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!

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

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

Great catch! LGTM

@bmeurer
Copy link
Contributor Author

bmeurer commented Dec 2, 2017

Please just merge the change and make the appropriate changes to the commit message.

@lucasfcosta lucasfcosta merged commit eae99d1 into chaijs:master Dec 2, 2017
@bmeurer bmeurer deleted the stringDistance_slice branch December 2, 2017 18:09
@sophiebits
Copy link
Contributor

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?

@keithamus
Copy link
Member

@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.

@sophiebits
Copy link
Contributor

I actually see some more opportunities for improvement here so I'll try to get the benchmark set up locally.

@sophiebits
Copy link
Contributor

(Posted #1098.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants