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

lib: refactor join to inline stuff in loop #33854

Closed
wants to merge 1 commit into from

Conversation

yashLadha
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 12, 2020
// It is faster not to use a template string here
str += output[i];
str += separator;
str += (i !== arrayLen - 1) ? separator : '';
Copy link
Member

Choose a reason for hiding this comment

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

This looks costly, which may be shown by a quick benchmark or simply time-ing this for a large number of iterations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think V8 should do the optimization to inline the stuff and remove comparison as the comparison is already provided. Will do a timing benchmark.

@devsnek
Copy link
Member

devsnek commented Jun 21, 2020

Can we benchmark just getting rid of this? It's been a long time since v8 6.0.

@sapics
Copy link
Contributor

sapics commented Jun 29, 2020

@devsnek
I make a benchmark native/join vs util/join at #33732 with nodejs v12, Windows 10.
native/join is much faster than util/join when array is big, however util/join is faster than native/join when array is small.

Join string performance check -- array size 1000000
native/join: 5.081699997186661 ms
  util/join: 146.29249899089336 ms

Join string performance check -- array size 5
native/join: 16.52709999680519 / 100000
  util/join: 9.16829900443554 / 100000

@yashLadha
Copy link
Contributor Author

@sapics how about if we do a conditional check and use the method based on the length of the array to join.

If length >= 100
  -> Use native impl
else
  -> Use nodejs impl

One interesting thing to note is the performance difference in smaller parts 🤔

@sapics
Copy link
Contributor

sapics commented Jun 29, 2020

@yashLadha I think that it is better performance, generally.
When I searched previously, util/join was not used much, probably.
So, there are some possibilities that most of use-cases are < 100 😄

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Oct 16, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@yashLadha yashLadha closed this Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues and PRs that are stalled. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants