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

Refactor test-readline-async-iterators into a benchmark #49224

Closed
mcollina opened this issue Aug 18, 2023 · 5 comments · Fixed by #49237 · May be fixed by shubham9411/node#2
Closed

Refactor test-readline-async-iterators into a benchmark #49224

mcollina opened this issue Aug 18, 2023 · 5 comments · Fixed by #49237 · May be fixed by shubham9411/node#2
Labels
good first issue Issues that are suitable for first-time contributors. test Issues and PRs related to the tests.

Comments

@mcollina
Copy link
Member

          test-readline-async-iterators is now flaky in the CI: https://github.com/nodejs/reliability/issues/640, and it's quite slow to run so it might create timeout issues on slower machines in the CI. is there a reason the performance test must be done in the test suite? It looks like this could be placed in benchmarks instead

Originally posted by @joyeecheung in #41276 (comment)

@mcollina mcollina added the good first issue Issues that are suitable for first-time contributors. label Aug 18, 2023
@shubham9411
Copy link
Contributor

Hi @mcollina, I was thinking to pick this issue.

According to my current analysis test-readline-async-iterators is taking time in CI because of added testPerformance. In my local with testPerformance test is taking around 5s while without that it is taking around 0.05s. We want to move that perf test to benchmark. Is my understanding correct? Thanks

@joyeecheung
Copy link
Member

joyeecheung commented Aug 18, 2023

My suggestion would be, simply moving

for await (const line of oldWay.call(rlOldWay)) {
and the implementation of 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).

shubham9411 added a commit to shubham9411/node that referenced this issue Aug 19, 2023
Fixes: nodejs#49224
Added old way of readline async iterator to benchmark.
@shubham9411
Copy link
Contributor

@joyeecheung I've opened a PR with your suggested changes. Can you please take a look. :)

@Prateek462003
Copy link

Hey @shubham9411 are u still working on this Issue?

@shubham9411
Copy link
Contributor

Yup @Prateek462003, Here is link to the PR #49237.

@VoltrexKeyva VoltrexKeyva added the test Issues and PRs related to the tests. label Aug 22, 2023
shubham9411 added a commit to shubham9411/node that referenced this issue Aug 23, 2023
Fixes: nodejs#49224
Added old way of readline async iterator to benchmark.
nodejs-github-bot pushed a commit that referenced this issue Sep 23, 2023
ruyadorno pushed a commit that referenced this issue Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. test Issues and PRs related to the tests.
Projects
None yet
5 participants