From 34389e1d5472f458934b0af2a15192e2b8d83e1e Mon Sep 17 00:00:00 2001 From: "Alexis (Poliorcetics) Bourget" Date: Sat, 15 Oct 2022 19:16:17 +0200 Subject: [PATCH] nit: Do less allocations in `ui::menu::Item::label` implementations This complicates the code a little but it often divides by two the number of allocations done by the functions. LSP labels especially can easily be called dozens of time in a single menu popup, when listing references for example. --- helix-term/src/commands.rs | 25 +++++++------ helix-term/src/commands/lsp.rs | 66 ++++++++++++++++++---------------- 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index e44b851e5..468e9814d 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -54,7 +54,7 @@ use crate::{ use crate::job::{self, Jobs}; use futures_util::StreamExt; -use std::{collections::HashMap, fmt, future::Future}; +use std::{collections::HashMap, fmt, fmt::Write, future::Future}; use std::{collections::HashSet, num::NonZeroUsize}; use std::{ @@ -2416,19 +2416,18 @@ impl ui::menu::Item for MappableCommand { type Data = ReverseKeymap; fn label(&self, keymap: &Self::Data) -> Spans { - // formats key bindings, multiple bindings are comma separated, - // individual key presses are joined with `+` let fmt_binding = |bindings: &Vec>| -> String { - bindings - .iter() - .map(|bind| { - bind.iter() - .map(|key| key.to_string()) - .collect::>() - .join("+") - }) - .collect::>() - .join(", ") + bindings.iter().fold(String::new(), |mut acc, bind| { + if !acc.is_empty() { + acc.push_str(", "); + } + bind.iter().fold(false, |needs_plus, key| { + write!(&mut acc, "{}{}", if needs_plus { "+" } else { "" }, key) + .expect("Writing to a string can only fail on an Out-Of-Memory error"); + true + }); + acc + }) }; match self { diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 987fc4ce1..3c72cd2a5 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -18,7 +18,9 @@ use crate::{ }, }; -use std::{borrow::Cow, cmp::Ordering, collections::BTreeMap, path::PathBuf, sync::Arc}; +use std::{ + borrow::Cow, cmp::Ordering, collections::BTreeMap, fmt::Write, path::PathBuf, sync::Arc, +}; /// Gets the language server that is attached to a document, and /// if it's not active displays a status message. Using this macro @@ -43,23 +45,32 @@ impl ui::menu::Item for lsp::Location { type Data = PathBuf; fn label(&self, cwdir: &Self::Data) -> Spans { - let file: Cow<'_, str> = (self.uri.scheme() == "file") - .then(|| { - self.uri - .to_file_path() - .map(|path| { - // strip root prefix - path.strip_prefix(&cwdir) - .map(|path| path.to_path_buf()) - .unwrap_or(path) - }) - .map(|path| Cow::from(path.to_string_lossy().into_owned())) - .ok() - }) - .flatten() - .unwrap_or_else(|| self.uri.as_str().into()); - let line = self.range.start.line; - format!("{}:{}", file, line).into() + // The preallocation here will overallocate a few characters since it will account for the + // URL's scheme, which is not used most of the time since that scheme will be "file://". + // Those extra chars will be used to avoid allocating when writing the line number (in the + // common case where it has 5 digits or less, which should be enough for a cast majority + // of usages). + let mut res = String::with_capacity(self.uri.as_str().len()); + + if self.uri.scheme() == "file" { + // With the preallocation above and UTF-8 paths already, this closure will do one (1) + // allocation, for `to_file_path`, else there will be two (2), with `to_string_lossy`. + let mut write_path_to_res = || -> Option<()> { + let path = self.uri.to_file_path().ok()?; + res.push_str(&path.strip_prefix(&cwdir).unwrap_or(&path).to_string_lossy()); + Some(()) + }; + write_path_to_res(); + } else { + // Never allocates since we declared the string with this capacity already. + res.push_str(self.uri.as_str()); + } + + // Most commonly, this will not allocate, especially on Unix systems where the root prefix + // is a simple `/` and not `C:\` (with whatever drive letter) + write!(&mut res, ":{}", self.range.start.line) + .expect("Will only failed if allocating fail"); + res.into() } } @@ -73,10 +84,8 @@ impl ui::menu::Item for lsp::SymbolInformation { } else { match self.location.uri.to_file_path() { Ok(path) => { - let relative_path = helix_core::path::get_relative_path(path.as_path()) - .to_string_lossy() - .into_owned(); - format!("{} ({})", &self.name, relative_path).into() + let get_relative_path = path::get_relative_path(path.as_path()); + format!("{} ({})", &self.name, get_relative_path.to_string_lossy()).into() } Err(_) => format!("{} ({})", &self.name, &self.location.uri).into(), } @@ -115,24 +124,21 @@ impl ui::menu::Item for PickerDiagnostic { // remove background as it is distracting in the picker list style.bg = None; - let code = self + let code: Cow<'_, str> = self .diag .code .as_ref() .map(|c| match c { - NumberOrString::Number(n) => n.to_string(), - NumberOrString::String(s) => s.to_string(), + NumberOrString::Number(n) => n.to_string().into(), + NumberOrString::String(s) => s.as_str().into(), }) - .map(|code| format!(" ({})", code)) .unwrap_or_default(); let path = match format { DiagnosticsFormat::HideSourcePath => String::new(), DiagnosticsFormat::ShowSourcePath => { - let path = path::get_truncated_path(self.url.path()) - .to_string_lossy() - .into_owned(); - format!("{}: ", path) + let path = path::get_truncated_path(self.url.path()); + format!("{}: ", path.to_string_lossy()) } };