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

Improve Compilation Performance/ Code Quality #103

Merged
merged 15 commits into from
Aug 21, 2024

Conversation

DaniD3v
Copy link
Collaborator

@DaniD3v DaniD3v commented Aug 18, 2024

Changelog

  • removed unused dependencies (cargo-machete)
  • removed unused dependency features (cargo-unused-features)
  • updated dependencies (cargo-update)
  • ran a project-wide cargo-fmt

  • feature gated some rarely used features
  • applied clippy lints
  • updated dependencies with breaking changes (To avoid compiling them twice. Check cargo tree -d --edges normal)
  • made enquote a mac-only dependency (#cfg(macos) -> #cfg(target_os = "macos"))) Enquote Failure for Mac Installation #43
  • renamed deprecated default_features to default-features

Some ideas on further code cleanup/ compile time reducation

See comments in Cargo.toml

ahash

I don't see any reason why we would need a custom hashing algorithm?
If there is a reason that should probably be documented.

In case it's just an optimization (which I'll assume because there's no other reason documented), there should at least be a benchmark or sth to prove it's necessary

indexmap

Not too sure about this one. If the data isn't too large we could probably get away with sorting a Vec?
Or even better, use a BTreeSet instead.

The IndexMap is only used once.

src/scheme.rs

3: use indexmap::IndexMap;

21:    pub light: IndexMap<String, material_colors::color::Argb, ahash::random_state::RandomState>,
22:    pub dark: IndexMap<String, material_colors::color::Argb, ahash::random_state::RandomState>,

92:        dark: IndexMap::from_iter(scheme_dark.into_iter().chain(custom_colors_dark)),
93:        light: IndexMap::from_iter(scheme_light.into_iter().chain(custom_colors_light)),

CHANGELOG.md says

use IndexMap for --show-colors table

Which I don't really understand because the tables color order is deterministic anyways?

resolve-path

This one is only used once in this function

src/util/template.rs

fn get_absolute_paths(
    config_path: &Option<PathBuf>,
    template: &Template,
) -> Result<(PathBuf, PathBuf), Report> {
    let (input_path_absolute, output_path_absolute) = if config_path.is_some() {
        let base = std::fs::canonicalize(config_path.as_ref().unwrap())?;
        (
            template
                .input_path
                .try_resolve_in(&base)?
                .to_path_buf()
                .strip_canonicalization(),
            template
                .output_path
                .try_resolve_in(&base)?
                .to_path_buf()
                .strip_canonicalization(),
        )
    } else {
        (
            template.input_path.try_resolve()?.to_path_buf(),
            template.output_path.try_resolve()?.to_path_buf(),
        )
    };
    Ok((input_path_absolute, output_path_absolute))
}

Again this functions documentation could be improved.
Why would you ever want to resolve paths relative to the file the config is in?
Relative paths starting with . should clearly be prohibited.
There's no reasonable expected output you could have for them.

In Rust 1.79 they added std::path::absolute which we could maybe use instead.

Honestly tho I don't really understand why we even need to have an absolute path.

Code Quality Rant:

  • That function should not even exist. Just merge it into the impl of Template.
  • The file src/util/template.rs is badly named. Template.rs is not a utility at all. It's literally a main part of the functionality. Consider moving it into src/template.rs?

Sorry for going a little off-topic here but the code quality (especially of the part handling files) is a bit questionable.

execute

I didn't put too much research into that (partially because the hooks aren't documented),
but it feels like this could be done with std easily.

This is the only usecase (apart from traits)

src/exec/hook.rs

3:use execute::{shell, Execute};
33:    let mut command = shell(&res);

paris

The paris crate is way more popular than paris-log and we don't seem to require the log crate in any way
so why are we installing 3 dependencies instead of just using one?

Migrating this is a bit more effort but the Logger struct also looks more ergonomic to use than the macros we're currently using.

enquote and winapi

I'd suggest just using the wallpaper crate instead. It seems to be less hacky than the current macOS implementation.
For linux we could just keep the current code.

the wallpaper module currently has 179 LOCS, 120 of which are for the unix stuff. that means we could remove ~59 + we don't have to maintain windows and mac specific stuff. Sounds like a pretty good idea to me.

Benchmarks

Here's the compilation performance improvement

Default Features

Compilation Type Upstream This PR Percentage
Debug - Clean 66.99 43.46 -35.12%
Release - Clean 75.90 46.07 -33.30%
Debug - Incremental 0.16 0.12 -25%
Release - Incremental 0.19 0.15 -21.05%

All Features

Compilation Type Upstream This PR Percentage
Debug - Clean 66.99 56.18 -16.13%
Release - Clean 75.90 68.56 -9.67%
Debug - Incremental 0.16 0.12 -25%
Release - Incremental 0.19 0.12 -36.84%

@DaniD3v DaniD3v changed the title Improve Compilation Performance Improve Compilation Performance/ Code Quality Aug 18, 2024
@InioX
Copy link
Owner

InioX commented Aug 19, 2024

Thanks for all the ideas, I'll try to add them tomorrow.

@InioX InioX merged commit 9b33071 into InioX:main Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants