Skip to content

Conversation

@arturcic
Copy link
Member

@arturcic arturcic commented Oct 8, 2025

Follow up for #4681

@HHobeck
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).

@arturcic
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

@pvanbuijtene
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.

@arturcic
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 3 times, most recently from 468ac8d to ac0e28b Compare October 15, 2025 15:52
@arturcic
Copy link
Member Author

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

@pvanbuijtene
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

@arturcic
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?

@pvanbuijtene
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

@arturcic
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

@pvanbuijtene
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.

@pvanbuijtene
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

@arturcic arturcic force-pushed the fix/cache-git-objects branch 8 times, most recently from 30f83ff to d5b92bf Compare October 27, 2025 08:23
@arturcic
Copy link
Member Author

@pvanbuijtene mind to have a new run on memory usage for main and PR branch against your original big repo?

@arturcic arturcic force-pushed the fix/cache-git-objects branch from d5b92bf to bfcf3ec Compare October 27, 2025 11:00
@arturcic arturcic marked this pull request as ready for review October 27, 2025 11:27
Copilot AI review requested due to automatic review settings October 27, 2025 11:27
@arturcic arturcic force-pushed the fix/cache-git-objects branch from bfcf3ec to d6b6483 Compare October 27, 2025 11:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the git object caching mechanism by introducing a dedicated GitRepositoryCache class to centralize object wrapping and caching. The changes improve code organization and maintainability by extracting caching logic from the GitRepository class into a specialized cache component.

Key Changes:

  • Introduces a new GitRepositoryCache class to centralize caching of git objects (branches, commits, tags, remotes, references, refspecs)
  • Removes the IGitObject interface and moves its properties directly to ICommit
  • Renames Refs property to References throughout the codebase for consistency
  • Updates collection classes to use the new caching mechanism instead of creating objects directly

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
GitRepositoryCache.cs New cache class implementing centralized caching logic for git objects
GitRepository.cs Refactored to use GitRepositoryCache and lazy initialization for collections
Commit.cs No longer inherits from GitObject; implements ICommit directly with Id/Sha properties
IGitObject.cs Removed interface; properties moved to ICommit
ICommit.cs Now contains Id and Sha properties directly instead of inheriting from IGitObject
IGitRepository.cs Renamed Refs property to References
Branch.cs, BranchCollection.cs Updated to use GitRepositoryCache instead of GitRepository
Tag.cs, TagCollection.cs Updated to use GitRepositoryCache instead of GitRepository
CommitCollection.cs Updated to use GitRepositoryCache instead of GitRepository
Remote.cs, RemoteCollection.cs Updated to use GitRepositoryCache instead of GitRepository
Reference.cs, ReferenceCollection.cs Updated to use GitRepositoryCache instead of GitRepository
RefSpec.cs, RefSpecCollection.cs Marked RefSpec as sealed; updated to use GitRepositoryCache
RepositoryStore.cs Updated method signature to use ICommit instead of IGitObject
GitPreparer.cs Updated all Refs references to References
PublicAPI files Updated shipped/unshipped API surfaces

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@arturcic arturcic force-pushed the fix/cache-git-objects branch from d6b6483 to 9eafb6e Compare October 27, 2025 14:09
@arturcic arturcic added this to the 6.x milestone Oct 27, 2025
Introduces caching mechanisms for remotes, references, and ref specs in `GitRepository`. Refactors related collections to utilize the new cache for improved efficiency and consistency.
Replaces caching logic within `GitRepository` with a new dedicated `GitRepositoryCache` class. Updates related classes and collections to utilize the cache through `GetOrWrap` methods, improving maintainability and reducing duplication.
@arturcic arturcic force-pushed the fix/cache-git-objects branch from 9eafb6e to 681782b Compare October 27, 2025 14:53
@sonarqubecloud
Copy link

@arturcic arturcic requested review from Copilot and removed request for Copilot October 27, 2025 15:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@arturcic arturcic requested review from HHobeck and asbjornu October 27, 2025 15:14
@arturcic
Copy link
Member Author

arturcic commented Oct 27, 2025

@pvanbuijtene mind to have a review as well?, please also attach the new memory consumption data you see on your big repo.

@pvanbuijtene
Copy link

With the restricted knowledge I have of this repository... looks good, ran it twice and the memory usage of the pr branch is slightly lower.

main:
image

pr:
image

@arturcic
Copy link
Member Author

With the restricted knowledge I have of this repository... looks good, ran it twice and the memory usage of the pr branch is slightly lower.

main: image

pr: image

thanks @pvanbuijtene. I think I will merge this one then. Thanks once again for the initial work you've done.

@arturcic arturcic merged commit 232048e into GitTools:main Oct 27, 2025
85 checks passed
@arturcic arturcic deleted the fix/cache-git-objects branch October 27, 2025 20:09
@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2025

Thank you @arturcic for your contribution!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants