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.

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

gitbase: use only one cache for all repositories #898

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

jfontan
Copy link
Contributor

@jfontan jfontan commented Jun 21, 2019

Also fix a bug getting the cache size from cli.

Before integrating the use of go-borges repositories were managed by RepositoryPool. When it was created it initialized a git object cache that is used by all the repositories. This cache is also used when using indexes and reading directly from packfiles (cache is retrieved with Repository.Cache(), https://github.com/src-d/gitbase/blob/master/packfiles.go#L239).

Now go-borges takes care of repository libraries and retrieving go-git repositories from them. To make it work the same we initialize go-borges library with a cache generated in gitbase (command.Server) but I forgot that RepositoryPool was also generating a cache internally and returning it when calling Repository.Cache(). This means that there was two different objects caches, one for repositories opened without indexes and other for reading packfiles when the index was used. The problems are:

  • It could theoretically use twice the amount of memory
  • The objects in one of the cache won't be seen by the other so they will be read twice from disk

The change makes RepositoryPool get an initialized cache that will be used by all repos (both with and without index).

  • I updated the documentation explaining the new behavior if any.
  • I updated CHANGELOG.md file adding the new feature or bug fix.
  • I updated go-mysql-server using make upgrade command if applicable.
  • I added or updated examples if applicable.
  • I checked that changes on schema are reflected into the documentation, if applicable.

@jfontan jfontan requested a review from a team June 21, 2019 12:10
@juanjux
Copy link
Contributor

juanjux commented Jun 21, 2019

One question (not doubting, just want to understand better): what's the rationale for this change?

@jfontan
Copy link
Contributor Author

jfontan commented Jun 21, 2019

One question (not doubting, just want to understand better): what's the rationale for this change?

@juanjux You are right, there was almost no description to the PR 🙈. I've updated the description with the problem it's fixing.

@juanjux
Copy link
Contributor

juanjux commented Jun 21, 2019

@jfontan awesome description, thanks!

Also fix a bug getting the cache size from cli.

Signed-off-by: Javi Fontan <jfontan@gmail.com>
@jfontan
Copy link
Contributor Author

jfontan commented Jun 21, 2019

I've just rebased with latest master.

@ajnavarro ajnavarro merged commit 5aba59c into src-d:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants