-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Refactor test-readline-async-iterators into a benchmark #49224
Refactor test-readline-async-iterators into a benchmark #49224
Comments
Hi @mcollina, I was thinking to pick this issue. According to my current analysis |
My suggestion would be, simply moving
oldWay to benchmark/readline/readline-iterable.js , and add a type: ['new', 'old'] to the benchmark configs that allows the benchmark to run it with both the new and the old way of async iteration - it already runs the new way of async iteration and just needs a new config for the old way. If anyone still wants to compare the numbers they can just run the benchmarks once and compare the op/s output of different configs. We have been doing that for benchmarking different URL parsers for example. (Not sure how I can make myself clear without writing the whole thing myself, but you can read up on https://github.com/nodejs/node/blob/main/doc/contributing/writing-and-running-benchmarks.md#basics-of-a-benchmark to understand how benchmark works).
|
Fixes: nodejs#49224 Added old way of readline async iterator to benchmark.
@joyeecheung I've opened a PR with your suggested changes. Can you please take a look. :) |
Hey @shubham9411 are u still working on this Issue? |
Yup @Prateek462003, Here is link to the PR #49237. |
Fixes: nodejs#49224 Added old way of readline async iterator to benchmark.
PR-URL: #49237 Fixes: #49224 Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #49237 Fixes: #49224 Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#49237 Fixes: nodejs#49224 Reviewed-By: Matteo Collina <[email protected]>
Originally posted by @joyeecheung in #41276 (comment)
The text was updated successfully, but these errors were encountered: