Introduce InMemoryDataTreeModification.AppliedToSnapshot

Once we complete ready(), the user side of DataTreeModification is free
to invoke applyToCursor() and newModification() methods.

Introduce AppliedToSnapshot, which we enter into when we first observe
Ready state in newModification(). Any subsequent calls to
newModification() will just pick up snapshot root from AppliedToSnapshot
without touching ModifiedNode -- and thus we do not need any further
locking.

For the applyToCursor() path we may be racing with validate()/prepare(),
so perform that operation with the exclusive lock.

This brings more clarity as to what ModifiedNode.children is now and how
it should behave in the future -- which we thoroughly outline in FIXMEs.

JIRA: YANGTOOLS-1651
Change-Id: Icfb55beba97d9432e444df87631a03975bebb8e4
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit b7935ec11fcc89d14c8e78db52e2bbba85ee0f23)

diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java
index d87846c2fafbff3a93e0661afddf5310c75ec859..de7dd5a12b4eb80127906ee1b0a3bf67865a94ac 100644 (file)
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java
@@ -124,8 +124,12 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
      *
      * <p>Since all of our state is kept on heap, we prioritize forward progress, so as to detach user state as soon
      * as possible, making it eligible for garbage collection.
+     *
+     * <p>We can transition from this state to
+     * <ul>
+     *   <li>{@link AppliedToSnapshot} via {@code newModification()}</li>
+     * </ul>
      */
-    // TODO: this is a terminal state for now, it needs to be further fleshed out
     @NonNullByDefault
     private record Ready(ModifiedNode root) implements State {
         Ready {
@@ -138,6 +142,28 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
         }
     }
 
+    /**
+     * The same thing as {@link Ready}, but we have also seen a call to {@code newModification()} and will never touch
+     * modification from that code path.
+     */
+    // TODO: This is a terminal state for now, it needs to be further fleshed out. Most notably root holds the side
+    //       effects of SchemaAwareOperation.apply(), but we do not use those results at all.
+    //       It feels like we should have a specialized transition when we observe this state in validate(), but doing
+    //       so requires a figuring out the relationship between 'applied' and the result of 'prepare': can we just
+    //       reuse the 'applied' when 'snapshot.getRootNode() == current'?
+    @NonNullByDefault
+    private record AppliedToSnapshot(ModifiedNode root, TreeNode applied) implements State {
+        AppliedToSnapshot {
+            requireNonNull(root);
+            requireNonNull(applied);
+        }
+
+        @Override
+        public String toString() {
+            return "AppliedToSnapshot";
+        }
+    }
+
     private static final Logger LOG = LoggerFactory.getLogger(InMemoryDataTreeModification.class);
 
     private static final VarHandle STATE;
@@ -223,6 +249,8 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
             rootNode = open.root;
         } else if (local instanceof Ready ready) {
             rootNode = ready.root;
+        } else if (local instanceof AppliedToSnapshot applied) {
+            rootNode = applied.root;
         } else if (local instanceof Noop) {
             // Defer to snapshot
             return Map.entry(YangInstanceIdentifier.of(), snapshot.getRootNode().getData());
@@ -317,25 +345,41 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
     public InMemoryDataTreeModification newModification() {
         final var local = acquireState();
         if (local instanceof Ready ready) {
-            return newModification(ready.root);
+            // Hard: we are the first ones to run apply(), perhaps racing with concurrent validate/prepare/commit
+            return newModification(ready);
+        } else if (local instanceof AppliedToSnapshot ready) {
+            // Simple: reuse already computed
+            return newModification(ready.applied);
         } else if (local instanceof Noop) {
-            // Simple fast case: just use the underlying modification
+            // Trivial: just use the underlying modification
             return snapshot.newModification();
         } else {
             throw illegalState(local, "chain on");
         }
     }
 
+    @NonNullByDefault
+    private InMemoryDataTreeModification newModification(final TreeNode rootNode) {
+        return new InMemoryDataTreeSnapshot(snapshot.modelContext(), rootNode, strategyTree).newModification();
+    }
+
     // synchronizes with prepare() and validate() to protect rootNode internals
     @NonNullByDefault
-    private synchronized InMemoryDataTreeModification newModification(final ModifiedNode rootNode) {
+    private synchronized InMemoryDataTreeModification newModification(final Ready ready) {
         // We will use preallocated version, this means returned snapshot will  have same version each time this method
         // is called.
-        final var newRoot = getStrategy().apply(rootNode, snapshotRoot(), version);
-        if (newRoot == null) {
+        final var after = getStrategy().apply(ready.root, snapshotRoot(), version);
+        if (after == null) {
+            // TODO: This precludes non-presence container as a root which completely disappears. I think we need to
+            //       supported a state when we have a non-existent root. IIRC there are other parts of code are not
+            //       ready for that happening just yet.
             throw new IllegalStateException("Data tree root is not present, possibly removed by previous modification");
         }
-        return new InMemoryDataTreeSnapshot(snapshot.modelContext(), newRoot, strategyTree).newModification();
+
+        // We are about to release exit a synchronized method, which implies a release fence. We therefore use only
+        // a plain set() here.
+        STATE.set(this, new AppliedToSnapshot(ready.root, after));
+        return newModification(after);
     }
 
     Version getVersion() {
@@ -397,6 +441,8 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
             rootNode = open.root;
         } else if (local instanceof Ready ready) {
             rootNode = ready.root;
+        } else if (local instanceof AppliedToSnapshot applied) {
+            rootNode = applied.root;
         } else if (local instanceof Noop) {
             // Nothing to apply
             return;
@@ -502,6 +548,8 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
         final var local = acquireState();
         if (local instanceof Ready ready) {
             rootNode = ready.root;
+        } else if (local instanceof AppliedToSnapshot applied) {
+            rootNode = applied.root;
         } else if (local instanceof Noop) {
             // Nothing to validate
             return;
@@ -530,6 +578,8 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements
         final var local = acquireState();
         if (local instanceof Ready ready) {
             return prepare(ready.root, path, current);
+        } else if (local instanceof AppliedToSnapshot applied) {
+            return prepare(applied.root, path, current);
         } else if (local instanceof Noop) {
             // Nothing to prepare
             return new NoopDataTreeCandidate(YangInstanceIdentifier.of(), current);

diff --git a/data/yang-data-tree-ri/src/test/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModificationTest.java b/data/yang-data-tree-ri/src/test/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModificationTest.java
index c817d6839eb6a8552417ca4874569434b971208e..5539262d49ddd4a81ed4725adfcb02c719566e14 100644 (file)
--- a/data/yang-data-tree-ri/src/test/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModificationTest.java
+++ b/data/yang-data-tree-ri/src/test/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModificationTest.java
@@ -63,37 +63,36 @@ class InMemoryDataTreeModificationTest extends AbstractTestModelTest {
             .withNodeIdentifier(new NodeIdentifier(TestModel.TEST_QNAME))
             .build());
         mod.ready();
-        final var ready = assertState("Ready");
+        assertState("Ready");
 
         // transitions state
         final var firstMod = assertInstanceOf(InMemoryDataTreeModification.class, mod.newModification());
+        final var applied = assertState("AppliedToSnapshot");
         assertNotSame(mod, firstMod);
         assertNotSame(mod.snapshotRoot(), firstMod.snapshotRoot());
-        assertState(ready);
 
         // does not transition state, but reuses underlying snapshot root node
         final var secondMod = assertInstanceOf(InMemoryDataTreeModification.class, mod.newModification());
-        assertState(ready);
+        assertState(applied);
         assertNotSame(mod, secondMod);
         assertNotSame(mod.snapshotRoot(), secondMod.snapshotRoot());
         assertNotSame(firstMod, secondMod);
-        // FIXME: we should return the same snapshot
-        assertNotSame(firstMod.snapshotRoot(), secondMod.snapshotRoot());
+        assertSame(firstMod.snapshotRoot(), secondMod.snapshotRoot());
 
         // validate is a no-op as we have already
         tree.validate(mod);
-        assertState(ready);
+        assertState(applied);
 
         final var candidate = tree.prepare(mod);
         assertNotNull(candidate);
         // FIXME: candidate does not transition for now: extend checks once it does, including a subsequent
         //        newModification()
-        assertState(ready);
+        assertState(applied);
 
         tree.commit(candidate);
         // TODO: we really would like to have a dedicated state which routes to the data tree for 'newModification'
         //       et al.
-        assertState(ready);
+        assertState(applied);
     }
 
     @Test
@@ -134,7 +133,7 @@ class InMemoryDataTreeModificationTest extends AbstractTestModelTest {
         assertState("Ready");
 
         assertNotNull(mod.newModification());
-        final var applied = assertState("Ready");
+        final var applied = assertState("AppliedToSnapshot");
 
         doNothing().when(cursor).write(eq(data.name()), same(data));
         mod.applyToCursor(cursor);