-
Notifications
You must be signed in to change notification settings - Fork 157
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
Improve performance of currentVersion by caching #346
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
package pl.allegro.tech.build.axion.release.infrastructure.git; | ||
|
||
import groovy.lang.Tuple2; | ||
import org.eclipse.jgit.api.Git; | ||
import org.eclipse.jgit.api.errors.GitAPIException; | ||
import org.eclipse.jgit.lib.Ref; | ||
import org.eclipse.jgit.revwalk.RevCommit; | ||
import org.eclipse.jgit.revwalk.RevWalk; | ||
import pl.allegro.tech.build.axion.release.domain.scm.ScmException; | ||
import pl.allegro.tech.build.axion.release.domain.scm.ScmRepository; | ||
|
||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
/** | ||
* Provides cached version for some operations on {@link ScmRepository} | ||
*/ | ||
public class ScmCache { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you think about enum based singletons? also it would be nice if this class would be package-private ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can not be easily package private right now as it is accessed from VersionResolver. Do you have suggestion how to move necessary classes so ti encapsulates better? |
||
|
||
/** | ||
* Since this cache is statis and Gradle Demon might keep JVM process in background for a long | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some typos |
||
* time, we have to put some TTL for cached values. | ||
*/ | ||
private static final long INVALIDATE_AFTER_MILLIS = 1000 * 60; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use Duration ;) |
||
|
||
private static final ScmCache CACHE = new ScmCache(); | ||
|
||
public static ScmCache getInstance() { | ||
return CACHE; | ||
} | ||
|
||
private ScmCache() { } | ||
|
||
private final Map<String, CachedState> cache = new HashMap<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when a ScmCache hashmap will have more than one key? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if it can happen somehow that this singleton will get involved in mixed/multiple projects. This is coming from the inception work I picked up and continued: Do you have suggestion for a change? |
||
|
||
synchronized void invalidate(ScmRepository repository) { | ||
cache.remove(repository.id()); | ||
} | ||
|
||
public synchronized boolean checkUncommittedChanges(ScmRepository repository) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't a concurrent hash map be better than a synchronized block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. possibly. do you think it will be noticeable improvement? |
||
CachedState state = retrieveCachedStateFor(repository); | ||
if (state.hasUncommittedChanges == null) { | ||
state.hasUncommittedChanges = repository.checkUncommittedChanges(); | ||
} | ||
return state.hasUncommittedChanges; | ||
} | ||
|
||
synchronized List<Tuple2<Ref, RevCommit>> parsedTagList(ScmRepository repository, Git git, RevWalk walker) throws GitAPIException { | ||
CachedState state = retrieveCachedStateFor(repository); | ||
if (state.tags == null) { | ||
List<Tuple2<Ref, RevCommit>> list = new ArrayList<>(); | ||
for (Ref tag : git.tagList().call()) { | ||
try { | ||
list.add(new Tuple2<>(tag, walker.parseCommit(tag.getObjectId()))); | ||
} catch (IOException e) { | ||
throw new ScmException(e); | ||
} | ||
} | ||
state.tags = list; | ||
} | ||
return state.tags; | ||
} | ||
|
||
private CachedState retrieveCachedStateFor(ScmRepository scmRepository) { | ||
String key = scmRepository.id(); | ||
String currentHeadRevision = scmRepository.currentPosition().getRevision(); | ||
long currentTime = System.currentTimeMillis(); | ||
CachedState state = cache.get(key); | ||
if (state == null) { | ||
state = new CachedState(currentHeadRevision); | ||
cache.put(key, state); | ||
} else { | ||
if (!currentHeadRevision.equals(state.headRevision) || (state.createTimestamp + INVALIDATE_AFTER_MILLIS) < currentTime) { | ||
state = new CachedState(currentHeadRevision); | ||
cache.put(key, state); | ||
} | ||
} | ||
return state; | ||
} | ||
|
||
/** | ||
* Helper object holding cached values per SCM repository | ||
*/ | ||
private static class CachedState { | ||
|
||
final long createTimestamp; | ||
|
||
/** | ||
* HEAD revision of repo for which this cache was created. Cache has to be invalidated | ||
* if HEAD changes. | ||
*/ | ||
final String headRevision; | ||
|
||
Boolean hasUncommittedChanges; | ||
|
||
List<Tuple2<Ref, RevCommit>> tags; | ||
|
||
CachedState(String headRevision) { | ||
createTimestamp = System.currentTimeMillis(); | ||
this.headRevision = headRevision; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package pl.allegro.tech.build.axion.release.infrastructure.git; | ||
|
||
import groovy.lang.Tuple2; | ||
import org.eclipse.jgit.api.*; | ||
import org.eclipse.jgit.api.errors.GitAPIException; | ||
import org.eclipse.jgit.api.errors.NoHeadException; | ||
|
@@ -9,13 +10,15 @@ | |
import org.eclipse.jgit.revwalk.RevSort; | ||
import org.eclipse.jgit.revwalk.RevWalk; | ||
import org.eclipse.jgit.transport.*; | ||
import org.eclipse.jgit.util.FS; | ||
import pl.allegro.tech.build.axion.release.domain.logging.ReleaseLogger; | ||
import pl.allegro.tech.build.axion.release.domain.scm.*; | ||
|
||
import java.io.File; | ||
import java.io.IOException; | ||
import java.net.URISyntaxException; | ||
import java.util.*; | ||
import java.util.function.Supplier; | ||
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.StreamSupport; | ||
|
@@ -33,7 +36,8 @@ public class GitRepository implements ScmRepository { | |
public GitRepository(ScmProperties properties) { | ||
try { | ||
this.repositoryDir = properties.getDirectory(); | ||
this.jgitRepository = Git.open(repositoryDir); | ||
RepositoryCache.FileKey key = RepositoryCache.FileKey.lenient(repositoryDir, FS.DETECTED); | ||
this.jgitRepository = Git.wrap(RepositoryCache.open(key, true)); | ||
this.properties = properties; | ||
} catch (RepositoryNotFoundException exception) { | ||
throw new ScmRepositoryUnavailableException(exception); | ||
|
@@ -51,6 +55,11 @@ public GitRepository(ScmProperties properties) { | |
|
||
} | ||
|
||
@Override | ||
public String id() { | ||
return repositoryDir.getAbsolutePath(); | ||
} | ||
|
||
/** | ||
* This fetch method behaves like git fetch, meaning it only fetches thing without merging. | ||
* As a result, any fetched tags will not be visible via GitRepository tag listing methods | ||
|
@@ -93,6 +102,7 @@ public void tag(final String tagName) { | |
|
||
if (!isOnExistingTag) { | ||
jgitRepository.tag().setName(tagName).call(); | ||
ScmCache.getInstance().invalidate(this); | ||
} else { | ||
logger.debug("The head commit " + headId + " already has the tag " + tagName + "."); | ||
} | ||
|
@@ -109,6 +119,7 @@ private ObjectId head() throws IOException { | |
public void dropTag(String tagName) { | ||
try { | ||
jgitRepository.tagDelete().setTags(GIT_TAG_PREFIX + tagName).call(); | ||
ScmCache.getInstance().invalidate(this); | ||
} catch (GitAPIException e) { | ||
throw new ScmException(e); | ||
} | ||
|
@@ -139,7 +150,9 @@ public ScmPushResult push(ScmIdentity identity, ScmPushOptions pushOptions, bool | |
|
||
private Iterable<PushResult> callPush(PushCommand pushCommand) { | ||
try { | ||
return pushCommand.call(); | ||
Iterable<PushResult> result = pushCommand.call(); | ||
ScmCache.getInstance().invalidate(this); | ||
return result; | ||
} catch (GitAPIException e) { | ||
throw new ScmException(e); | ||
} | ||
|
@@ -204,6 +217,7 @@ public void commit(List<String> patterns, String message) { | |
} | ||
|
||
jgitRepository.commit().setMessage(message).call(); | ||
ScmCache.getInstance().invalidate(this); | ||
} catch (GitAPIException | IOException e) { | ||
throw new ScmException(e); | ||
} | ||
|
@@ -331,39 +345,36 @@ private List<TagsOnCommit> taggedCommitsInternal(Pattern pattern, String maybeSi | |
startingCommit = headId; | ||
} | ||
|
||
|
||
RevWalk walk = walker(startingCommit); | ||
if (!inclusive) { | ||
walk.next(); | ||
} | ||
|
||
Map<String, List<String>> allTags = tagsMatching(pattern, walk); | ||
|
||
RevCommit currentCommit; | ||
List<String> currentTagsList; | ||
for (currentCommit = walk.next(); currentCommit != null; currentCommit = walk.next()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not quite sure if this simplification is not having some serious drawbacks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I walker is to get the order of tags, which might not be needed for all cases. I tried to address it with latest. |
||
currentTagsList = allTags.get(currentCommit.getId().getName()); | ||
|
||
if (currentTagsList != null) { | ||
TagsOnCommit taggedCommit = new TagsOnCommit( | ||
currentCommit.getId().name(), | ||
currentTagsList | ||
); | ||
taggedCommits.add(taggedCommit); | ||
|
||
if (stopOnFirstTag) { | ||
break; | ||
try { | ||
if (!inclusive) { | ||
walk.next(); | ||
} | ||
Map<String, List<String>> allTags = tagsMatching(pattern, walk); | ||
if (stopOnFirstTag) { | ||
// stopOnFirstTag needs to get latest tag, therefore the order does matter | ||
// order is given by walking the repository commits. this can be slower in some | ||
// situations than returning all tagged commits | ||
RevCommit currentCommit; | ||
for (currentCommit = walk.next(); currentCommit != null; currentCommit = walk.next()) { | ||
List<String> tagList = allTags.get(currentCommit.getId().getName()); | ||
if (tagList != null) { | ||
TagsOnCommit taggedCommit = new TagsOnCommit(currentCommit.getId().name(), tagList); | ||
taggedCommits.add(taggedCommit); | ||
break; | ||
} | ||
} | ||
|
||
} else { | ||
// order not needed, we can just return all tagged commits | ||
allTags.forEach((key, value) -> taggedCommits.add(new TagsOnCommit(key, value))); | ||
} | ||
|
||
} finally { | ||
walk.dispose(); | ||
} | ||
|
||
walk.dispose(); | ||
} catch (IOException | GitAPIException e) { | ||
throw new ScmException(e); | ||
} | ||
|
||
return taggedCommits; | ||
} | ||
|
||
|
@@ -380,27 +391,17 @@ private RevWalk walker(ObjectId startingCommit) throws IOException { | |
} | ||
|
||
private Map<String, List<String>> tagsMatching(Pattern pattern, RevWalk walk) throws GitAPIException { | ||
List<Ref> tags = jgitRepository.tagList().call(); | ||
List<Tuple2<Ref, RevCommit>> parsedTagList = ScmCache.getInstance().parsedTagList(this, jgitRepository, walk); | ||
|
||
return tags.stream() | ||
.map(tag -> new TagNameAndId( | ||
tag.getName().substring(GIT_TAG_PREFIX.length()), | ||
parseCommitSafe(walk, tag.getObjectId()) | ||
)) | ||
return parsedTagList.stream() | ||
.map(pair -> new TagNameAndId( | ||
pair.getFirst().getName().substring(GIT_TAG_PREFIX.length()), | ||
pair.getSecond().getName())) | ||
.filter(t -> pattern.matcher(t.name).matches()) | ||
.collect( | ||
HashMap::new, | ||
(m, t) -> m.computeIfAbsent(t.id, (s) -> new ArrayList<>()).add(t.name), | ||
HashMap::putAll | ||
); | ||
} | ||
|
||
private String parseCommitSafe(RevWalk walk, AnyObjectId commitId) { | ||
try { | ||
return walk.parseCommit(commitId).getName(); | ||
} catch (IOException e) { | ||
throw new ScmException(e); | ||
} | ||
HashMap::putAll); | ||
} | ||
|
||
private final static class TagNameAndId { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,7 +169,7 @@ class GitRepositoryTest extends Specification { | |
List<TagsOnCommit> allTaggedCommits = repository.taggedCommits(~/^release.*/) | ||
|
||
then: | ||
allTaggedCommits.collect { c -> c.tags[0] } == ['release-3', 'release-4', 'release-2', 'release-1'] | ||
allTaggedCommits.collect { c -> c.tags[0] }.toSet() == ['release-3', 'release-4', 'release-2', 'release-1'].toSet() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this OK or are tags inside TagsOnCommit required to be in ordered? |
||
} | ||
|
||
def "should return only tags that match with prefix"() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should not use groovy in java classes ;)