Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Improves git object cache #4685

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

Draft
arturcic wants to merge 11 commits into GitTools:main
base: main
Choose a base branch
Loading
from arturcic:fix/cache-git-objects

Conversation

@arturcic
Copy link
Member

@arturcic arturcic commented Oct 8, 2025

Follow up for #4681

Copy link
Contributor

HHobeck commented Oct 8, 2025

@pvanbuijtene: Thank you for your contribution and the work you have done.

I have following remarks/suggestions:

  • The classes Commit, Branch and so on are wrapper classes around the LibGit2Sharp objects. I would say the wrapper classes should not have a reference to GitRepostory. Is it an idea to extract the functionality of caching in a separate class and use this instead?
  • Because of the nature of a wrapper class I would expect that the LibGit2Sharp objects are reused as well and the hash code is the same (otherwise we would have still a memory problem). Probably you can use the hash code instead of using an arbitrary string (needs to be tested).

Copy link
Member Author

arturcic commented Oct 8, 2025

@pvanbuijtene: Thank you for your contribution and the work you have done.

I have following remarks/suggestions:

  • The classes Commit, Branch and so on are wrapper classes around the LibGit2Sharp objects. I would say the wrapper classes should not have a reference to GitRepostory. Is it an idea to extract the functionality of caching in a separate class and use this instead?
  • Because of the nature of a wrapper class I would expect that the LibGit2Sharp objects are reused as well and the hash code is the same (otherwise we would have still a memory problem). Probably you can use the hash code instead of using an arbitrary string (needs to be tested).

That was my idea to move the functionality from the GitCache class back into the GitRepository as I think the repository wrapper should handle the caching mechanism as it's the abstraction of the database which maintains the collections and the cache, but we can reconsider that

Copy link

Happy to help :)

@arturcic as I understand you will implement the suggestions, feel free to ping me in case you want to have some changes tested against a repository causing issues.

Copy link
Member Author

arturcic commented Oct 8, 2025

@pvanbuijtene thank you, yeah, I implemented the suggestions so far in this PR, but it's a draft for now, I want to test caching the other collections as well before having this merged

@arturcic arturcic force-pushed the fix/cache-git-objects branch 6 times, most recently from 7393ace to ec03709 Compare October 13, 2025 22:46
@arturcic arturcic changed the title (削除) improves git object cache (削除ここまで) (追記) Improves git object cache (追記ここまで) Oct 14, 2025
@arturcic arturcic force-pushed the fix/cache-git-objects branch 2 times, most recently from e78fba2 to 468ac8d Compare October 14, 2025 06:05
arturcic added 11 commits October 15, 2025 16:42
Simplifies branch/tag cache keys by including the remote name and SHA.
Renames `GetOrCreate` to `GetOrWrap` to better reflect its caching behavior.
refactors the project to improve code organization and maintainability.
Replaces several internal classes with readonly structs to improve performance by avoiding unnecessary heap allocations. Updates related code to ensure compatibility with the new struct-based implementation.
moves schema and testing projects to misc folder.
Uses lazy initialization to cache references and remotes.
Introduces `GitRepositoryCache` to centralize caching logic for branches, commits, tags, references, and remotes. Updates related classes to use `GitRepositoryCache` instead of directly maintaining individual cache dictionaries.
Replaces `Refs` with `References` across the codebase for better clarity and consistency. Updates all method calls, variable names, and interfaces to reflect this change.
Replaces direct instantiations of Tags, Commits, Remotes, and References with lazy initialization. This avoids redundant object creation and improves performance by deferring instantiation until the collection is accessed.
Copy link

Copy link
Member Author

@pvanbuijtene do you have the time to test this PR on the big repository you have and compare?

Copy link

@arturcic sure, the changes seem to have introduced some additional memory usage:

main: peak around 380MB
image

this pr: peak around 725MB
image

Copy link
Member Author

@arturcic sure, the changes seem to have introduced some additional memory usage:

main: peak around 380MB image

this pr: peak around 725MB image

btw what are you using for the memory usage analysis?

Copy link

It's the standard monitoring when running it from Rider.

Here's some more detailed information from a dotMemory snapshot taken roughly at the peak:

main:
image

pr:
image

Copy link
Member Author

It's the standard monitoring when running it from Rider.

Here's some more detailed information from a dotMemory snapshot taken roughly at the peak:

main: image

pr: image

Ok I'm also using Rider, I will have to check it out as well, thanks

Copy link

I will also do a check with the anonymized repo to see if the object counts are similar, this way there might still be a way to verify things with that repo.

arturcic reacted with heart emoji

Copy link

Here are the results with the anonymized repo, not exactly the same but it looks quite similar. So I would assume you should be able to get similar results when comparing main vs. this pr.

main:
image

pr:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /