Stop cloning the index state #200
Conversation
|
@Byron Just tagging you because you might get a laugh at this. What a rookie mistake 🤦 Running this locally on a shallow clone of the linux repo, performance is now comparable to v0.6.0. |
This drastically improves performance by no longer cloning the index state, which only needs to be set once. Resolves #197
spenserblack
force-pushed
the
perf/do-not-clone-index-state
branch
from
dd142fb to
1f4af1b
Compare
Codecov Report
@@ Coverage Diff @@
## main #200 +/- ##
==========================================
- Coverage 82.69% 82.66% -0.04%
==========================================
Files 16 16
Lines 572 571 -1
==========================================
- Hits 473 472 -1
Misses 99 99
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
If this is a fix then I don't see it, but maybe it's faster than it was before and that's what this PR is about.
&self here must be &mut self, or there must be other means to access thread-local state mutably. Usually, this is passed in then.
To repeat: certain state must be present once per thread to be able to mutate it, and cloning isn't the solution.
I hope that helps.
| .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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for the feedback! I was just expecting a 😆 emoji.
I probably didn't interpret your code correctly. This was adapted from this line:
Line 140 in 14fb5c3
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.
There was a problem hiding this comment.
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 :)!
There was a problem hiding this comment.
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!
The new |
|
I hope this will be fixed - it's not problem to achieve what needs to be done. Pretty please 🥺. |
|
Yeah, please don't consider the merge to be an end to the conversation. But this PR alone dropped |
|
So this conversation doesn't get forgotten #202 😉 |
This drastically improves performance by no longer cloning the index
state, which only needs to be set once.
Resolves #197
Closes #199