From 7ca8e1247a7fe52adef2febb71187791fb27782e Mon Sep 17 00:00:00 2001
From: Robert Varga <robert.varga@pantheon.tech>
Date: Wed, 15 Jan 2025 12:07:36 +0100
Subject: [PATCH] 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>
---
 .../data/tree/impl/ChildTrackingPolicy.java   |  3 ++-
 .../yang/data/tree/impl/ModifiedNode.java     | 23 ++++++++++++++-----
 .../yang/data/tree/impl/NodeModification.java |  2 +-
 3 files changed, 20 insertions(+), 8 deletions(-)

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 cbb4f3ac3b..e94a83e71d 100644
--- 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 988cbdb9e8..5c42c4c0a3 100644
--- 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 ad66331a94..7b12283c0e 100644
--- 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
-- 
2.36.6