Skip to content

Caching to improve performance - #1100

Merged
JakeGinnivan merged 8 commits into
GitTools:masterfrom
DanielRose:cache-merge-base
Nov 28, 2016
Merged

JakeGinnivan merged 8 commits into
GitTools:masterfrom
DanielRose:cache-merge-base

Conversation

@DanielRose

Copy link
Copy Markdown
Contributor

When I use GitVersion on my repository (about 30k commits and 14 branches), it gets very slow. The main reason is that it calculates lots of merge bases and their commits. However, this data is ephemeral, so it gets recalculated multiple times.

Here I add two caches to improve this. The first commit caches the result of FindMergeBase(). The second caches the internal list of FindCommitBranchWasBranchedFrom().

Improvement in run times (on my machine):

Time
No Caching 4m 51s
Caching of FindMergeBase() 1m 34s
Both caches 30s

This builds upon my other PR (#1085), since I use some of the refactorings I made as part of it.

@asbjornu

Copy link
Copy Markdown
Member

@DanielRose: Thanks for this! If I understand this correctly, the caching you're talking about is an in-memory dictionary and has no relation to the disk-based cache stored in .git/gitversion_cache.yml (see GitVersionCache)?

@DanielRose

DanielRose commented Nov 25, 2016 •

Copy link
Copy Markdown
Contributor Author

@asbjornu Yes, you are right. Note that this includes the changes of my other PR, so either that one needs to be approved as well or I would need to pull them apart.

A second step would be to investigate ways of storing/invalidating this data, since the disk-based cache is currently completely bypassed when anything has changed (such as adding even a single commit).

@asbjornu

Copy link
Copy Markdown
Member

@DanielRose: Now that @JakeGinnivan has done the herculean effort of getting everything from GitTools.Core to GitVersion builds green again, can you please rebase this to see if it turns green as well?

@JakeGinnivan

Copy link
Copy Markdown
Contributor

@DanielRose can you rebase this?

@JakeGinnivan

Copy link
Copy Markdown
Contributor

I merged your first PR :)

@DanielRose

Copy link
Copy Markdown
Contributor Author

@JakeGinnivan @asbjornu I noticed; was in the process of rebasing. Thanks!

@DanielRose

DanielRose commented Nov 26, 2016 •

Copy link
Copy Markdown
Contributor Author

Some tests are causing a stack overflow. I'll check this in more detail this evening.

@DanielRose

Copy link
Copy Markdown
Contributor Author

@JakeGinnivan @asbjornu Fixed it. The stack overflow was due to a possible infinite loop when having a branch inherit its configuration, but since it cannot find a parent branch, it ends up choosing itself as parent. Since the config is "inherit", it ends up finding itself again, etc.

Now I issue a warning and use "Patch" instead.

I also fixed the rest of the tests. In the unit tests, GitVersion runs multiple times, so I invalidate the caches at every run.

@asbjornu

Copy link
Copy Markdown
Member

@DanielRose: Excellent. Just to be sure, is this meant for a v4 or v3.6 target? master currently represents v4, so if you want this merged into both, you would need to create a new PR with these commits rebased on the support/3.6 branch.

@DanielRose

Copy link
Copy Markdown
Contributor Author

@asbjornu I noticed the extreme slowness in v4 (due to the new develop version strategy), but it would also make sense on 3.6. I can cherry-pick the relevant parts for 3.6 as well, if you wish :)

@JakeGinnivan

Copy link
Copy Markdown
Contributor

A lot of this has been re-written in v4. You would have to re-write for v3 and I don't know it's worth it.

@JakeGinnivan

Copy link
Copy Markdown
Contributor

@DanielRose when you are online can you ping me on gitter.

DanielRose and others added 8 commits November 28, 2016 10:44
Pass on data, check that each branch is added only once.
Clear the caches for each new run of GitVersion.
…ository

This makes the cache non-static and hopefully easier to maintain in the long wrong
Additional refactoring for caching.
Fix typos, remove unused usings, invoke extension methods properly.
@JakeGinnivan
JakeGinnivan merged commit d4801d7 into GitTools:master Nov 28, 2016
@DanielRose
DanielRose deleted the cache-merge-base branch November 28, 2016 11:43

@xproax xproax left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

4 participants