-
-
Notifications
You must be signed in to change notification settings - Fork 376
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
@middy/http-json-body-parser perf improvement #643
Conversation
Here is my unscientific bench:
Also, note that the Moreover, my parser doesn't allow arbitrary application/foo+json content types. But if a user wants them, he can list them individually. It's very rare that you really need to support all JSON-related content types instead of a finite subset of them. Neither it is clear whether it's the regexp there that slows it down. It may be the HTTP-errors package. So more research is needed. Upd: |
Yeah, it's the regex. I think supporting |
My opinion is that the predefined standards also predefine the types. So it's only one or a few fixed standard-compliant content-types. BTW |
After a bunch of jspref testing, I think this is a fast as it's going to get for now. I did try passing in a static value and checking for an exact match but found it performed the same. Once we have a proper benchmarking tool in place we can revisit. |
The current version is as fast as my best code. Congratulations!
I think it's because the RegExp is still compiled. Also I wonder if there's measurable difference between // and '': const pattern = new RegExp('^application/(.+\\+)?json(;.*)?$') BTW the PR has a typo: it's iMprovement. |
Formatting is wrong |
Good idea. The big gain was from only compiling the RegExp once. const slowest = new RegExp( '^application\/(.+\\+)?json(;.*)?$')
const fast = new RegExp( /^application\/(.+\\+)?json(;.*)?$/)
const fastest = /^application\/(.+\\+)?json(;.*)?$/ |
@nponeccop This should be much faster, let me know if this is what you had in mind.
Related to #641