-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
config system prototype #9318
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
Draft
pascalkuthe
wants to merge
5
commits into
master
Choose a base branch
from
config_refactor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
config system prototype #9318
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5e74d3c
config system prototype
pascalkuthe 7ba8674
define new config options
pascalkuthe fb13130
migrate language server config to new config system
pascalkuthe dcbe849
remove unused languag.scope option
pascalkuthe 87266b0
replace serde structs with helix-config toml adapter
pascalkuthe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| resolver = "2" | ||
| members = [ | ||
| "helix-core", | ||
| "helix-config", | ||
| "helix-view", | ||
| "helix-term", | ||
| "helix-tui", | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| [package] | ||
| name = "helix-config" | ||
| description = "Helix editor core editing primitives" | ||
| include = ["src/**/*", "README.md"] | ||
| version.workspace = true | ||
| authors.workspace = true | ||
| edition.workspace = true | ||
| license.workspace = true | ||
| rust-version.workspace = true | ||
| categories.workspace = true | ||
| repository.workspace = true | ||
| homepage.workspace = true | ||
|
|
||
| [dependencies] | ||
| ahash = "0.8.6" | ||
| hashbrown = { version = "0.14.3", features = ["raw"] } | ||
| parking_lot = "0.12" | ||
| anyhow = "1.0.79" | ||
| indexmap = { version = "2.1.0", features = ["serde"] } | ||
| serde = { version = "1.0" } | ||
| serde_json = "1.0" | ||
| globset = "0.4.14" | ||
| regex = "1.10.2" | ||
| regex-syntax = "0.8.2" | ||
| which = "5.0.0" | ||
|
|
||
| regex-syntax = "0.8.2" | ||
| which = "5.0.0" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /// this is a reimplementation of dynamic dispatch that only stores the | ||
| /// information we need and stores everythin inline. Values that are smaller or | ||
| /// the same size as a slice (2 usize) are also stored inline. This avoids | ||
| /// significant overallocation when setting lots of simple config | ||
| /// options (integers, strings, lists, enums) | ||
| use std::any::{Any, TypeId}; | ||
| use std::mem::{align_of, size_of, MaybeUninit}; | ||
|
|
||
| pub struct ConfigData { | ||
| data: MaybeUninit<[usize; 2]>, | ||
| ty: TypeId, | ||
| drop_fn: unsafe fn(MaybeUninit<[usize; 2]>), | ||
| } | ||
|
|
||
| const fn store_inline<T>() -> bool { | ||
| size_of::<T>() <= size_of::<[usize; 2]>() && align_of::<T>() <= align_of::<[usize; 2]>() | ||
| } | ||
|
|
||
| impl ConfigData { | ||
| unsafe fn drop_impl<T: Any>(mut data: MaybeUninit<[usize; 2]>) { | ||
| if store_inline::<T>() { | ||
| data.as_mut_ptr().cast::<T>().drop_in_place(); | ||
| } else { | ||
| let ptr = data.as_mut_ptr().cast::<*mut T>().read(); | ||
| drop(Box::from_raw(ptr)); | ||
| } | ||
| } | ||
|
|
||
| pub fn get<T: Any>(&self) -> &T { | ||
| assert_eq!(TypeId::of::<T>(), self.ty); | ||
| unsafe { | ||
| if store_inline::<T>() { | ||
| return &*self.data.as_ptr().cast(); | ||
| } | ||
| let data: *const T = self.data.as_ptr().cast::<*const T>().read(); | ||
| &*data | ||
| } | ||
| } | ||
| pub fn new<T: Any>(val: T) -> Self { | ||
| let mut data = MaybeUninit::uninit(); | ||
| if store_inline::<T>() { | ||
| let data: *mut T = data.as_mut_ptr() as _; | ||
| unsafe { | ||
| data.write(val); | ||
| } | ||
| } else { | ||
| assert!(store_inline::<*const T>()); | ||
| let data: *mut *const T = data.as_mut_ptr() as _; | ||
| unsafe { | ||
| data.write(Box::into_raw(Box::new(val))); | ||
| } | ||
| }; | ||
| Self { | ||
| data, | ||
| ty: TypeId::of::<T>(), | ||
| drop_fn: ConfigData::drop_impl::<T>, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for ConfigData { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| (self.drop_fn)(self.data); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Debug for ConfigData { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("ConfigData").finish_non_exhaustive() | ||
| } | ||
| } | ||
|
|
||
| unsafe impl Send for ConfigData {} | ||
| unsafe impl Sync for ConfigData {} | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| use crate::any::ConfigData; | ||
| use crate::validator::Ty; | ||
| use crate::Value; | ||
|
|
||
| pub trait IntoTy: Clone { | ||
| type Ty: Ty; | ||
| fn into_ty(self) -> Self::Ty; | ||
| } | ||
|
|
||
| impl<T: Ty> IntoTy for T { | ||
| type Ty = Self; | ||
|
|
||
| fn into_ty(self) -> Self::Ty { | ||
| self | ||
| } | ||
| } | ||
| impl<T: IntoTy> IntoTy for &[T] { | ||
| type Ty = Box<[T::Ty]>; | ||
|
|
||
| fn into_ty(self) -> Self::Ty { | ||
| self.iter().cloned().map(T::into_ty).collect() | ||
| } | ||
| } | ||
| impl<T: IntoTy, const N: usize> IntoTy for &[T; N] { | ||
| type Ty = Box<[T::Ty]>; | ||
|
|
||
| fn into_ty(self) -> Self::Ty { | ||
| self.iter().cloned().map(T::into_ty).collect() | ||
| } | ||
| } | ||
|
|
||
| impl IntoTy for &str { | ||
| type Ty = Box<str>; | ||
|
|
||
| fn into_ty(self) -> Self::Ty { | ||
| self.into() | ||
| } | ||
| } | ||
|
|
||
| pub(super) fn ty_into_value<T: Ty>(val: &ConfigData) -> Value { | ||
| T::to_value(val.get()) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| use std::time::Duration; | ||
|
|
||
| use crate::*; | ||
|
|
||
| mod language; | ||
| mod lsp; | ||
| mod ui; | ||
|
|
||
| pub use lsp::init_language_server_config; | ||
|
|
||
| options! { | ||
| use ui::*; | ||
| use lsp::*; | ||
| use language::*; | ||
|
|
||
| struct WrapConfig { | ||
| /// Soft wrap lines that exceed viewport width. | ||
| enable: bool = false, | ||
| /// Maximum free space left at the end of the line. | ||
| /// Automatically limited to a quarter of the viewport. | ||
| max_wrap: u16 = 20, | ||
| /// Maximum indentation to carry over when soft wrapping a line. | ||
| /// Automatically limited to a quarter of the viewport. | ||
| max_indent_retain: u16 = 40, | ||
| /// Text inserted before soft wrapped lines, highlighted with `ui.virtual.wrap`. | ||
| wrap_indicator: String = "↪", | ||
| /// Soft wrap at `text-width` instead of using the full viewport size. | ||
| wrap_at_text_width: bool = false, | ||
| /// Maximum line length. Used for the `:reflow` command and | ||
| /// soft-wrapping if `soft-wrap.wrap-at-text-width` is set | ||
| text_width: usize = 80, | ||
| } | ||
|
|
||
| struct MouseConfig { | ||
| /// Enable mouse mode | ||
| #[read = copy] | ||
| mouse: bool = true, | ||
| /// Number of lines to scroll per scroll wheel step. | ||
| #[read = copy] | ||
| scroll_lines: usize = 3, | ||
| /// Middle click paste support | ||
| #[read = copy] | ||
| middle_click_paste: bool = true, | ||
| } | ||
| struct SmartTabConfig { | ||
| /// If set to true, then when the cursor is in a position with | ||
| /// non-whitespace to its left, instead of inserting a tab, it will run | ||
| /// `move_parent_node_end`. If there is only whitespace to the left, | ||
| /// then it inserts a tab as normal. With the default bindings, to | ||
| /// explicitly insert a tab character, press Shift-tab. | ||
| #[name = "smart-tab.enable"] | ||
| #[read = copy] | ||
| enable: bool = true, | ||
| /// Normally, when a menu is on screen, such as when auto complete | ||
| /// is triggered, the tab key is bound to cycling through the items. | ||
| /// This means when menus are on screen, one cannot use the tab key | ||
| /// to trigger the `smart-tab` command. If this option is set to true, | ||
| /// the `smart-tab` command always takes precedence, which means one | ||
| /// cannot use the tab key to cycle through menu items. One of the other | ||
| /// bindings must be used instead, such as arrow keys or `C-n`/`C-p`. | ||
| #[name = "smart-tab.supersede-menu"] | ||
| #[read = copy] | ||
| supersede_menu: bool = false, | ||
| } | ||
|
|
||
| struct SearchConfig { | ||
| /// Enable smart case regex searching (case-insensitive unless pattern | ||
| /// contains upper case characters) | ||
| #[name = "search.smart-case"] | ||
| #[read = copy] | ||
| smart_case: bool = true, | ||
| /// Whether the search should wrap after depleting the matches | ||
| #[name = "search.wrap-round"] | ||
| #[read = copy] | ||
| wrap_round: bool = true, | ||
| } | ||
|
|
||
| struct MiscConfig { | ||
| /// Number of lines of padding around the edge of the screen when scrolling. | ||
| #[read = copy] | ||
| scrolloff: usize = 5, | ||
| /// Shell to use when running external commands | ||
| #[read = deref] | ||
| shell: List<String> = if cfg!(windows) { | ||
| &["cmd", "/C"] | ||
| } else { | ||
| &["sh", "-c"] | ||
| }, | ||
| /// Enable automatic saving on the focus moving away from Helix. | ||
| /// Requires [focus event support](https://github.com/helix-editor/ | ||
| /// helix/wiki/Terminal-Support) from your terminal | ||
| #[read = copy] | ||
| auto_save: bool = false, | ||
| /// Whether to automatically insert a trailing line-ending on write | ||
| /// if missing | ||
| #[read = copy] | ||
| insert_final_newline: bool = true, | ||
| /// Time in milliseconds since last keypress before idle timers trigger. | ||
| /// Used for autocompletion, set to 0 for instant | ||
| #[read = copy] | ||
| idle_timeout: Duration = Duration::from_millis(250), | ||
| } | ||
| } | ||
|
|
||
| impl Ty for Duration { | ||
| fn from_value(val: Value) -> anyhow::Result<Self> { | ||
| let val: usize = val.typed()?; | ||
| Ok(Duration::from_millis(val as _)) | ||
| } | ||
| fn to_value(&self) -> Value { | ||
| Value::Int(self.as_millis().try_into().unwrap()) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is avoiding an allocation for every config item the only reason we're doing this? Has there been a known performance bottleneck on config memory access?
Trying to understand where this is coming from. This is much harder to understand than just boxing everything, and I was not under the impression that config was in a hot path.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually do kind of think its pretty hot. We read config fields on every single frame for a large amount of config options (at least a hundred) sometimes multiple times. It also reduces memory consumption quite a bit which is more relevant if each config option is set as default, by the user per languages and potentially on the document (so in total up to 4 times). Really this sort of thing ought to be built into rust. There are some efforts around that.
You don't really need to be aware of how this work tough just treat like
Box<dyn Any>when reading the rest of the code its really just optimizing accessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's true, I didn't consider they are read in the rendering hot loop, good point 👍