add caching to git describe #684

yiweny · 2023-01-30T22:55:51Z

Before this PR

gitVersion() or versionDetails() does not have caching, every time it is called, it will create a new 'VersionDetails' object and call the expensive git describe operation to get the version. This causes long configuration time in a lot of the repos where version gitVersion() is called under allprojects.

After this PR

Build service is used so if we call gitVersion() on all projects, we will not call the expensive git describe many times. Instead, the build service will have a concurrent hash map that stores the created VersionDetails object with a specific prefix as key the first time it is called. If gitVersion() is previously computed, it will return the cached value the next time it is called.

Possible downsides?

Build service is only supported from Gradle 6. This means this plugin will no longer support Gradle 5.

changelog-app · 2023-01-30T22:55:54Z

Generate changelog in `changelog/@unreleased`

Type

Description

Caching is added to gitVersion() or versionDetails() to avoid long configuration time caused by repeatedly calling git describe on subprojects.
The implementation uses BuildService which is introduced in Gradle 6.1. Therefore, any version before Gradle 6.1 won't be supported.

Check the box to generate changelog(s)

Generate changelog entry

src/test/groovy/com/palantir/gradle/gitversion/GitVersionPluginTests.groovy

CRogers · 2023-02-07T12:45:58Z

src/main/java/com/palantir/gradle/gitversion/GitVersionPlugin.java

+                .getSharedServices()
+                .registerIfAbsent("GitVersionCacheService", GitVersionCacheService.class, spec -> {
+                    // Provide some parameters
+                    spec.getMaxParallelUsages().set(maxParallelUsage);


Do we need to set a max parallel usages at all? I think provided we make the class thread safe, it should be ok to allow max parallel access.

I removed this line.

CRogers · 2023-02-07T12:47:41Z

src/main/java/com/palantir/gradle/gitversion/GitVersionPlugin.java


-        final Git git = gitRepo(project);
+        if (project.getRootProject() == project) {


I think it'd be best to leave this in the root plugin, if possible. Just to separate out the code a bit and avoid this if.

I added back the GitVersionRootPlugin. The only reason why I deleted the root plugin is that we'd have to have the following in both plugins

Provider<GitVersionCacheService> serviceProvider = project.getGradle() .getSharedServices() .registerIfAbsent("GitVersionCacheService", GitVersionCacheService.class, _spec -> {});

CRogers · 2023-02-07T12:52:20Z

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java

+        Git git = gitRepo(gitDir);
+        VersionDetails versionDetails =
+                TimingVersionDetails.wrap(timer, new VersionDetailsImpl(git, GitVersionArgs.fromGroovyClosure(args)));
+        versionDetailsMap.put(key, versionDetails);


This is not thread safe. I'd make the map a ConcurrentHashMap and use .computeIfAbsent:

versionDetailsMap.computeIfAbsent(key, _ignored -> { // calculate version details for key });

We already have guava as a dependency here - perhaps just a LoadingCache?

A cache that expires is unnecessary complexity for this use case imo, the number of items in this map should never grow too large, and evicting entries is probably not what we want.

Ah well, the checkstyle config bans guava caches in favor of Caffeine anyway :D

I resolved by using the concurrent hash map instead of hash map here.

CRogers · 2023-02-07T12:53:10Z

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java

+    public final String getGitVersion(String project, Object args) {
+        File gitDir = getRootGitDir(new File(project));
+        GitVersionArgs gitVersionArgs = GitVersionArgs.fromGroovyClosure(args);
+        String key = gitDir.toString() + "|" + gitVersionArgs.getPrefix();


Why does git dir need to be part of the key?

Because I am thinking of the scenario where different projects have different work tree. I can remove this if it is considered an extreme edge case not likely to happen.

^^ I removed the gitDir part in the key. If you think the above case might happen and it is necessary to keep the worktree as part of the key, please let me know.

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java

CRogers · 2023-02-07T13:17:46Z

changelog/@unreleased/pr-684.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: add caching to git describe


We need a better description here. I'd make it a break and say it removes support for Gradle 5 (and say what is the new minimum version we support). I'd also a bit more detail, saying how the pattern of calling gitversion() in each project used to lead to extremely long configuration times, however these are now cached so it's faster.

bjlaub · 2023-02-07T15:20:29Z

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java

+        if (versionDetailsMap.containsKey(key)) {
+            return versionDetailsMap.get(key).getVersion();
+        }
+        Git git = gitRepo(gitDir);


I wonder if this could just be pushed down into VersionDetailsImpl? For a couple of reasons:

It seems odd to initialize a JGit object in this class just to inject it into the VersionDetailsImpl instance

If we ever do decide to eliminate JGit, it would be good to have the usage limited in scope to make refactoring easier

On the other hand, I wonder if the Git class is thread-safe? The JGit javadoc doesn't really specify and a quick search indicates... maybe? If it is, then I wonder if it would make sense to cache a single instance here in this class instead of creating one instance per VersionDetailsImpl.

My preference would probably be to move this inside VersionDetailsImpl and instead pass gitDir as a constructor arg; a quick glance at VersionDetailsImpl seems to indicate that it is using both JGit and Native Git to compute the description, which seems a little broken (see e.g.: https://github.com/palantir/gradle-git-version/blob/develop/src/main/java/com/palantir/gradle/gitversion/VersionDetailsImpl.java#L54-L60)

Fixed by passing gitDir to the constructor rather than Git object. @bjlaub Do you want me to delete the Native git part as well?

CRogers · 2023-02-08T12:23:47Z

src/main/java/com/palantir/gradle/gitversion/GitVersionPlugin.java

-        final Git git = gitRepo(project);
+        Provider<GitVersionCacheService> serviceProvider = project.getGradle()
+                .getSharedServices()
+                .registerIfAbsent("GitVersionCacheService", GitVersionCacheService.class, _spec -> {});


I would make a static method to get the Provider<GitVersionCacheService> from a Project which is shared by both plugins.

You could even put it on GitVersionCacheService

CRogers · 2023-02-08T12:42:40Z

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java

+    private static final Logger log = LoggerFactory.getLogger(GitVersionCacheService.class);
+
+    private final Timer timer = new Timer();
+    private final Map<GitVersionArgs, VersionDetails> versionDetailsMap = new ConcurrentHashMap<>();


I would make this a ConcurrentMap to make it obvious it's the concurrent version.

CRogers · 2023-02-08T12:45:32Z

changelog/@unreleased/pr-684.v2.yml

+    called under allprojects. \n\n## After this PR\nBuild service is used so if we
+    call `gitVersion()` on all projects, we will not call the expensive `git describe`
+    many times. Instead, the build service will have a concurrent hash map that stores
+    the created `VersionDetails` object with a specific prefix as key the first time


I think this sentence is too much detail for an end user - I'd leave out any details of how the caching happens (build service, hashmaps, prefixes etc) and just say it's now cached.

CRogers · 2023-02-08T12:46:20Z

changelog/@unreleased/pr-684.v2.yml

@@ -0,0 +1,16 @@
+type: break
+break:
+  description: "## Before this PR\n`gitVersion()` or `versionDetails()` does not have


I would also leave out the "Before this PR" bits and just have a couple of bullet points, one for the change and one describing the break.

bjlaub · 2023-02-08T18:51:39Z

src/main/java/com/palantir/gradle/gitversion/NativeGitDescribe.java

@@ -1,126 +0,0 @@
-/*


out of curiosity, what's the reason for removing this? If there's a potential future where we remove jgit, don't we want to keep this code around?

Yeah makes sense. I deleted because it won't be used if we do not call Native git.

because it won't be used if we do not call Native git.

not sure i follow - don't we always try native git first, and then fall back to jgit if it fails?

CRogers · 2023-02-09T17:25:51Z

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java

+    private static final Logger log = LoggerFactory.getLogger(GitVersionCacheService.class);
+
+    private final Timer timer = new Timer();
+    private final ConcurrentMap<GitVersionArgs, VersionDetails> versionDetailsMap = new ConcurrentHashMap<>();


I tried this out on a project and was wondering why it doesn't appear to produce a speedup on repos with large numbers of projects and calling gitversion() on each. I think this is the reason - GitVersionArgs does not implement hashCode() or equals() so a new entry is created on the map each time. I'd either just use String here (probably easiest), or make a new immutables type containing the string prefix and immutables will generate hashCode() and equals() for us.

I switched back to using the concatenation of gitDir path and prefix as the key.

If we do not use the gitDir as part of the key, we'd fail the test subproject version test where the two projects have different worktree.

CRogers

Looks good!

yiweny force-pushed the yyuan/add-build-service branch from 2a9b159 to 621fef4 Compare January 30, 2023 22:57

yiweny changed the title ~~add caching to git describe~~ [WIP] add caching to git describe Jan 30, 2023

yiweny added the do not merge label Jan 30, 2023

yiweny closed this Feb 3, 2023

yiweny reopened this Feb 6, 2023

Yiwen Yuan and others added 5 commits February 6, 2023 13:09

add caching to git describe

d1d0f1e

fix the service

86f273d

fix test case

9c7c561

make the constructor not empty

14c0c85

add caching

f8f5576

yiweny force-pushed the yyuan/add-build-service branch from ab3dc73 to f8f5576 Compare February 6, 2023 21:09

yiweny changed the base branch from develop to yyuan/remove-dep-to-gradle5 February 6, 2023 21:11

add build service

9d18544

yiweny removed the do not merge label Feb 6, 2023

add tests

59a03bd

yiweny changed the title ~~[WIP] add caching to git describe~~ add caching to git describe Feb 7, 2023

yiweny commented Feb 7, 2023

View reviewed changes

src/test/groovy/com/palantir/gradle/gitversion/GitVersionPluginTests.groovy Outdated Show resolved Hide resolved

svc-changelog added 2 commits February 7, 2023 00:38

Add generated changelog entries

d6507e2

Add generated changelog entries

0d34cae

yiweny marked this pull request as ready for review February 7, 2023 00:42

yiweny requested review from CRogers and bjlaub February 7, 2023 00:42

fix some style

2ab9172

yiweny force-pushed the yyuan/add-build-service branch from c4b8dcc to 2ab9172 Compare February 7, 2023 00:51

CRogers reviewed Feb 7, 2023

View reviewed changes

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java Outdated Show resolved Hide resolved

CRogers reviewed Feb 7, 2023

View reviewed changes

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java Outdated Show resolved Hide resolved

CRogers reviewed Feb 7, 2023

View reviewed changes

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java Outdated Show resolved Hide resolved

CRogers reviewed Feb 7, 2023

View reviewed changes

src/main/java/com/palantir/gradle/gitversion/GitVersionCacheService.java Outdated Show resolved Hide resolved

CRogers reviewed Feb 7, 2023

View reviewed changes

bjlaub reviewed Feb 7, 2023

View reviewed changes

add back GitVersionRootPlugin

05d9f3a

yiweny force-pushed the yyuan/remove-dep-to-gradle5 branch from 8d686b5 to 0aa0914 Compare February 7, 2023 19:53

fix code based on review and remove test cases of extreme edge cases

a56c821

yiweny force-pushed the yyuan/add-build-service branch from 6923741 to a56c821 Compare February 8, 2023 01:39

CRogers reviewed Feb 8, 2023

View reviewed changes

fix code based on review comments

ce24731

yiweny force-pushed the yyuan/add-build-service branch from 8bb771f to ce24731 Compare February 8, 2023 18:44

bjlaub reviewed Feb 8, 2023

View reviewed changes

svc-changelog added 3 commits February 8, 2023 10:53

Add generated changelog entries

176e185

Add generated changelog entries

6399f43

Add generated changelog entries

7b734e5

yiweny force-pushed the yyuan/add-build-service branch 2 times, most recently from 71d0fe6 to 7b734e5 Compare February 8, 2023 19:00

CRogers reviewed Feb 9, 2023

View reviewed changes

fix map key

8915a4c

yiweny force-pushed the yyuan/add-build-service branch from 94ba4c8 to 8915a4c Compare February 9, 2023 18:56

CRogers approved these changes Feb 9, 2023

View reviewed changes

yiweny merged commit ce40722 into yyuan/remove-dep-to-gradle5 Feb 9, 2023

pkoenig10 mentioned this pull request May 1, 2024

Cache results of git commands #799

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add caching to git describe #684

add caching to git describe #684

yiweny commented Jan 30, 2023 •

edited

Loading

changelog-app bot commented Jan 30, 2023 •

edited by yiweny

Loading

CRogers Feb 7, 2023

yiweny Feb 8, 2023

CRogers Feb 7, 2023

yiweny Feb 8, 2023

CRogers Feb 7, 2023 •

edited

Loading

bjlaub Feb 7, 2023

CRogers Feb 7, 2023

bjlaub Feb 7, 2023

yiweny Feb 8, 2023

CRogers Feb 7, 2023

yiweny Feb 7, 2023

yiweny Feb 8, 2023

CRogers Feb 7, 2023

bjlaub Feb 7, 2023

yiweny Feb 8, 2023

CRogers Feb 8, 2023

CRogers Feb 8, 2023

CRogers Feb 8, 2023

CRogers Feb 8, 2023 •

edited

Loading

CRogers Feb 8, 2023

bjlaub Feb 8, 2023

yiweny Feb 8, 2023

bjlaub Feb 8, 2023

CRogers Feb 9, 2023

yiweny Feb 9, 2023

yiweny Feb 9, 2023

CRogers left a comment


		final Git git = gitRepo(project);
		if (project.getRootProject() == project) {

add caching to git describe #684

add caching to git describe #684

Conversation

yiweny commented Jan 30, 2023 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Jan 30, 2023 • edited by yiweny Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRogers Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRogers Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRogers left a comment

Choose a reason for hiding this comment

yiweny commented Jan 30, 2023 •

edited

Loading

changelog-app bot commented Jan 30, 2023 •

edited by yiweny

Loading

Generate changelog in `changelog/@unreleased`

CRogers Feb 7, 2023 •

edited

Loading

CRogers Feb 8, 2023 •

edited

Loading