-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
perf: Optimize proxify and stringDistance #1098
Conversation
- Fill 2D array with ints upfront to reduce property access cost - Change from recursive to simpler iterative (DP) solution - Add cap parameter to stringDistanceCapped to limit computation - Make candidate generation use a simple loop to avoid allocating unnecessary arrays and to call stringDistance only once on each pair of strings instead of every time in the sort callback This improves chai perf by about 13% on @bmeurer's https://github.com/v8/web-tooling-benchmark.
Codecov Report
@@ Coverage Diff @@
## master #1098 +/- ##
==========================================
+ Coverage 93.61% 93.64% +0.03%
==========================================
Files 32 32
Lines 1628 1637 +9
Branches 393 393
==========================================
+ Hits 1524 1533 +9
Misses 104 104
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks promising! I was actually thinking of using this algorithm instead which requires fewer temporary arrays. Have you considered/tested that as well?
I didn't try that but it makes sense. I did try hoisting the 2D array as a static 41x41 array (and bailing out on names longer than 40 chars) and it didn't seem to help much. Not sure where the bottleneck is now. |
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using new Array(…)
instead of Array(…)
. The [[Call]]
syntax lacks elements kind tracking in V8 and is therefore less efficient.
In this case it doesn't matter, since the arrays contain only small integers. |
memo[i] = []; | ||
} | ||
function stringDistanceCapped(strA, strB, cap) { | ||
if (Math.abs(strA.length - strB.length) >= cap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a good catch 👍
Whoah! I'm really glad that we have so many awesome contributors that are also performance specialists. I've taken a look through the code and the logic LGTM but since I don't feel like I could give any other valuable insights when it comes to performance I'll be happy to review this again when you are happy with it. Thanks everyone for your collaboration 😄 |
I don't think I'll get a chance to look back at this so feel free to merge! Making it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sophiebits Thanks for contributing! This LGTM. Let's merge. Any future perf improvements can come in follow-up PRs.
Thanks a lot @sophiebits and @keithamus! Excited to see this happening. Anyone interested to explore the memory efficient variant of the Levenshtein algorithm? That should give another nice boost. 👍 |
This improvement looks great, although I'm confused about why it's affecting performance so much -- ostensibly the |
The |
|
@meeber But those are only 7 tests? |
@bmeurer Yup, just verified those are the only tests calling |
@meeber Thanks for the explanation. Interesting to see that it made such a big difference. I'll think we still keep the 7 tests following your reasoning, especially since in the near future when we roll a new version of Chai, the impact will be less noticeable. |
Yes I suppose it is worth noting - on a typical users test suite, passing tests won't hit these code paths. This likely makes most of a difference to a users development machine, where they'll see this paths hit from typos. Recently I've been considering how we can improve test times for passing tests, so that tests in typical workloads (e.g. CI) run as quick as possible. One example of slowdown that is instric to the architecture of chai at current - is that we format error messages even for passing tests. |
@keithamus This is a good point. We've observed that error message formatting is indeed quite significant for chai. |
This improves chai perf by about 13% on @bmeurer's https://github.com/v8/web-tooling-benchmark.