make HistoryGuru#getLastHistoryEntry more efficient (#4271)

vladak · web-flow · commit 22e1b30970bd · 2023-04-05T16:46:58.000+02:00
diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/FileHistoryCache.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/FileHistoryCache.java
@@ -209,6 +209,22 @@ static History readHistory(File cacheFile, Repository repository) throws IOExcep
         return history;
     }
 
+    static HistoryEntry readLastHistoryEntry(File cacheFile) throws IOException {
+        SmileFactory factory = new SmileFactory();
+        ObjectMapper mapper = new SmileMapper();
+        HistoryEntry historyEntry = null;
+
+        try (SmileParser parser = factory.createParser(cacheFile)) {
+            parser.setCodec(mapper);
+            Iterator<HistoryEntry> historyEntryIterator = parser.readValuesAs(HistoryEntry.class);
+            if (historyEntryIterator.hasNext()) {
+                historyEntry = historyEntryIterator.next();
+            }
+        }
+
+        return historyEntry;
+    }
+
     /**
      * Write serialized object to file.
      * @param history {@link History} instance to be stored
@@ -653,6 +669,27 @@ public History get(File file, Repository repository, boolean withFiles) throws C
         return null;
     }
 
+    @Override
+    public HistoryEntry getLastHistoryEntry(File file) throws CacheException {
+        if (isUpToDate(file)) {
+            File cacheFile = getCachedFile(file);
+            try {
+                if (fileHistoryCacheHits != null) {
+                    fileHistoryCacheHits.increment();
+                }
+                return readLastHistoryEntry(cacheFile);
+            } catch (Exception e) {
+                LOGGER.log(Level.WARNING, String.format("Error when reading cache file '%s'", cacheFile), e);
+            }
+        }
+
+        if (fileHistoryCacheMisses != null) {
+            fileHistoryCacheMisses.increment();
+        }
+
+        return null;
+    }
+
     /**
      * @param file the file to check
      * @return {@code true} if the cache is up-to-date for the file, {@code false} otherwise
diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryCache.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryCache.java
@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opengrok.indexer.history;
 
@@ -43,6 +43,14 @@ interface HistoryCache extends Cache {
      */
     History get(File file, @Nullable Repository repository, boolean withFiles) throws CacheException;
 
+    /**
+     * Retrieve last (newest) history entry for the given file from the cache.
+     *
+     * @param file The file to retrieve history for
+     * @throws CacheException if the history cache cannot be read
+     */
+    HistoryEntry getLastHistoryEntry(File file) throws CacheException;
+
     /**
      * Store the history for a repository.
      *
diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryGuru.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryGuru.java
@@ -418,32 +418,39 @@ private History getHistoryFromCache(File file, Repository repository, boolean wi
         return null;
     }
 
+    @Nullable
+    private HistoryEntry getLastHistoryEntryFromCache(File file, Repository repository) throws CacheException {
+
+        if (useHistoryCache() && historyCache.supportsRepository(repository)) {
+            return historyCache.getLastHistoryEntry(file);
+        }
+
+        return null;
+    }
+
     /**
      * Get last {@link HistoryEntry} for a file. First, try to retrieve it from the cache.
      * If that fails, fallback to the repository method.
      * @param file file to get the history entry for
      * @param ui is the request coming from the UI
+     * @param fallback whether to perform fallback to repository method if cache retrieval fails
      * @return last (newest) history entry for given file or {@code null}
      * @throws HistoryException if history retrieval failed
      */
     @Nullable
-    public HistoryEntry getLastHistoryEntry(File file, boolean ui) throws HistoryException {
+    public HistoryEntry getLastHistoryEntry(File file, boolean ui, boolean fallback) throws HistoryException {
         Statistics statistics = new Statistics();
         LOGGER.log(Level.FINEST, "started retrieval of last history entry for ''{0}''", file);
         final File dir = file.isDirectory() ? file : file.getParentFile();
         final Repository repository = getRepository(dir);
 
-        History history;
         try {
-            history = getHistoryFromCache(file, repository, false);
-            if (history != null) {
-                HistoryEntry lastHistoryEntry = history.getLastHistoryEntry();
-                if (lastHistoryEntry != null) {
-                    statistics.report(LOGGER, Level.FINEST,
-                            String.format("got latest history entry %s for ''%s'' from history cache",
-                            lastHistoryEntry, file), "history.entry.latest");
-                    return lastHistoryEntry;
-                }
+            HistoryEntry lastHistoryEntry = getLastHistoryEntryFromCache(file, repository);
+            if (lastHistoryEntry != null) {
+                statistics.report(LOGGER, Level.FINEST,
+                        String.format("got latest history entry %s for ''%s'' from history cache",
+                        lastHistoryEntry, file), "history.entry.latest");
+                return lastHistoryEntry;
             }
         } catch (CacheException e) {
             LOGGER.log(Level.FINER,
@@ -452,6 +459,14 @@ public HistoryEntry getLastHistoryEntry(File file, boolean ui) throws HistoryExc
                             e.getMessage());
         }
 
+        if (!fallback) {
+            statistics.report(LOGGER, Level.FINEST,
+                    String.format("cannot retrieve the last history entry for ''%s'' in %s because fallback to" +
+                                    "repository method is disabled",
+                            file, repository), "history.entry.latest");
+            return null;
+        }
+
         if (!isRepoHistoryEligible(repository, file, ui)) {
             statistics.report(LOGGER, Level.FINEST,
                     String.format("cannot retrieve the last history entry for ''%s'' in %s because of settings",
diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/LatestRevisionUtil.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/LatestRevisionUtil.java
@@ -113,7 +113,7 @@ public static String getLastRevFromIndex(File file) {
 
     @Nullable
     private static String getLastRevFromHistory(File file) throws HistoryException {
-        HistoryEntry he = HistoryGuru.getInstance().getLastHistoryEntry(file, true);
+        HistoryEntry he = HistoryGuru.getInstance().getLastHistoryEntry(file, true, true);
         if (he != null) {
             return he.getRevision();
         }
diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/FileHistoryCacheTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/FileHistoryCacheTest.java
@@ -983,4 +983,23 @@ void testStoreAndTryToGetIgnored() throws Exception {
         retrievedHistory = cache.get(makefile, repo, true);
         assertNotNull(retrievedHistory, "history for Makefile should not be null");
     }
+
+    @Test
+    void testGetLastHistoryEntry() throws Exception {
+        File repositoryRoot = new File(repositories.getSourceRoot(), "git");
+
+        Repository repository = RepositoryFactory.getRepository(repositoryRoot);
+
+        cache.clear(repository);
+        File sourceFile = new File(repositoryRoot, "main.c");
+        assertTrue(repository.getHistory(sourceFile).getHistoryEntries().size() > 1);
+        assertTrue(sourceFile.exists());
+        assertNull(cache.getLastHistoryEntry(sourceFile));
+
+        History historyToStore = repository.getHistory(repositoryRoot);
+        cache.store(historyToStore, repository);
+        HistoryEntry historyEntry = cache.getLastHistoryEntry(sourceFile);
+        assertNotNull(historyEntry);
+        assertEquals("aa35c25882b9a60a97758e0ceb276a3f8cb4ae3a", historyEntry.getRevision());
+    }
 }
diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/HistoryGuruTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/HistoryGuruTest.java
@@ -377,7 +377,7 @@ void testScanningDepth() throws IOException {
     void testGetLastHistoryEntryNonexistent() throws Exception {
         HistoryGuru instance = HistoryGuru.getInstance();
         File file = new File("/nonexistent");
-        assertNull(instance.getLastHistoryEntry(file, true));
+        assertNull(instance.getLastHistoryEntry(file, true, true));
     }
 
     @ParameterizedTest
@@ -391,9 +391,9 @@ void testGetLastHistoryEntryVsIndexer(boolean isIndexerParam) throws HistoryExce
         File file = new File(repository.getSourceRoot(), "git");
         assertTrue(file.exists());
         if (isIndexerParam) {
-            assertThrows(IllegalStateException.class, () -> instance.getLastHistoryEntry(file, true));
+            assertThrows(IllegalStateException.class, () -> instance.getLastHistoryEntry(file, true, true));
         } else {
-            assertNotNull(instance.getLastHistoryEntry(file, true));
+            assertNotNull(instance.getLastHistoryEntry(file, true, true));
         }
         env.setIndexer(isIndexer);
         env.setTagsEnabled(isTagsEnabled);
@@ -417,7 +417,7 @@ void testGetLastHistoryEntryRepoHistoryDisabled() throws Exception {
 
         assertNotNull(repository);
         repository.setHistoryEnabled(false);
-        assertNull(instance.getLastHistoryEntry(file, false));
+        assertNull(instance.getLastHistoryEntry(file, false, true));
 
         // cleanup
         repository.setHistoryEnabled(true);

Original file line number	Diff line number	Diff line change
`@@ -113,7 +113,7 @@ public static String getLastRevFromIndex(File file) {`
`113`	`113`
`114`	`114`	`@Nullable`
`115`	`115`	`private static String getLastRevFromHistory(File file) throws HistoryException {`
`116`		`- HistoryEntry he = HistoryGuru.getInstance().getLastHistoryEntry(file, true);`
	`116`	`+ HistoryEntry he = HistoryGuru.getInstance().getLastHistoryEntry(file, true, true);`
`117`	`117`	`if (he != null) {`
`118`	`118`	`return he.getRevision();`
`119`	`119`	`}`