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

perf: replace querystring with fast-querystring #10248

Closed
wants to merge 1 commit into from

Conversation

anonrig
Copy link

@anonrig anonrig commented Sep 6, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other: Performance boost

What is the current behavior?

Replacing querystring with fast-querystring improves performance

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@jmcdo29
Copy link
Member

jmcdo29 commented Sep 6, 2022

This is about an 25% increase compared to the original querystring library. 727674.83 op/sec vs 913716.89 op/sec (ran the benchmarks on my machine to ensure the same number of requests) so the increase is nice, but the library also only has 3 stars and depends on a library that hasn't been touched since 2019 with 19 stars. That library truly may be feature complete, and it doesn't have any dependencies, but it's still something for us to consider.

My vote is to keep an eye on this parsing implementation, but not to bring it into Nest quite yet, as it's still very fresh.

@anonrig
Copy link
Author

anonrig commented Sep 6, 2022

Thanks for the comment @jmcdo29. You're absolutely right about the age of the library. I released it couple of days ago, but the benchmarks and the test suite (which is a replica of node.js querystring module) speak for itself.

PS: find-my-way and fastify are already in progress of replacing node:querystring with fast-querystring (delvedor/find-my-way#300) and I thought nest.js can share the same performance boost when the time comes.

Regarding fast-decode-uri-component, it has 19 stars, but has a weekly download of 784,858 and provides a faster solution to decodeURIComponent function. The star count & fork count, does not really mean anything in terms of performance & engineering since there is a query string parsing library with 7300+ stars, and 3x worse performance than this package.

@jmcdo29
Copy link
Member

jmcdo29 commented Sep 6, 2022

the benchmarks and the test suite (which is a replica of node.js querystring module) speak for itself.

True, and that's definitely good to see. I'm usually hesitant with brand new libraries just to make sure that there will be long term support for them.

PS: find-my-way and fastify are already in progress of replacing node:querystring with fast-querystring

Oh interesting, seeing that fastify is already planning for this, looked like in February/March, we may try to do the same around then. Let's see what @kamilmysliwiec thinks as well.

The star count & fork count, does not really mean anything in terms of performance & engineering

True, but it can help give insight into how much the library is used by the community and how backed it is in the case of a dev leaving maintenance of it (more stars, more likely to have someone fork it usually).

@anonrig
Copy link
Author

anonrig commented Sep 7, 2022

Somehow npm i updates package-lock.json with a massive change list. (I'm using Node 18.8 with npm 8.18)

@jmcdo29
Copy link
Member

jmcdo29 commented Sep 7, 2022

Somehow npm i updates package-lock.json with a massive change list. (I'm using Node 18.8 with npm 8.18)

Ah, yeah, we're still using npm 6 with a package-lock.json v1. npm 8 uses v2 and changes the structure

@anonrig
Copy link
Author

anonrig commented Sep 8, 2022

Hey @jmcdo29 I just released 0.7.0 which includes more performance improvements. Can you redo your benchmark when you have the chance? I'm quite curious about a real-time improvement of fast-querystring. Thanks!

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.

2 participants