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.

Sign up for GitHub

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

Stop cloning the index state #200

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Stop cloning the index state #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ resolver = "2"

[workspace.package]
description = "Get the language distribution stats of your repository"
version = "0.7.0"
version = "0.7.1"
edition = "2021"
repository = "https://github.com/spenserblack/gengo"
readme = "README.md"
Expand Down
18 changes: 8 additions & 10 deletions gengo/src/file_source/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,23 +107,22 @@ impl<'repo> FileSource<'repo> for Git {

fn overrides<O: AsRef<Path>>(&self, path: O) -> Overrides {
let repo = self.repository.to_thread_local();
let state = {
let mut state = self.state.clone();
let Ok(platform) = state
.attr_stack
.at_path(path, Some(false), |id, buf| repo.objects.find_blob(id, buf))
let attr_matches = {
let mut attr_stack = self.state.attr_stack.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Cloning attr-stack and attr-matches per path isn't the intended use - these need to be mutable. Creating a thread-local repo per path also isn't intended.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I was just expecting a 😆 emoji.

I probably didn't interpret your code correctly. This was adapted from this line:

gengo/gengo/src/lib.rs

Line 140 in 14fb5c3

move |_| (state.clone(), repo.to_thread_local()),

A GitState, which has attr_stack and attr_matches fields, gets cloned here to attain a mutable reference (or so I thought). I thought this closure would do exactly that: return a thread-local repo and state clone per entry, which is effectively per-path for our uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line could be doing that, that's true. But what I see here still happily clones two structures in order to make them mutable. Also, repo.to_thread_local() is called once per path.

The problem is that the whole self seems to be tuned to be Sync and thus is read-only on top of that. The one coordinating the parallelism is responsible for passing in mutable state. I get it, each implementation has its own state, but if that's the case it must be a type parameter somewhere so it can be instantiated and passed in mutably.

Maybe this happened due to rayon which clearly prefers Sync (share immutable state or shared mutable state behind locks), but even with rayon there are ways to get thread-local state that I strongly recommend getting into this method call.

This really is all I can say here, so: Happy Rusting :)!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Alright, thanks again! I do have a habit of avoiding mutable references in favor of immutable references, which influenced my design decisions here. Thanks for the insight!

let mut attr_matches = self.state.attr_matches.clone();
let Ok(platform) =
attr_stack.at_path(path, Some(false), |id, buf| repo.objects.find_blob(id, buf))
else {
// NOTE If we cannot get overrides, simply don't return them.
return Default::default();
};
platform.matching_attributes(&mut state.attr_matches);
state
platform.matching_attributes(&mut attr_matches);
attr_matches
};

let attrs = {
let mut attrs = [None, None, None, None, None];
state
.attr_matches
attr_matches
.iter_selected()
.zip(attrs.iter_mut())
.for_each(|(info, slot)| {
Expand Down Expand Up @@ -185,7 +184,6 @@ impl<'repo> Iterator for Iter<'repo> {
}
}

#[derive(Clone)]
struct State {
attr_stack: WTStack,
attr_matches: AttrOutcome,
Expand Down