You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by mr...@apache.org on 2015/10/27 13:48:56 UTC
svn commit: r1710800 - in /jackrabbit/oak/trunk/oak-core/src:
main/java/org/apache/jackrabbit/oak/plugins/document/MemoryDiffCache.java
test/java/org/apache/jackrabbit/oak/plugins/document/JournalEntryTest.java
Author: mreutegg
Date: Tue Oct 27 12:48:55 2015
New Revision: 1710800
URL: http://svn.apache.org/viewvc?rev=1710800&view=rev
Log:
OAK-3494: MemoryDiffCache should also check parent paths before falling to Loader (or returning null)
Apply patch by Vikas Saurabh and Marcel Reutegger
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/MemoryDiffCache.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalEntryTest.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/MemoryDiffCache.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/MemoryDiffCache.java?rev=1710800&r1=1710799&r2=1710800&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/MemoryDiffCache.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/MemoryDiffCache.java Tue Oct 27 12:48:55 2015
@@ -30,6 +30,9 @@ import org.apache.jackrabbit.oak.plugins
import com.google.common.cache.Cache;
import static com.google.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.commons.PathUtils.denotesRoot;
+import static org.apache.jackrabbit.oak.commons.PathUtils.getName;
+import static org.apache.jackrabbit.oak.commons.PathUtils.getParentPath;
/**
* An in-memory diff cache implementation.
@@ -53,20 +56,27 @@ public class MemoryDiffCache extends Dif
@CheckForNull
@Override
- public String getChanges(@Nonnull Revision from,
- @Nonnull Revision to,
- @Nonnull String path,
- final @Nullable Loader loader) {
+ public String getChanges(@Nonnull final Revision from,
+ @Nonnull final Revision to,
+ @Nonnull final String path,
+ @Nullable final Loader loader) {
PathRev key = diffCacheKey(path, from, to);
StringValue diff;
if (loader == null) {
diff = diffCache.getIfPresent(key);
+ if (diff == null && isUnchanged(from, to, path)) {
+ diff = new StringValue("");
+ }
} else {
try {
diff = diffCache.get(key, new Callable<StringValue>() {
@Override
public StringValue call() throws Exception {
- return new StringValue(loader.call());
+ if (isUnchanged(from, to, path)) {
+ return new StringValue("");
+ } else {
+ return new StringValue(loader.call());
+ }
}
});
} catch (ExecutionException e) {
@@ -119,4 +129,61 @@ public class MemoryDiffCache extends Dif
return new PathRev(from + path, to);
}
+ /**
+ * Returns {@code true} if it can be inferred from cache entries on
+ * ancestors of the given {@code path} that the node was not changed between
+ * the two revisions. This method returns {@code false} if there are no
+ * matching cache entries for the given revision range or one of them
+ * indicates that the node at the given path may have been modified.
+ *
+ * @param from the from revision.
+ * @param to the to revision.
+ * @param path the path of the node to check.
+ * @return {@code true} if there are cache entries that indicate the node
+ * at the given path was modified between the two revisions.
+ */
+ private boolean isUnchanged(@Nonnull final Revision from,
+ @Nonnull final Revision to,
+ @Nonnull final String path) {
+ return !denotesRoot(path)
+ && isChildUnchanged(from, to, getParentPath(path), getName(path));
+ }
+
+ private boolean isChildUnchanged(@Nonnull final Revision from,
+ @Nonnull final Revision to,
+ @Nonnull final String parent,
+ @Nonnull final String name) {
+ PathRev parentKey = diffCacheKey(parent, from, to);
+ StringValue parentCachedEntry = diffCache.getIfPresent(parentKey);
+ boolean unchanged;
+ if (parentCachedEntry == null) {
+ if (denotesRoot(parent)) {
+ // reached root and we don't know whether name
+ // changed between from and to
+ unchanged = false;
+ } else {
+ // recurse down
+ unchanged = isChildUnchanged(from, to,
+ getParentPath(parent), getName(parent));
+ }
+ } else {
+ unchanged = parseJsopDiff(parentCachedEntry.asString(), new Diff() {
+ @Override
+ public boolean childNodeAdded(String n) {
+ return !name.equals(n);
+ }
+
+ @Override
+ public boolean childNodeChanged(String n) {
+ return !name.equals(n);
+ }
+
+ @Override
+ public boolean childNodeDeleted(String n) {
+ return !name.equals(n);
+ }
+ });
+ }
+ return unchanged;
+ }
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalEntryTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalEntryTest.java?rev=1710800&r1=1710799&r2=1710800&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalEntryTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalEntryTest.java Tue Oct 27 12:48:55 2015
@@ -21,6 +21,7 @@ import java.util.Collections;
import java.util.List;
import java.util.Random;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
@@ -34,7 +35,9 @@ import org.junit.Test;
import static org.apache.jackrabbit.oak.plugins.document.Collection.JOURNAL;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -65,6 +68,46 @@ public class JournalEntryTest {
sort.close();
}
+ //OAK-3494
+ @Test
+ public void useParentDiff() throws Exception {
+ DiffCache cache = new MemoryDiffCache(new DocumentMK.Builder());
+ Revision from = new Revision(1, 0, 1);
+ Revision to = new Revision(2, 0, 1);
+ Revision unjournalled = new Revision(3, 0, 1);
+
+ //Put one entry for (from, to, "/a/b")->["c1", "c2"] manually
+ DiffCache.Entry entry = cache.newEntry(from, to, false);
+ entry.append("/a/b", "^\"c1\":{}^\"c2\":{}");
+ entry.done();
+
+ //NOTE: calling validateCacheUsage fills the cache with an empty diff for the path being validated.
+ //So, we need to make sure that each validation is done on a separate path.
+
+ //Cases that cache can answer (only c1 and c2 sub-trees are possibly changed)
+ validateCacheUsage(cache, from, to, "/a/b/c3", true);
+ validateCacheUsage(cache, from, to, "/a/b/c4/e/f/g", true);
+
+ //Cases that cache can't answer
+ validateCacheUsage(cache, from, to, "/a/b/c1", false); //cached entry says that c1 sub-tree is changed
+ validateCacheUsage(cache, from, to, "/a/b/c2/d", false); //cached entry says that c2 sub-tree is changed
+ validateCacheUsage(cache, from, to, "/c", false);//there is no cache entry for the whole hierarchy
+
+ //Fill cache using journal
+ List<String> paths = Lists.newArrayList("/content/changed", "/content/changed1/child1");
+ StringSort sort = JournalEntry.newSorter();
+ add(sort, paths);
+ sort.sort();
+ JournalEntry.applyTo(sort, cache, from, to);
+
+ validateCacheUsage(cache, from, to, "/topUnchanged", true);
+ validateCacheUsage(cache, from, to, "/content/changed/unchangedLeaf", true);
+ validateCacheUsage(cache, from, to, "/content/changed1/child2", true);
+
+ //check against an unjournalled revision (essentially empty cache)
+ validateCacheUsage(cache, from, unjournalled, "/unjournalledPath", false);
+ }
+
@Test
public void fillExternalChanges() throws Exception {
DocumentStore store = new MemoryDocumentStore();
@@ -146,4 +189,24 @@ public class JournalEntryTest {
}
}
}
+
+ private void validateCacheUsage(DiffCache cache, Revision from, Revision to, String path, boolean cacheExpected) {
+ String nonLoaderDiff = cache.getChanges(from, to, path, null);
+ final AtomicBoolean loaderCalled = new AtomicBoolean(false);
+ cache.getChanges(from, to, path, new DiffCache.Loader() {
+ @Override
+ public String call() {
+ loaderCalled.set(true);
+ return "";
+ }
+ });
+
+ if (cacheExpected) {
+ assertNotNull(nonLoaderDiff);
+ assertFalse(loaderCalled.get());
+ } else {
+ assertNull(nonLoaderDiff);
+ assertTrue(loaderCalled.get());
+ }
+ }
}