Improve performance of currentVersion by caching #346
Conversation
|
@adamdubiel there is a failing test still but I wanted to get an opinion in advance if this approach is going the way which is acceptable for the plugin. |
|
|
||
| RevCommit currentCommit; | ||
| List<String> currentTagsList; | ||
| for (currentCommit = walk.next(); currentCommit != null; currentCommit = walk.next()) { |
There was a problem hiding this comment.
I am not quite sure if this simplification is not having some serious drawbacks
There was a problem hiding this comment.
I walker is to get the order of tags, which might not be needed for all cases. I tried to address it with latest.
JozoVilcek
force-pushed
the
improve-configuration-phase-performance
branch
from
723f7a6
to
8c1448d
Compare
JozoVilcek
force-pushed
the
improve-configuration-phase-performance
branch
from
8c1448d
to
85f5be6
Compare
| @@ -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.
Is this OK or are tags inside TagsOnCommit required to be in ordered?
|
@adamdubiel will you get some time to review this if the approach is acceptable? |
|
@adamdubiel do you have some estimate when you might get some time to review this? |
Backport of allegro#346 to 1.10.3 version.
|
@JozoVilcek Hi, thanks for your contribution! |
|
@bgalek By a brief look, I was not able to find the patch which might be related to this. Can you share the merge request URL? |
|
@JozoVilcek |
|
@bgalek, I am sorry, it took me quite a bit time to get back to you. So I did try the current master on the project I use. I executed
|
|
@JozoVilcek that's quite an improvement! I'll look at this PR in a few days! |
| public class ScmCache { | ||
|
|
||
| /** | ||
| * Since this cache is statis and Gradle Demon might keep JVM process in background for a long |
There was a problem hiding this comment.
some typos
statis -> static
Demon - Daemon
| * Since this cache is statis and Gradle Demon might keep JVM process in background for a long | ||
| * time, we have to put some TTL for cached values. | ||
| */ | ||
| private static final long INVALIDATE_AFTER_MILLIS = 1000 * 60; |
| @@ -0,0 +1,106 @@ | |||
| package pl.allegro.tech.build.axion.release.infrastructure.git; | |||
|
|
|||
| import groovy.lang.Tuple2; | |||
There was a problem hiding this comment.
i think we should not use groovy in java classes ;)
|
|
||
| private ScmCache() { } | ||
|
|
||
| private final Map<String, CachedState> cache = new HashMap<>(); |
There was a problem hiding this comment.
when a ScmCache hashmap will have more than one key?
There was a problem hiding this comment.
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?
| cache.remove(repository.id()); | ||
| } | ||
|
|
||
| public synchronized boolean checkUncommittedChanges(ScmRepository repository) { |
There was a problem hiding this comment.
wouldn't a concurrent hash map be better than a synchronized block?
There was a problem hiding this comment.
possibly. do you think it will be noticeable improvement?
| /** | ||
| * Provides cached version for some operations on {@link ScmRepository} | ||
| */ | ||
| public class ScmCache { |
There was a problem hiding this comment.
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.
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?
bgalek
force-pushed
the
master
branch
2 times, most recently
from
5bfb848
to
48ce606
Compare
No description provided.