60 fps progress spinner #6446
60 fps progress spinner #6446
Conversation
Move all private methods from Spinner to ProgressSpinners so the API is simpler and user does not have to worry about the underlying interface if it uses HashMap or other methods, making consumer APIs more ergonomic and allows more refactoring, given that I am thinking to reduce the size of Spinner and inline all the stuff inside directly. At the same time, the checks for Spinner::is_stopped is removed, the call is redundant and user can just call start and stop without the check instead.
Put the start time directly into ProgressSpinner, always reuse the same interval and progress spinner icons.
This changes the x86 instructions from
push rax
mov edx, 80
xor ecx, ecx
call qword ptr [rip + __udivti3@GOTPCREL]
To
mov rax, rdi
shr eax, 7
While still being usable and see almost no difference.
https://godbolt.org/z/YE3vz1Ynz
Add a interval timer to main event loop. Check if there are any spinners and do the spinning job without waiting for progress event so spinner does not look stuck waiting for event. Not sure if there is a better place to add this, I was looking at the redraw notification but I think it is better to keep it separated so in the future we can separate both easier.
Try and reduce unnecessary renders for progress.
|
Looks very cool a nice visual improvement 👍 However I am a bit iffy about running the rendering code this often. Right now it's mostly fine to always redraw everything because usually only the editor is visible and that needs to be redrawn on every command. But with animations that's. a lot of wasted CPU cycles especially for larger files where TS can get slow or even just transversing the document at all for rendering could be slow (altough that will be improved with chunked softwrapping). These cass would also make the animation laggy. This issue actually already exists in a limited capacity. Scrolling in the picker or typing in command mode can lag if a very large file is opened in the background (altough ebersince the TS query performance improvements that's less of an issue in practice). I think we can learn from UI libraries here: The compositor should support invalidating components so they can be redraw individually. The easiest implementation right now would simply redraw all componenents that come after the first invalidated componenent (om top of the last render). In most cases we would redraw everything because most cases where we redraw right now invalidate the editor. For compositor events we can simply invalidate the components that consume them. In this specific case that wouldn't quite work because the status line is not it's own componemnnt. That is kind of unfortunate but we can probably do a workaround inside the editor rendering code |
|
I like the consistent spin interval. Though I think having it refresh at a high rate could cause issues when using helix via ssh. |
Helix computes a diff when rendering and only.emits events for cells that actually changed so during the animation only a single cell should update which would take very little bandwidth |
|
But I am surprised that even though I limited the render rate, having just sending the emitting a regular event every 16ms is causing a 2% cpu usage even though it only renders 8 frame per second. Maybe somewhere in tokio select it is using the cpu. |
|
Either way it's a lot of operations per cycle since the entire screen is recalculated (but then the render diffing just updates a single character). This is a 👎🏻 for me: it'll waste battery life and the stuttering is actually useful to me too as a sort of a progress bar: it means the LSP is stuck on a certain step. This would at the very least need the "damage tracking"/region invalidation updates to the compositor that @pascalkuthe is recommending, but I also think it's fine to not have a 60 FPS spinner on the terminal frontend. |
|
I am thinking of putting 8 fps just enough to render the progress bar and with the event being sent only when there are any progress bar. Okay, I will add region invalidation too. |
|
Please test this over a mosh connection as well - my experience with mosh is that it does some weird optimizations with character rendering over high-latency connections. Thanks, |
FWIW. If the spinner is running your LSP is probably busy at work so CPUs are not going to be idling. But I think 60 fps in a CLI is a complete overkill. 8fps seems more reasonable, but I'd be fine even with 1fps, just as long as I can tell that LSP is still spinning instead of scratching my head. |
|
Is this only an aesthetic improvement? In my mind, the current spinner presents semantic information that is lost if it always updates:
But I don't really need the spinner to tell me whether the UI is frozen or not, since there are already other ways to tell that (i.e. the ui doesn't respond to input). I guess this is all to say, could this be a configurable preference, if it is implemented? |
I just looked at the spinners behavior on our 100k LoC Rust project, and I think the main problem is that there is no distinction between spinner waiting for LSP and just being idle. When I press a key and IMO, the spinner should either disappear or turn into something else when there are no more outstanding LSP request. Possibly a lot of what I considered "stuck" was actually already idle. |
Previous spinner kinda look a bit weird given that it needs to wait for progress to spin, makes the editor feels laggy.
Now it spins at 60 fps which makes the editor look fast.
Not sure if there is a better way to do the interval timer at commit fb572fb, not sure if there are any performance issues either. I did see a 2% cpu usage when idle but not sure how to prevent that, maybe render only within a period of time rather than all 8 frames?
The pause at the end to remove the progress message looks weird due to the idle timeout, I was thinking to remove it, but then I figure out if the last message is an error, user should see it, but so far I only see rust-analyzer outputting completed message, so not removing it as I am not sure.
cc @vv9k