Guard empty children

Switch backing implementations when we encountere an empty child map,
making empty HashMaps more readily reclaimable.

This improves our ability to spot state errors, such as we just solved,
at least for obvious situations.

JIRA: YANGTOOLS-1651
Change-Id: I500b8e8758c7499af777773409ec16615da4294a
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ChildTrackingPolicy.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ChildTrackingPolicy.java
index cbb4f3ac3b7eeb0ae83411f131a697f72f802cb2..e94a83e71df04037c5222844d8ae04e37051e8ee 100644 (file)
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ChildTrackingPolicy.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ChildTrackingPolicy.java
@@ -11,6 +11,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument;
 
 /**
@@ -54,5 +55,5 @@ abstract class ChildTrackingPolicy {
      *
      * @return An empty map instance
      */
-    abstract Map<PathArgument, ModifiedNode> createMap();
+    abstract @NonNull Map<PathArgument, ModifiedNode> createMap();
 }

diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java
index 988cbdb9e80681dd6c3b2e79cfc5dee505e77456..5c42c4c0a3e30e571f8a612fefc251c24a72759e 100644 (file)
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java
@@ -70,8 +70,8 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
     // on top of 'original' -- see the seal() method below.
     //
     // After this process completes, the following four fields are guaranteed to remain stable.
-    private final Map<PathArgument, ModifiedNode> children;
-    private LogicalOperation operation;
+    private @NonNull Map<PathArgument, ModifiedNode> children;
+    private @NonNull LogicalOperation operation;
 
     // The argument to LogicalOperation.{MERGE,WRITE}, invalid otherwise
     private NormalizedNode value;
@@ -296,19 +296,21 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
         clearSnapshot();
         writtenOriginal = null;
 
+        var clearChildren = children.isEmpty();
+
         switch (operation) {
             case TOUCH -> {
                 // A TOUCH node without any children is a no-op
-                if (children.isEmpty()) {
+                if (clearChildren) {
                     updateOperationType(LogicalOperation.NONE);
                 }
             }
             case WRITE -> {
                 // A WRITE can collapse all of its children
-                if (!children.isEmpty()) {
+                if (!clearChildren) {
                     final var applied = schema.apply(this, original(), version);
                     value = applied != null ? applied.data() : null;
-                    children.clear();
+                    clearChildren = true;
                 }
 
                 if (value == null) {
@@ -322,6 +324,15 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
                 // No-op
             }
         }
+
+        // Mild enforcement of invariants: children is supposed to be immutable from this point on.
+        // We could perform a defensive ImmutableMap.copyOf(), but that would force re-indexing, which we want to avoid.
+        // We could wrap in with Collections.unmodifiableMap(), but that costs heap, which we do not want to pay.
+        // This just squashes empty children into an immutable singleton, so we catch at least some would-be offenders.
+        // Removing references to empty HashMaps should also be friendly to GC.
+        if (clearChildren) {
+            children = Map.of();
+        }
     }
 
     private void clearSnapshot() {
@@ -338,7 +349,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
     }
 
     void updateOperationType(final LogicalOperation type) {
-        operation = type;
+        operation = requireNonNull(type);
         modType = null;
         applyChildren = null;
 

diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java
index ad66331a94a49688f894ec1619e7445264af1fd3..7b12283c0e9422604c6d014312e5d77c03d4e1c9 100644 (file)
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java
@@ -26,7 +26,7 @@ abstract sealed class NodeModification implements Identifiable<PathArgument> per
      *
      * @return Operation type.
      */
-    abstract LogicalOperation getOperation();
+    abstract @NonNull LogicalOperation getOperation();
 
     /**
      * Return the value which was written to this node. The returned object is only valid for