Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

gitserver(fs): Remove caching of cloned state#63066

Merged
eseliger merged 1 commit into
mainfrom
es/06-04-gitserverfsremovecachingofclonedstate
Jun 4, 2024
Merged

gitserver(fs): Remove caching of cloned state#63066
eseliger merged 1 commit into
mainfrom
es/06-04-gitserverfsremovecachingofclonedstate

Conversation

@eseliger

@eseliger eseliger commented Jun 4, 2024

Copy link
Copy Markdown
Member

I thought it might be nice to save on a few IOPS from the Stat calls to check if a repo is cloned by caching it for a bit. But turns out that the mutex locking and unlocking is more expensive than the stat operation.

We can come back here later and see if we can optimize it, we never had this in the past so this will be no regression.

Some background on mutex performance: https://stackoverflow.com/questions/57562606/why-does-sync-mutex-largely-drop-performance-when-goroutine-contention-is-more-t

Screenshot 2024-06-04 at 09.51.29@2x.png

Screenshot 2024-06-04 at 09.51.52@2x.png

Closes SRC-322

Test plan:

Without the cache, the tests still pass.

I thought it might be nice to save on a few IOPS from the Stat calls to check if a repo is cloned by caching it for a bit. But turns out that the mutex locking and unlocking is more expensive than the stat operation.

We can come back here later and see if we can optimize it, we never had this in the past so this will be no regression.

Some background on mutex performance: https://stackoverflow.com/questions/57562606/why-does-sync-mutex-largely-drop-performance-when-goroutine-contention-is-more-t

Closes SRC-322

Test plan:

Without the cache, the tests still pass.
@cla-bot cla-bot Bot added the cla-signed label Jun 4, 2024

eseliger commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite Graphite

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jun 4, 2024
@eseliger eseliger marked this pull request as ready for review June 4, 2024 07:58
@eseliger eseliger requested a review from a team June 4, 2024 07:58

@ggilmore ggilmore left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can try using lock striping as a strategy to reduce contention.

@eseliger eseliger merged commit 54472b7 into main Jun 4, 2024
@eseliger eseliger deleted the es/06-04-gitserverfsremovecachingofclonedstate branch June 4, 2024 20:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants