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 fr...@apache.org on 2016/07/19 13:33:08 UTC
svn commit: r1753384 - in /jackrabbit/oak/trunk/oak-segment-tar/src:
main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java
test/java/org/apache/jackrabbit/oak/segment/NodeRecordTest.java
Author: frm
Date: Tue Jul 19 13:33:08 2016
New Revision: 1753384
URL: http://svn.apache.org/viewvc?rev=1753384&view=rev
Log:
OAK-4570 - Base node states should be reused across generations
Modified:
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java
jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/NodeRecordTest.java
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java?rev=1753384&r1=1753383&r2=1753384&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentWriter.java Tue Jul 19 13:33:08 2016
@@ -987,26 +987,11 @@ public class SegmentWriter {
}
assert compactionStats != null;
compactionStats.nodeCount++;
- if (state instanceof SegmentNodeState) {
- SegmentNodeState sns = ((SegmentNodeState) state);
- if (sameStore(sns)) {
- // This is a segment node state from an old generation. Check whether
- // an equivalent one of the current generation is in the cache
- if (isOldGeneration(sns.getRecordId())) {
- RecordId cachedId = nodeCache.get(sns.getStableId());
- if (cachedId != null) {
- compactionStats.cacheHits++;
- return cachedId;
- } else {
- compactionStats.cacheMiss++;
- }
- } else {
- // This segment node state is already in this store,
- // no need to write it again,
- compactionStats.deDupNodes++;
- return sns.getRecordId();
- }
- }
+
+ RecordId compactedId = deduplicateNode(state);
+
+ if (compactedId != null) {
+ return compactedId;
}
compactionStats.writesOps++;
@@ -1022,21 +1007,24 @@ public class SegmentWriter {
}
private RecordId writeNodeUncached(@Nonnull NodeState state, int depth) throws IOException {
- SegmentNodeState before = null;
- Template beforeTemplate = null;
ModifiedNodeState after = null;
+
if (state instanceof ModifiedNodeState) {
after = (ModifiedNodeState) state;
- NodeState base = after.getBaseState();
- if (base instanceof SegmentNodeState) {
- SegmentNodeState sns = ((SegmentNodeState) base);
- if (sameStore(sns)) {
- if (!isOldGeneration(sns.getRecordId())) {
- before = sns;
- beforeTemplate = before.getTemplate();
- }
- }
- }
+ }
+
+ RecordId beforeId = null;
+
+ if (after != null) {
+ beforeId = deduplicateNode(after.getBaseState());
+ }
+
+ SegmentNodeState before = null;
+ Template beforeTemplate = null;
+
+ if (beforeId != null) {
+ before = reader.readNode(beforeId);
+ beforeTemplate = before.getTemplate();
}
List<RecordId> ids = newArrayList();
@@ -1076,6 +1064,17 @@ public class SegmentWriter {
PropertyState property = state.getProperty(name);
assert property != null;
+ if (before != null) {
+ // If this property is already present in before (the base state)
+ // and it hasn't been modified use that one. This will result
+ // in an already compacted property to be reused given before
+ // has been already compacted.
+ PropertyState beforeProperty = before.getProperty(name);
+ if (property.equals(beforeProperty)) {
+ property = beforeProperty;
+ }
+ }
+
if (sameStore(property)) {
RecordId pid = ((Record) property).getRecordId();
if (isOldGeneration(pid)) {
@@ -1117,6 +1116,51 @@ public class SegmentWriter {
}
/**
+ * Try to deduplicate the passed {@code node}. This succeeds if
+ * the passed node state has already been persisted to this store and
+ * either it has the same generation or it has been already compacted
+ * and is still in the de-duplication cache for nodes.
+ *
+ * @param node The node states to de-duplicate.
+ * @return the id of the de-duplicated node or {@code null} if none.
+ */
+ private RecordId deduplicateNode(NodeState node) {
+ assert compactionStats != null;
+
+ if (!(node instanceof SegmentNodeState)) {
+ // De-duplication only for persisted node states
+ return null;
+ }
+
+ SegmentNodeState sns = (SegmentNodeState) node;
+
+ if (!sameStore(sns)) {
+ // De-duplication only within same store
+ return null;
+ }
+
+ if (!isOldGeneration(sns.getRecordId())) {
+ // This segment node state is already in this store, no need to
+ // write it again
+ compactionStats.deDupNodes++;
+ return sns.getRecordId();
+ }
+
+ // This is a segment node state from an old generation. Check
+ // whether an equivalent one of the current generation is in the
+ // cache
+ RecordId compacted = nodeCache.get(sns.getStableId());
+
+ if (compacted == null) {
+ compactionStats.cacheMiss++;
+ return null;
+ }
+
+ compactionStats.cacheHits++;
+ return compacted;
+ }
+
+ /**
* @param node
* @return {@code true} iff {@code node} originates from the same segment store.
*/
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/NodeRecordTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/NodeRecordTest.java?rev=1753384&r1=1753383&r2=1753384&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/NodeRecordTest.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/NodeRecordTest.java Tue Jul 19 13:33:08 2016
@@ -20,6 +20,7 @@ package org.apache.jackrabbit.oak.segmen
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import javax.annotation.Nonnull;
@@ -28,7 +29,6 @@ import com.google.common.base.Supplier;
import org.apache.jackrabbit.oak.plugins.memory.EmptyNodeState;
import org.apache.jackrabbit.oak.segment.file.FileStore;
import org.apache.jackrabbit.oak.segment.file.FileStoreBuilder;
-import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
@@ -90,7 +90,6 @@ public class NodeRecordTest {
}
@Test
- @Ignore("OAK-4570")
public void baseNodeStateShouldBeReusedAcrossGenerations() throws Exception {
try (FileStore store = newFileStore()) {
Generation generation = new Generation();
@@ -110,7 +109,11 @@ public class NodeRecordTest {
// Write a new node with a non trivial template. This record will
// belong to generation 1.
- SegmentNodeState base = writer.writeNode(EmptyNodeState.EMPTY_NODE.builder().setProperty("k", "v1").getNodeState());
+ SegmentNodeState base = writer.writeNode(EmptyNodeState.EMPTY_NODE.builder()
+ .setProperty("a", "a")
+ .setProperty("k", "v1")
+ .getNodeState()
+ );
writer.flush();
generation.set(2);
@@ -148,6 +151,16 @@ public class NodeRecordTest {
// to a new generation.
assertEquals(modified.getTemplateId(), compacted.getTemplateId());
+
+ // Similarly the node state should have reused the property from
+ // the compacted node state, since this property didn't change.
+
+ Record modifiedProperty = (Record) modified.getProperty("a");
+ Record compactedProperty = (Record) compacted.getProperty("a");
+
+ assertNotNull(modifiedProperty);
+ assertNotNull(compactedProperty);
+ assertEquals(modifiedProperty.getRecordId(), compactedProperty.getRecordId());
}
}