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

fix constant updating of progressbar #488

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

untitaker
Copy link
Collaborator

In other issues I've already lamented how slow lychee is when used
without -n. This fixes an issue where without -n, lychee would take
1 minute instead of 4 seconds to check sentry-docs.

In other issues I've already lamented how slow lychee is when used
without `-n`. This fixes an issue where without `-n`, lychee would take
1 minute instead of 4 seconds to check sentry-docs.
@mre
Copy link
Member

mre commented Feb 4, 2022

Yeah indicatif is really challenged with many concurrent requests.
Will this change cause the progress bar to "stutter" or is it fine?

@lebensterben
Copy link
Member

Actually running faster is more important than progress bar looks faster....

@untitaker
Copy link
Collaborator Author

untitaker commented Feb 4, 2022 via email

@untitaker
Copy link
Collaborator Author

I might have forgotten to recompile. But I think you should try yiurself and decide if it feels right. We can also set draw-rate to 100 and probably get the same effect

@mre
Copy link
Member

mre commented Feb 4, 2022

I will benchmark it tonight. Sounds promising! 😄

@lebensterben
Copy link
Member

"cargo" itself updates per 100/500 ticks:

  • Initially it wait for 500 ms to prevent short lived operation from flickering.
  • When a progress bar actually starts moving, it updates once per 100 ms.

@untitaker
Copy link
Collaborator Author

untitaker commented Feb 4, 2022

i just checked again, and realized I used the wrong units for set_draw_rate, i.e. i was limiting the updates to 500/second, so once every 2ms. I pasted in @lebensterben's values and I still can't see a significant difference, visually. Maybe I just have no taste. Now lychee should roughly do the same as cargo.

@mre
Copy link
Member

mre commented Feb 4, 2022

Thanks for adding the comments.
Reading through the documentation, I wonder if we even need enable_steady_tick:

Spawns a background thread to tick the progress bar
When this is enabled a background thread will regularly tick the progress bar in the given interval (in milliseconds). This is useful to advance progress bars that are very slow by themselves.

Our progress bar should not be slow. 😆
Did you try to remove that entirely?
A tick automatically happens on every change to a progress bar.
The only situation where this might not be enough is when we're waiting for the last few requests that we'd have to retry. 🤔 Overall it might not be worth the extra testing, but I wanted to put this out there at least.

@untitaker
Copy link
Collaborator Author

A tick automatically happens on every change to a progress bar.

Well not anymore now, that's the point, right? But yeah it's probably unlikely that lychee gets into a situation where steady tick does something... unless it is literally stuck on a single request taking forever.

@mre
Copy link
Member

mre commented Feb 4, 2022

unless it is literally stuck on a single request taking forever.

yeah, let's keep the steady tick. Users might be confused if the tool is "frozen" when it gets stuck on a slow website.

@untitaker
Copy link
Collaborator Author

Seems like indicatif has their performance story mostly sorted out on master: console-rs/indicatif#363

@mre mre merged commit d8305f7 into lycheeverse:master Feb 7, 2022
@mre
Copy link
Member

mre commented Feb 7, 2022

Benchmarks from my machine:
master: 12.722 seconds
this branch: 1.374 seconds
Nice improvement indeed. 🚀

@mre
Copy link
Member

mre commented Feb 7, 2022

Thanks @untitaker. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants