tldr-pages
Way faster without spinner
Description
I'm creating a repository to benchmark various tldr implementation (https://github.com/buonomo/tldr-benchmarks). RN, the node client is in the slow category and I'm not even running the benchmark for it as time for a tldr --update was >2mn. I've investigated a bit with 0x and some googling, and it seems like the spinner is actually the cause. When removing it I end up having a 5s update time, which is the time for an average client. So this PR is just removing this slow timer.
Checklist
Please review this checklist before submitting a pull request.
- [x] Code compiles correctly
- [ ] Created tests, if possible
- [x] All tests passing (
npm run test:all) - [ ] Extended the README / documentation, if necessary
I see that the latest version of ora is 8.2.0. Would you be able to run your benchmarks using the latest major version and see if there's any improvement?
I see that the latest version of ora is 8.2.0. Would you be able to run your benchmarks using the latest major version and see if there's any improvement?
I already tried ora 8.2 and yocto-spinner (supposedly fast and tiny according to ora's doc). Both were slow. I also tried yocto-spinner with an interval of 800ms instead of 80. TBH I don't understand how this can be so slow. I've looked at yocto's code, it is just an interval and some printing...
Thanks. Very interesting. I hope it's not blocking the event loop in any way. Because at the end of the update, we have an index rebuilding phase which is purely CPU work. So if the spinner is blocking a lot of that, then it gets slow. But even then, it shouldn't be so much slow as you are reporting.
Thanks. Very interesting. I hope it's not blocking the event loop in any way. Because at the end of the update, we have an index rebuilding phase which is purely CPU work. So if the spinner is blocking a lot of that, then it gets slow. But even then, it shouldn't be so much slow as you are reporting.
I experienced the same issue when updating the cache (last time I used the node client on Windows, currently I use the Rust one) where cache update would take a lot of time >2 minutes even on fairly recent hardware that I have. It never occurred to me that the spinner could be the main reason for it to be very slow.
Right. I think the spinner is not meant to be used when some CPU intensive work which does not yield to the event loop is running. Let's remove it.
@sebastiaanspeck I've seen you finally rebased yourself. Shall I let you handle the PR from now on?
@sebastiaanspeck I've seen you finally rebased yourself. Shall I let you handle the PR from now on?
Feel free to take the PR back if you believe any changes are still needed.
Well I saw that tests were failing, but looking at the test it seems completely unrelated, isn't it? Besides that, I have nothing in mind
Well I saw that tests were failing, but looking at the test it seems completely unrelated, isn't it? Besides that, I have nothing in mind
Yeah, I see #517 failed as well yesterday. In #517 he thought it was related to Cloudflare issues yesterday.
Could you ping me whenever you publish the new version, so I can update the benchmark :)