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

avoid excessive memory consumption in picker #8127

Merged
merged 2 commits into from
Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions helix-term/src/compositor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub enum EventResult {
}

use crate::job::Jobs;
use crate::ui::picker;
use helix_view::Editor;

pub use helix_view::input::Event;
Expand Down Expand Up @@ -100,6 +101,11 @@ impl Compositor {

/// Add a layer to be rendered in front of all existing layers.
pub fn push(&mut self, mut layer: Box<dyn Component>) {
// immediately clear last_picker field to avoid excessive memory
// consumption for picker with many items
if layer.id() == Some(picker::ID) {
self.last_picker = None;
}
let size = self.size();
// trigger required_size on init
layer.required_size((size.width, size.height));
Expand Down
2 changes: 1 addition & 1 deletion helix-term/src/ui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub mod lsp;
mod markdown;
pub mod menu;
pub mod overlay;
mod picker;
pub mod picker;
pub mod popup;
mod prompt;
mod spinner;
Expand Down
48 changes: 33 additions & 15 deletions helix-term/src/ui/picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ use helix_view::{
Document, DocumentId, Editor,
};

pub const ID: &str = "picker";
use super::{menu::Item, overlay::Overlay};

pub const MIN_AREA_WIDTH_FOR_PREVIEW: u16 = 72;
Expand Down Expand Up @@ -802,11 +803,28 @@ impl<T: Item + 'static + Send + Sync> Component for Picker<T> {
_ => return EventResult::Ignored(None),
};

let close_fn =
EventResult::Consumed(Some(Box::new(|compositor: &mut Compositor, _ctx| {
// remove the layer
compositor.last_picker = compositor.pop();
})));
let close_fn = |picker: &mut Self| {
// if the picker is very large don't store it as last_picker to avoid
// excessive memory consumption
let callback: compositor::Callback = if picker.matcher.snapshot().item_count() > 100_000
{
Box::new(|compositor: &mut Compositor, _ctx| {
// remove the layer
compositor.pop();
})
} else {
// stop streaming in new items in the background, really we should
// be restarting the stream somehow once the picker gets
// reopened instead (like for an FS crawl) that would also remove the
// need for the special case above but that is pretty tricky
picker.shutdown.store(true, atomic::Ordering::Relaxed);
Box::new(|compositor: &mut Compositor, _ctx| {
// remove the layer
compositor.last_picker = compositor.pop();
})
};
EventResult::Consumed(Some(callback))
};

// So that idle timeout retriggers
ctx.editor.reset_idle_timer();
Expand All @@ -830,9 +848,7 @@ impl<T: Item + 'static + Send + Sync> Component for Picker<T> {
key!(End) => {
self.to_end();
}
key!(Esc) | ctrl!('c') => {
return close_fn;
}
key!(Esc) | ctrl!('c') => return close_fn(self),
alt!(Enter) => {
if let Some(option) = self.selection() {
(self.callback_fn)(ctx, option, Action::Load);
Expand All @@ -842,19 +858,19 @@ impl<T: Item + 'static + Send + Sync> Component for Picker<T> {
if let Some(option) = self.selection() {
(self.callback_fn)(ctx, option, Action::Replace);
}
return close_fn;
return close_fn(self);
}
ctrl!('s') => {
if let Some(option) = self.selection() {
(self.callback_fn)(ctx, option, Action::HorizontalSplit);
}
return close_fn;
return close_fn(self);
}
ctrl!('v') => {
if let Some(option) = self.selection() {
(self.callback_fn)(ctx, option, Action::VerticalSplit);
}
return close_fn;
return close_fn(self);
}
ctrl!('t') => {
self.toggle_preview();
Expand Down Expand Up @@ -882,6 +898,10 @@ impl<T: Item + 'static + Send + Sync> Component for Picker<T> {
self.completion_height = height.saturating_sub(4);
Some((width, height))
}

fn id(&self) -> Option<&'static str> {
Some(ID)
}
}
impl<T: Item> Drop for Picker<T> {
fn drop(&mut self) {
Expand All @@ -906,8 +926,6 @@ pub struct DynamicPicker<T: ui::menu::Item + Send + Sync> {
}

impl<T: ui::menu::Item + Send + Sync> DynamicPicker<T> {
pub const ID: &'static str = "dynamic-picker";

pub fn new(file_picker: Picker<T>, query_callback: DynQueryCallback<T>) -> Self {
Self {
file_picker,
Expand Down Expand Up @@ -939,7 +957,7 @@ impl<T: Item + Send + Sync + 'static> Component for DynamicPicker<T> {
let callback = Callback::EditorCompositor(Box::new(move |editor, compositor| {
// Wrapping of pickers in overlay is done outside the picker code,
// so this is fragile and will break if wrapped in some other widget.
let picker = match compositor.find_id::<Overlay<DynamicPicker<T>>>(Self::ID) {
let picker = match compositor.find_id::<Overlay<DynamicPicker<T>>>(ID) {
Some(overlay) => &mut overlay.content.file_picker,
None => return,
};
Expand All @@ -960,6 +978,6 @@ impl<T: Item + Send + Sync + 'static> Component for DynamicPicker<T> {
}

fn id(&self) -> Option<&'static str> {
Some(Self::ID)
Some(ID)
}
}