Author: mreutegg
Date: Tue Mar 3 13:41:48 2015
New Revision: 1663705
URL: http://svn.apache.org/r1663705
Log:
OAK-2562: DiffCache is inefficient
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreDiffTest.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java?rev=1663705&r1=1663704&r2=1663705&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeState.java
Tue Mar 3 13:41:48 2015
@@ -57,7 +57,6 @@ import com.google.common.collect.Iterato
import com.google.common.collect.Maps;
import static com.google.common.base.Preconditions.checkNotNull;
-import static
org.apache.jackrabbit.oak.plugins.document.util.Utils.unshareString;
import static
org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE;
/**
@@ -265,8 +264,7 @@ public class DocumentNodeState extends A
// use DocumentNodeStore compare
final long start = perfLogger.start();
try {
- return dispatch(store.diffChildren(this, mBase),
- mBase, diff);
+ return store.compare(this, mBase, diff);
} finally {
perfLogger
.end(start,
@@ -411,64 +409,6 @@ public class DocumentNodeState extends A
|| this.lastRevision.equals(other.lastRevision);
}
- private boolean dispatch(@Nonnull String jsonDiff,
- @Nonnull DocumentNodeState base,
- @Nonnull NodeStateDiff diff) {
- if (!AbstractNodeState.comparePropertiesAgainstBaseState(this, base,
diff)) {
- return false;
- }
- if (jsonDiff.trim().isEmpty()) {
- return true;
- }
- JsopTokenizer t = new JsopTokenizer(jsonDiff);
- boolean continueComparison = true;
- while (continueComparison) {
- int r = t.read();
- if (r == JsopReader.END) {
- break;
- }
- switch (r) {
- case '+': {
- String name = unshareString(t.readString());
- t.read(':');
- t.read('{');
- while (t.read() != '}') {
- // skip properties
- }
- continueComparison = diff.childNodeAdded(name,
getChildNode(name));
- break;
- }
- case '-': {
- String name = unshareString(t.readString());
- continueComparison = diff.childNodeDeleted(name,
base.getChildNode(name));
- break;
- }
- case '^': {
- String name = unshareString(t.readString());
- t.read(':');
- if (t.matches('{')) {
- t.read('}');
- continueComparison = diff.childNodeChanged(name,
- base.getChildNode(name), getChildNode(name));
- } else if (t.matches('[')) {
- // ignore multi valued property
- while (t.read() != ']') {
- // skip values
- }
- } else {
- // ignore single valued property
- t.read();
- }
- break;
- }
- default:
- throw new IllegalArgumentException("jsonDiff: illegal
token '"
- + t.getToken() + "' at pos: " + t.getLastPos() + '
' + jsonDiff);
- }
- }
- return continueComparison;
- }
-
/**
* Returns up to {@code limit} child node entries, starting after the given
* {@code name}.
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java?rev=1663705&r1=1663704&r2=1663705&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
Tue Mar 3 13:41:48 2015
@@ -21,11 +21,13 @@ import static com.google.common.base.Pre
import static com.google.common.collect.Iterables.toArray;
import static com.google.common.collect.Iterables.transform;
import static org.apache.jackrabbit.oak.api.CommitFailedException.MERGE;
+import static org.apache.jackrabbit.oak.commons.PathUtils.concat;
import static org.apache.jackrabbit.oak.plugins.document.Collection.NODES;
import static org.apache.jackrabbit.oak.plugins.document.DocumentMK.FAST_DIFF;
import static
org.apache.jackrabbit.oak.plugins.document.DocumentMK.MANY_CHILDREN_THRESHOLD;
import static org.apache.jackrabbit.oak.plugins.document.UpdateOp.Key;
import static org.apache.jackrabbit.oak.plugins.document.UpdateOp.Operation;
+import static
org.apache.jackrabbit.oak.plugins.document.util.Utils.unshareString;
import java.io.Closeable;
import java.io.IOException;
@@ -94,8 +96,10 @@ import org.apache.jackrabbit.oak.spi.com
import org.apache.jackrabbit.oak.spi.commit.CommitHook;
import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
import org.apache.jackrabbit.oak.spi.commit.Observer;
+import org.apache.jackrabbit.oak.spi.state.AbstractNodeState;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.apache.jackrabbit.oak.spi.state.NodeStateDiff;
import org.apache.jackrabbit.oak.spi.state.NodeStore;
import org.apache.jackrabbit.oak.stats.Clock;
import org.apache.jackrabbit.oak.util.PerfLogger;
@@ -848,7 +852,7 @@ public final class DocumentNodeStore
String to = Utils.getKeyUpperLimit(checkNotNull(path));
String from;
if (name != null) {
- from = Utils.getIdFromPath(PathUtils.concat(path, name));
+ from = Utils.getIdFromPath(concat(path, name));
} else {
from = Utils.getKeyLowerLimit(path);
}
@@ -873,7 +877,7 @@ public final class DocumentNodeStore
} else if (c.childNames.size() < limit && !c.isComplete) {
// fetch more and update cache
String lastName = c.childNames.get(c.childNames.size() - 1);
- String lastPath = PathUtils.concat(path, lastName);
+ String lastPath = concat(path, lastName);
from = Utils.getIdFromPath(lastPath);
int remainingLimit = limit - c.childNames.size();
List<NodeDocument> docs = store.query(Collection.NODES,
@@ -890,7 +894,7 @@ public final class DocumentNodeStore
Iterable<NodeDocument> it = transform(c.childNames, new
Function<String, NodeDocument>() {
@Override
public NodeDocument apply(String name) {
- String p = PathUtils.concat(path, name);
+ String p = concat(path, name);
NodeDocument doc = store.find(Collection.NODES,
Utils.getIdFromPath(p));
if (doc == null) {
docChildrenCache.invalidateAll();
@@ -931,7 +935,7 @@ public final class DocumentNodeStore
new Function<String, DocumentNodeState>() {
@Override
public DocumentNodeState apply(String input) {
- String p = PathUtils.concat(parent.getPath(), input);
+ String p = concat(parent.getPath(), input);
DocumentNodeState result = getNode(p, readRevision);
if (result == null) {
throw new DocumentStoreException("DocumentNodeState is
null for revision " + readRevision + " of " + p
@@ -1245,26 +1249,42 @@ public final class DocumentNodeStore
/**
* Compares the given {@code node} against the {@code base} state and
- * reports the differences on the children as a json diff string. This
- * method does not report any property changes between the two nodes.
+ * reports the differences to the {@link NodeStateDiff}.
*
* @param node the node to compare.
* @param base the base node to compare against.
- * @return the json diff.
- */
- String diffChildren(@Nonnull final DocumentNodeState node,
- @Nonnull final DocumentNodeState base) {
+ * @param diff handler of node state differences
+ * @return {@code true} if the full diff was performed, or
+ * {@code false} if it was aborted as requested by the handler
+ * (see the {@link NodeStateDiff} contract for more details)
+ */
+ boolean compare(@Nonnull final DocumentNodeState node,
+ @Nonnull final DocumentNodeState base,
+ @Nonnull final NodeStateDiff diff) {
+ if (!AbstractNodeState.comparePropertiesAgainstBaseState(node, base,
diff)) {
+ return false;
+ }
if (node.hasNoChildren() && base.hasNoChildren()) {
- return "";
+ return true;
}
- return diffCache.getChanges(base.getLastRevision(),
- node.getLastRevision(), node.getPath(),
- new DiffCache.Loader() {
- @Override
- public String call() {
- return diffImpl(base, node);
- }
- });
+ boolean useReadRevision = true;
+ // first lookup with read revisions of nodes and without loader
+ String jsop = diffCache.getChanges(base.getRevision(),
+ node.getRevision(), node.getPath(), null);
+ if (jsop == null) {
+ useReadRevision = false;
+ // fall back to last revisions with loader, this
+ // guarantees we get a diff
+ jsop = diffCache.getChanges(base.getLastRevision(),
+ node.getLastRevision(), node.getPath(),
+ new DiffCache.Loader() {
+ @Override
+ public String call() {
+ return diffImpl(base, node);
+ }
+ });
+ }
+ return dispatch(jsop, node, base, diff, useReadRevision);
}
String diff(@Nonnull final String fromRevisionId,
@@ -1305,13 +1325,13 @@ public final class DocumentNodeStore
t.read(':');
t.read('{');
t.read('}');
- writer.tag((char) r).key(PathUtils.concat(path, name));
+ writer.tag((char) r).key(concat(path, name));
writer.object().endObject().newline();
break;
}
case '-': {
String name = t.readString();
- writer.tag('-').value(PathUtils.concat(path, name));
+ writer.tag('-').value(concat(path, name));
writer.newline();
}
}
@@ -1715,6 +1735,69 @@ public final class DocumentNodeStore
//-----------------------------< internal
>---------------------------------
+ private boolean dispatch(@Nonnull String jsonDiff,
+ @Nonnull DocumentNodeState node,
+ @Nonnull DocumentNodeState base,
+ @Nonnull NodeStateDiff diff,
+ boolean useReadRevision) {
+ if (jsonDiff.trim().isEmpty()) {
+ return true;
+ }
+ Revision nodeRev = useReadRevision ? node.getRevision() :
node.getLastRevision();
+ Revision baseRev = useReadRevision ? base.getRevision() :
base.getLastRevision();
+ JsopTokenizer t = new JsopTokenizer(jsonDiff);
+ boolean continueComparison = true;
+ while (continueComparison) {
+ int r = t.read();
+ if (r == JsopReader.END) {
+ break;
+ }
+ switch (r) {
+ case '+': {
+ String name = unshareString(t.readString());
+ t.read(':');
+ t.read('{');
+ while (t.read() != '}') {
+ // skip properties
+ }
+ NodeState child = getNode(concat(node.getPath(), name),
nodeRev);
+ continueComparison = diff.childNodeAdded(name, child);
+ break;
+ }
+ case '-': {
+ String name = unshareString(t.readString());
+ NodeState child = getNode(concat(base.getPath(), name),
baseRev);
+ continueComparison = diff.childNodeDeleted(name, child);
+ break;
+ }
+ case '^': {
+ String name = unshareString(t.readString());
+ t.read(':');
+ if (t.matches('{')) {
+ t.read('}');
+ NodeState nodeChild = getNode(concat(node.getPath(),
name), nodeRev);
+ NodeState baseChild = getNode(concat(base.getPath(),
name), baseRev);
+ continueComparison = diff.childNodeChanged(
+ name, baseChild, nodeChild);
+ } else if (t.matches('[')) {
+ // ignore multi valued property
+ while (t.read() != ']') {
+ // skip values
+ }
+ } else {
+ // ignore single valued property
+ t.read();
+ }
+ break;
+ }
+ default:
+ throw new IllegalArgumentException("jsonDiff: illegal
token '"
+ + t.getToken() + "' at pos: " + t.getLastPos() + '
' + jsonDiff);
+ }
+ }
+ return continueComparison;
+ }
+
/**
* Search for presence of child node as denoted by path in the children
cache of parent
*
@@ -1762,7 +1845,7 @@ public final class DocumentNodeStore
// changed or removed properties
PropertyState toValue = to.getProperty(name);
if (!fromValue.equals(toValue)) {
- w.tag('^').key(PathUtils.concat(from.getPath(), name));
+ w.tag('^').key(concat(from.getPath(), name));
if (toValue == null) {
w.value(null);
} else {
@@ -1773,7 +1856,7 @@ public final class DocumentNodeStore
for (String name : to.getPropertyNames()) {
// added properties
if (!from.hasProperty(name)) {
- w.tag('^').key(PathUtils.concat(from.getPath(), name))
+ w.tag('^').key(concat(from.getPath(), name))
.encodedValue(to.getPropertyAsString(name)).newline();
}
}
@@ -1908,7 +1991,7 @@ public final class DocumentNodeStore
if (!childrenSet.contains(n)) {
w.tag('-').value(n).newline();
} else {
- String path = PathUtils.concat(parentPath, n);
+ String path = concat(parentPath, n);
DocumentNodeState n1 = getNode(path, fromRev);
DocumentNodeState n2 = getNode(path, toRev);
// this is not fully correct:
@@ -1971,7 +2054,7 @@ public final class DocumentNodeStore
}
for (DocumentNodeState child : getChildNodes(source, null,
Integer.MAX_VALUE)) {
String childName = PathUtils.getName(child.getPath());
- String destChildPath = PathUtils.concat(targetPath, childName);
+ String destChildPath = concat(targetPath, childName);
moveOrCopyNode(move, child, destChildPath, commit);
}
}
Modified:
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreDiffTest.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreDiffTest.java?rev=1663705&r1=1663704&r2=1663705&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreDiffTest.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreDiffTest.java
Tue Mar 3 13:41:48 2015
@@ -25,7 +25,6 @@ import org.apache.jackrabbit.oak.spi.sta
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.apache.jackrabbit.oak.spi.state.NodeStore;
import org.apache.jackrabbit.oak.stats.Clock;
-import org.junit.Ignore;
import org.junit.Test;
import static
org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState.EMPTY_NODE;
@@ -35,7 +34,6 @@ import static org.junit.Assert.assertEqu
public class DocumentNodeStoreDiffTest extends AbstractMongoConnectionTest {
// OAK-2562
- @Ignore
@Test
public void diff() throws Exception {
DocumentNodeStore store = mk.getNodeStore();
Modified:
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java
URL:
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java?rev=1663705&r1=1663704&r2=1663705&view=diff
==============================================================================
---
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java
(original)
+++
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreIT.java
Tue Mar 3 13:41:48 2015
@@ -18,6 +18,7 @@ package org.apache.jackrabbit.oak.plugin
import java.util.Collections;
+import org.apache.jackrabbit.oak.json.JsopDiff;
import
org.apache.jackrabbit.oak.plugins.document.util.TimingDocumentStoreWrapper;
import org.apache.jackrabbit.oak.plugins.document.util.Utils;
import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
@@ -110,9 +111,10 @@ public class DocumentNodeStoreIT extends
ns1.runBackgroundOperations();
ns2.runBackgroundOperations();
- String diff = ns1.diffChildren(root2, root1);
+ JsopDiff diff = new JsopDiff("", 0);
+ ns1.compare(root2, root1, diff);
// must report /node as changed
- assertEquals("^\"node\":{}", diff.trim());
+ assertEquals("^\"node\":{}", diff.toString());
ns1.dispose();
ns2.dispose();
