Skip to content

Conversation

@pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Jan 11, 2024

not anywhere near ready to merge. The general system is mostly complete.
What is missing is porting the entire editor to use the new config system and to add an adapter for the legacy serde config.

I already ported the language server configuration in 27b8257 which should help showoff how this works out in practice.

Rough todo list:

  • move helix_core::config to dedicated helix_config crate (feels more appropriate as the codebase has grown a lot
  • finis of serde_json::Value to helx_config::Value conversion (going though serde is likely unessesairily complicated implementing from directly should work)
  • create definitions for all config options
  • add compatibility layer with old serde config
  • migrate config loading
  • migrate access to config in codebase
  • reimplement config based commands (:set :get :toggle) with the new config system

@pascalkuthe pascalkuthe added the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Jan 11, 2024
@pascalkuthe pascalkuthe force-pushed the config_refactor branch 4 times, most recently from 8e88b06 to a1f2a3a Compare January 16, 2024 13:28
Comment on lines +1 to +5
/// 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)
Copy link
Member

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.

Copy link
Member Author

@pascalkuthe pascalkuthe Feb 1, 2024

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 access

Copy link
Member

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants