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

Faster startup by offloading glob matcher building to a worker thread #2868

Merged
merged 3 commits into from
Mar 10, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
- Update git-version dependency to use Syn v2, see #2816 (@dtolnay)
- Update git2 dependency to v0.18.2, see #2852 (@eth-p)
- Apply clippy fixes #2864 (@cyqsimon)
- Faster startup by offloading glob matcher building to a worker thread #2868 (@cyqsimon)

## Syntaxes

Expand Down
4 changes: 4 additions & 0 deletions src/bin/bat/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ impl App {
};

let mut syntax_mapping = SyntaxMapping::new();
// start building glob matchers for builtin mappings immediately
// this is an appropriate approach because it's statistically likely that
// all the custom mappings need to be checked
syntax_mapping.start_offload_build_all();

if let Some(values) = self.matches.get_many::<String>("ignored-suffix") {
for suffix in values {
Expand Down
41 changes: 40 additions & 1 deletion src/syntax_mapping.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
use std::path::Path;
use std::{
path::Path,
sync::{
atomic::{AtomicBool, Ordering},
Arc,
},
thread,
};

use globset::{Candidate, GlobBuilder, GlobMatcher};
use once_cell::sync::Lazy;

use crate::error::Result;
use builtin::BUILTIN_MAPPINGS;
Expand Down Expand Up @@ -44,14 +52,45 @@ pub struct SyntaxMapping<'a> {
///
/// Rules in front have precedence.
custom_mappings: Vec<(GlobMatcher, MappingTarget<'a>)>,

pub(crate) ignored_suffixes: IgnoredSuffixes<'a>,

/// A flag to halt glob matcher building, which is offloaded to another thread.
///
/// We have this so that we can signal the thread to halt early when appropriate.
halt_glob_build: Arc<AtomicBool>,
}

impl<'a> Drop for SyntaxMapping<'a> {
fn drop(&mut self) {
// signal the offload thread to halt early
self.halt_glob_build.store(true, Ordering::Relaxed);
}
}

impl<'a> SyntaxMapping<'a> {
pub fn new() -> SyntaxMapping<'a> {
Default::default()
}

/// Start a thread to build the glob matchers for all builtin mappings.
///
/// The use of this function while not necessary, is useful to speed up startup
/// times by starting this work early in parallel.
///
/// The thread halts if/when `halt_glob_build` is set to true.
pub fn start_offload_build_all(&self) {
let halt = Arc::clone(&self.halt_glob_build);
thread::spawn(move || {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had problems in the past with threads that are never joined. Are we sure this can not cause any problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this some thought, but in the end couldn't see any possible failure mode. The thread is so simple that it's trivial to reason about its exact behaviour.

I believe it's okay to forgo joining the thread because there's no resource the thread holds that needs to be released upon halting - BUILTIN_MAPPINGS is 'static, and the halt flag is safely behind an Arc. The only synchronization requirement is to make sure that it indeed does halt, which it obviously does.

If you prefer defensive coding just to be safe, I can do that too. But in this case I really don't think it's necessary.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently had a problem in another project of mine where a non-joined thread caused problems on Windows (sharkdp/numbat#298). But that might have been only due to the fact that the background thread was accessing the network(?).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I guess I'm okay with giving this a shot 😄. But those type of things can be hard to track down (spurious crashes that depend on precise timings).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently had a problem in another project of mine where a non-joined thread caused problems on Windows (sharkdp/numbat#298). But that might have been only due to the fact that the background thread was accessing the network(?).

Yeah in that case releasing the socket (a shared resource) would require synchronization. We don't have that problem here.

Anyway, I guess I'm okay with giving this a shot 😄. But those type of things can be hard to track down (spurious crashes that depend on precise timings).

I guess I'll add a comment in the threading code to remind future maintainers to watch out for synchronization issues if it ever starts to hold interesting resources in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, that's a really interesting project you've got there! When I've got time I would like to take a look at the possibility of integrating it with Krunner.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. Let me know how if you have problems with the current API. It's not really stable and/or particularly nice at the moment, but I have built several frontends for Numbat so far. And there is a 3rd party integration into a krunner-like launcher here: https://github.com/MatthiasGrandl/Loungy/blob/main/src/commands/root/numbat.rs

for (matcher, _) in BUILTIN_MAPPINGS.iter() {
if halt.load(Ordering::Relaxed) {
break;
}
Lazy::force(matcher);
}
});
}

pub fn insert(&mut self, from: &str, to: MappingTarget<'a>) -> Result<()> {
let matcher = make_glob_matcher(from)?;
self.custom_mappings.push((matcher, to));
Expand Down