-
-
Notifications
You must be signed in to change notification settings - Fork 208
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: reduce if statement for NaN
(micro-optimization)
#697
Conversation
Signed-off-by: francesco <[email protected]>
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.
lgtm
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 probably done for the performance reasone. I would like to see the benchmarks first.
@ivan-tymoshenko the difference is very small (is a micro-optimization) before (master)
after (PR)
|
NaN
NaN
(micro-optimization)
NaN
(micro-optimization)NaN
(micro-optimization)
why are strings effected in the benchmark but integer and number not? |
@Uzlopak I think the reason is because string serialization is CPU intensive (more complex) and instable (maybe times change on each benchmark run). Integers and Numbers are super simple block of code. My PR can't influence strings and is a too little optimization for see big changes into "number" and "integer" benchmark time. If we want talk about a huge optimization, this is a good PR #696 |
I dont see the performance gain? Which of these benchmarks is for you significant? |
Maybe it is better to optimize for the valid case and check first if is an integer and if not, just throw that error. |
Signed-off-by: francesco <[email protected]>
is too small optimization to see changes into the benchmark... but this is the significant part before (master)
after (PR)
|
Signed-off-by: francesco <[email protected]>
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.
the last change is good, if we want the most performance, lgtm
Signed-off-by: francesco <[email protected]>
Co-authored-by: Aras Abbasi <[email protected]> Signed-off-by: francesco <[email protected]>
can i have some more benchmarks, please? |
@Uzlopak
PR
I have also update this check https://github.com/fastify/fast-json-stringify/blob/master/lib/serializer.js#L55 for "number" type
PR
|
Signed-off-by: francesco <[email protected]>
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.
lgtm
awesome finding
reduce if statement for
NaN
Checklist
npm run test
andnpm run benchmark
and the Code of conduct