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.

Sign up for GitHub

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

use redraw handle for debouncing LSP messages #7538

Merged
merged 1 commit into from
Jul 7, 2023
Merged

use redraw handle for debouncing LSP messages #7538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions helix-term/src/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use std::{
io::{stdin, stdout},
path::Path,
sync::Arc,
time::{Duration, Instant},
};

use anyhow::{Context, Error};
Expand All @@ -45,8 +44,6 @@ use {signal_hook::consts::signal, signal_hook_tokio::Signals};
#[cfg(windows)]
type Signals = futures_util::stream::Empty<()>;

const LSP_DEADLINE: Duration = Duration::from_millis(16);

#[cfg(not(feature = "integration"))]
use tui::backend::CrosstermBackend;

Expand Down Expand Up @@ -76,7 +73,6 @@ pub struct Application {
signals: Signals,
jobs: Jobs,
lsp_progress: LspProgressMap,
last_render: Instant,
}

#[cfg(feature = "integration")]
Expand Down Expand Up @@ -253,7 +249,6 @@ impl Application {
signals,
jobs: Jobs::new(),
lsp_progress: LspProgressMap::new(),
last_render: Instant::now(),
};

Ok(app)
Expand Down Expand Up @@ -300,7 +295,6 @@ impl Application {
S: Stream<Item = crossterm::Result<crossterm::event::Event>> + Unpin,
{
self.render().await;
self.last_render = Instant::now();

loop {
if !self.event_loop_until_idle(input_stream).await {
Expand Down Expand Up @@ -609,12 +603,7 @@ impl Application {
EditorEvent::LanguageServerMessage((id, call)) => {
self.handle_language_server_message(call, id).await;
// limit render calls for fast language server messages
let last = self.editor.language_servers.incoming.is_empty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context: the is_empty here checks that the set of streams is empty rather than that there is no pending message ready to be handled. Detecting that case is a little involved and not very effective for debouncing (see #7373 (comment))


if last || self.last_render.elapsed() > LSP_DEADLINE {
self.render().await;
self.last_render = Instant::now();
}
self.editor.redraw_handle.0.notify_one();
}
EditorEvent::DebuggerEvent(payload) => {
let needs_render = self.editor.handle_debugger_message(payload).await;
Expand Down
2 changes: 1 addition & 1 deletion helix-view/src/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,7 +1703,7 @@ impl Editor {
_ = self.redraw_handle.0.notified() => {
if !self.needs_redraw{
self.needs_redraw = true;
let timeout = Instant::now() + Duration::from_millis(96);
let timeout = Instant::now() + Duration::from_millis(33);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change here?

Copy link
Member Author

@pascalkuthe pascalkuthe Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only disadvantage of this approach was that the rendering now has a longer render timeout (96ms instead of 16ms) which felt a bit slow. I changed it to 33ms to emulate 30FPS. It doesn't matter too much for diff gutters what that value is since diff gutters only use this as a fallback mechanism but I would prefer to not lower this too much more to avoid adding too many renders.

#7373 (comment)

We were rendering with approximately 60fps in the past (the debouncer wasn't exact but we were using 16 ms to debounce). The redraw handle used to have a longer timeout since its only used as a fallback for the diff gutters (basically if we get here its already slow) but the exact value doesn't matter too much. I lowered it so we at least keep rending with 30 FPS. I didn't want to go quite as low as 16 here, if we really care we could allow different users of the redraw handle to specify different timeouts. That seemed overkill tough but if you think that's worth it, it shouldn't be too hard

if timeout < self.idle_timer.deadline(){
self.idle_timer.as_mut().reset(timeout)
}
Expand Down