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

Improve performance of currentVersion by caching #346

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ class DryRepository implements ScmRepository {
this.delegateRepository = delegateRepository
}

@Override
String id() {
return delegateRepository.id();
}

@Override
void fetchTags(ScmIdentity identity, String remoteName) {
log("fetching tags from remote")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ class DummyRepository implements ScmRepository {
logger.quiet("Couldn't perform $commandName command on uninitialized repository")
}

@Override
String id() {
return "DummyRepository"
}

@Override
void fetchTags(ScmIdentity identity, String remoteName) {
log('fetch tags')
Expand Down
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;
Copy link
Member

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 ;)

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 {
Copy link
Member

Choose a reason for hiding this comment

The 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 ;)

Copy link
Author

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

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

some typos
statis -> static
Demon - Daemon

* time, we have to put some TTL for cached values.
*/
private static final long INVALIDATE_AFTER_MILLIS = 1000 * 60;
Copy link
Member

Choose a reason for hiding this comment

The 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<>();
Copy link
Member

Choose a reason for hiding this comment

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

when a ScmCache hashmap will have more than one key?

Copy link
Author

Choose a reason for hiding this comment

The 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:
48e2371

Do you have suggestion for a change?


synchronized void invalidate(ScmRepository repository) {
cache.remove(repository.id());
}

public synchronized boolean checkUncommittedChanges(ScmRepository repository) {
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't a concurrent hash map be better than a synchronized block?

Copy link
Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -7,6 +7,7 @@
import pl.allegro.tech.build.axion.release.domain.scm.ScmPosition;
import pl.allegro.tech.build.axion.release.domain.scm.ScmRepository;
import pl.allegro.tech.build.axion.release.domain.scm.TaggedCommits;
import pl.allegro.tech.build.axion.release.infrastructure.git.ScmCache;

import java.util.regex.Pattern;

Expand Down Expand Up @@ -55,7 +56,7 @@ public VersionContext resolveVersion(VersionProperties versionProperties, TagPro
versions.onReleaseTag,
versions.onNextVersionTag,
versions.noTagsFound,
repository.checkUncommittedChanges()
ScmCache.getInstance().checkUncommittedChanges(repository)
);

VersionFactory.FinalVersion finalVersion = versionFactory.createFinalVersion(scmState, versions.current);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

public interface ScmRepository {

String id();

void fetchTags(ScmIdentity identity, String remoteName);

void tag(String tagName);
Expand Down
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;
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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 + ".");
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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()) {
Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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;
}

Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Author

Choose a reason for hiding this comment

The 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"() {
Expand Down