Fix performance issue with GitRepository.history & startup issues (#475)

Motivation:

- In a certain case, jGit's `RevWalk` can load the entire repository
  history, even including those unused, into memory, which may take a
  lot of time for a repository with a large number of commits.
- Central Dogma server can delete the PID file it did not create.
- Central Dogma server's default PID file path is usually unwritable by
  a non-super user.

Modifications:

- Set `false` to `RevWalk.rewriteParents` so that the excessive amount
  of commits are loaded into memory.
- Do not delete the PID file if not created by the current process.
- Change the default PID file path to less restrictive one.

Result:

- Fixes #472
- Central Dogma server does not delete the existing server's PID file
  anymore.
- Central Dogma server can be launched without an explicit `-pidfile`
  option even if a user is not a super user.

Author: trustin

server/src/main/java/com/linecorp/centraldogma/server/Main.java

@@ -15,13 +15,14 @@
  */
 package com.linecorp.centraldogma.server;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
+
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.AtomicMoveNotSupportedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
-import java.util.Optional;
 
 import javax.annotation.Nullable;
 
@@ -57,14 +58,18 @@ enum State {
                      File.separatorChar + "conf" +
                      File.separatorChar + "dogma.json");
 
+    private static final File DEFAULT_PID_FILE =
+            new File(System.getProperty("user.dir", ".") +
+                     File.separatorChar + "dogma.pid");
+
     @Nullable
     @Parameter(names = "-config", description = "The path to the config file", converter = FileConverter.class)
     private File configFile;
 
     @Nullable
     @Parameter(names = "-pidfile",
             description = "The path to the file containing the pid of the server" +
-                          " (defaults to /var/run/centraldogma.pid)",
+                          " (defaults to ./dogma.pid)",
             converter = FileConverter.class)
     private File pidFile;
 
@@ -81,6 +86,7 @@ enum State {
     private CentralDogma dogma;
     @Nullable
     private PidFile procIdFile;
+    private boolean procIdFileCreated;
 
     private Main(String[] args) {
         final JCommander commander = new JCommander(this);
@@ -90,8 +96,7 @@ private Main(String[] args) {
         if (help != null && help) {
             commander.usage();
         } else {
-            procIdFile = new PidFile(Optional.ofNullable(pidFile)
-                                             .orElseGet(() -> new File("/var/run/centraldogma.pid")));
+            procIdFile = new PidFile(firstNonNull(pidFile, DEFAULT_PID_FILE));
             state = State.INITIALIZED;
         }
     }
@@ -126,6 +131,7 @@ synchronized void start() throws Exception {
         // because the state has been updated.
         assert procIdFile != null;
         procIdFile.create();
+        procIdFileCreated = true;
     }
 
     @Nullable
@@ -174,10 +180,12 @@ void destroy() {
         }
 
         assert procIdFile != null;
-        try {
-            procIdFile.destroy();
-        } catch (IOException e) {
-            logger.warn("Failed to destroy the PID file:", e);
+        if (procIdFileCreated) {
+            try {
+                procIdFile.destroy();
+            } catch (IOException e) {
+                logger.warn("Failed to destroy the PID file:", e);
+            }
         }
 
         state = State.DESTROYED;

server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

@@ -459,7 +459,7 @@ private Map<String, Entry<?>> blockingFind(
         readLock();
         try (ObjectReader reader = jGitRepository.newObjectReader();
              TreeWalk treeWalk = new TreeWalk(reader);
-             RevWalk revWalk = new RevWalk(reader)) {
+             RevWalk revWalk = newRevWalk(reader)) {
 
             // Query on a non-exist revision will return empty result.
             final Revision headRevision = cachedHeadRevision();
@@ -566,13 +566,12 @@ private List<Commit> blockingHistory(Revision from, Revision to, String pathPatt
 
         // At this point, we are sure: from.major >= to.major
         readLock();
-        try (RevWalk revWalk = new RevWalk(jGitRepository)) {
+        try (RevWalk revWalk = newRevWalk()) {
             final ObjectId fromCommitId = commitIdDatabase.get(descendingRange.from());
             final ObjectId toCommitId = commitIdDatabase.get(descendingRange.to());
 
             // Walk through the commit tree to get the corresponding commit information by given filters
             revWalk.setTreeFilter(AndTreeFilter.create(TreeFilter.ANY_DIFF, PathPatternFilter.of(pathPattern)));
-
             revWalk.markStart(revWalk.parseCommit(fromCommitId));
             final RevCommit toCommit = revWalk.parseCommit(toCommitId);
             if (toCommit.getParentCount() != 0) {
@@ -599,7 +598,7 @@ private List<Commit> blockingHistory(Revision from, Revision to, String pathPatt
             // If the pathPattern does not contain "/**", the caller wants commits only with the specific path,
             // so skip the empty commit.
             if (needsLastCommit && pathPattern.contains(ALL_PATH)) {
-                try (RevWalk tmpRevWalk = new RevWalk(jGitRepository)) {
+                try (RevWalk tmpRevWalk = newRevWalk()) {
                     final RevCommit lastRevCommit = tmpRevWalk.parseCommit(toCommitId);
                     final Revision lastCommitRevision =
                             CommitUtil.extractRevision(lastRevCommit.getFullMessage());
@@ -665,7 +664,7 @@ public CompletableFuture<Map<String, Change<?>>> diff(Revision from, Revision to
 
             final RevisionRange range = normalizeNow(from, to).toAscending();
             readLock();
-            try (RevWalk rw = new RevWalk(jGitRepository)) {
+            try (RevWalk rw = newRevWalk()) {
                 final RevTree treeA = rw.parseTree(commitIdDatabase.get(range.from()));
                 final RevTree treeB = rw.parseTree(commitIdDatabase.get(range.to()));
 
@@ -709,7 +708,7 @@ private Map<String, Change<?>> blockingPreviewDiff(Revision baseRevision, Iterab
 
         readLock();
         try (ObjectReader reader = jGitRepository.newObjectReader();
-             RevWalk revWalk = new RevWalk(reader);
+             RevWalk revWalk = newRevWalk(reader);
              DiffFormatter diffFormatter = new DiffFormatter(null)) {
 
             final ObjectId baseTreeId = toTree(revWalk, baseRevision);
@@ -882,7 +881,7 @@ private CommitResult commit0(@Nullable Revision prevRevision, Revision nextRevis
 
         try (ObjectInserter inserter = jGitRepository.newObjectInserter();
              ObjectReader reader = jGitRepository.newObjectReader();
-             RevWalk revWalk = new RevWalk(reader)) {
+             RevWalk revWalk = newRevWalk(reader)) {
 
             final ObjectId prevTreeId = prevRevision != null ? toTree(revWalk, prevRevision) : null;
 
@@ -1025,8 +1024,8 @@ private int applyChanges(@Nullable Revision baseRevision, @Nullable ObjectId bas
                         }
                         break;
                     case RENAME: {
-                        final String newPath = ((String) change.content()).substring(
-                                1); // Strip the leading '/'.
+                        final String newPath =
+                                ((String) change.content()).substring(1); // Strip the leading '/'.
 
                         if (dirCache.getEntry(newPath) != null) {
                             throw new ChangeConflictException("a file exists at the target path: " + change);
@@ -1299,7 +1298,7 @@ private Revision blockingFindLatestRevision(Revision lastKnownRevision, String p
         // Convert the revisions to Git trees.
         final List<DiffEntry> diffEntries;
         readLock();
-        try (RevWalk revWalk = new RevWalk(jGitRepository)) {
+        try (RevWalk revWalk = newRevWalk()) {
             final RevTree treeA = toTree(revWalk, range.from());
             final RevTree treeB = toTree(revWalk, range.to());
             diffEntries = blockingCompareTrees(treeA, treeB);
@@ -1442,7 +1441,7 @@ private Revision cachedHeadRevision() {
      * Returns the current revision.
      */
     private Revision uncachedHeadRevision() {
-        try (RevWalk revWalk = new RevWalk(jGitRepository)) {
+        try (RevWalk revWalk = newRevWalk()) {
             final ObjectId headRevisionId = jGitRepository.resolve(R_HEADS_MASTER);
             if (headRevisionId != null) {
                 final RevCommit revCommit = revWalk.parseCommit(headRevisionId);
@@ -1466,6 +1465,23 @@ private RevTree toTree(RevWalk revWalk, Revision revision) {
         }
     }
 
+    private RevWalk newRevWalk() {
+        final RevWalk revWalk = new RevWalk(jGitRepository);
+        configureRevWalk(revWalk);
+        return revWalk;
+    }
+
+    private static RevWalk newRevWalk(ObjectReader reader) {
+        final RevWalk revWalk = new RevWalk(reader);
+        configureRevWalk(revWalk);
+        return revWalk;
+    }
+
+    private static void configureRevWalk(RevWalk revWalk) {
+        // Disable rewriteParents because otherwise `RevWalk` will load every commit into memory.
+        revWalk.setRewriteParents(false);
+    }
+
     private void readLock() {
         rwLock.readLock().lock();
         if (closePending.get() != null) {