Optimize MandatoryLeafEnforcer footprint

Instead of using internal YangInstanceIdentifiers, which have overhead,
keep simple ImmutableList<PathArgument> -- and wire to a more direct
alternative of findNode().

Change-Id: Icfdf5424a1478db0b45515f43df8406ae7527c3d
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/MandatoryLeafEnforcer.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/MandatoryLeafEnforcer.java
index fc1cf9f1e9d6bfdc9d6c1cb47d3de025eb1f04c8..5bd626a83ca48dcf117024eb79ed689c5b1f9cd4 100644 (file)
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/MandatoryLeafEnforcer.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/MandatoryLeafEnforcer.java
@@ -15,6 +15,7 @@ import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.yangtools.concepts.Immutable;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNodes;
 import org.opendaylight.yangtools.yang.data.tree.api.DataTreeConfiguration;
@@ -31,9 +32,42 @@ import org.slf4j.LoggerFactory;
 final class MandatoryLeafEnforcer implements Immutable {
     private static final Logger LOG = LoggerFactory.getLogger(MandatoryLeafEnforcer.class);
 
-    private final ImmutableList<YangInstanceIdentifier> mandatoryNodes;
+    // FIXME: Well, there is still room to optimize footprint and performance. This list of lists can have overlaps,
+    //        i.e. in the case of:
+    //
+    //          container foo {
+    //            presence "something";
+    //            container bar {
+    //              leaf baz { type string; mandatory true; }
+    //              leaf xyzzy { type string; mandatory true; }
+    //            }
+    //          }
+    //
+    //        we will have populated:
+    //          [ foo, bar ]
+    //          [ foo, xyzzy ]
+    //        and end up looking up 'foo' twice. A better alternative to the inner list would be a recursive tree
+    //        structure with terminal and non-terminal nodes:
+    //
+    //          non-terminal(foo):
+    //            terminal(bar)
+    //            terminal(xyzzy)
+    //
+    //        And then we would have a List of (unique) mandatory immediate descendants -- and recurse through them.
+    //
+    //        For absolute best footprint this means keeping a PathArgument (for terminal) and perhaps an
+    //        ImmutableList<Object> (for non-terminal) nodes -- since PathArgument and ImmutableList are disjunct, we
+    //        can make a quick instanceof determination to guide us.
+    //
+    //        We then can short-circuit to using NormalizedNodes.getDirectChild(), or better yet, we can talk directly
+    //        to DistinctNodeContainer.childByArg() -- which seems to be the context in which we are invoked.
+    //        At any rate, we would perform lookups step by step -- and throw an exception when to find a child.
+    //        Obviously in that case we need to reconstruct the full path -- but that is an error path and we can expend
+    //        some time to get at that. A simple Deque<PathArgument> seems like a reasonable compromise to track what we
+    //        went through.
+    private final ImmutableList<ImmutableList<PathArgument>> mandatoryNodes;
 
-    private MandatoryLeafEnforcer(final ImmutableList<YangInstanceIdentifier> mandatoryNodes) {
+    private MandatoryLeafEnforcer(final ImmutableList<ImmutableList<PathArgument>> mandatoryNodes) {
         this.mandatoryNodes = requireNonNull(mandatoryNodes);
     }
 
@@ -43,7 +77,7 @@ final class MandatoryLeafEnforcer implements Immutable {
             return null;
         }
 
-        final var builder = ImmutableList.<YangInstanceIdentifier>builder();
+        final var builder = ImmutableList.<ImmutableList<PathArgument>>builder();
         findMandatoryNodes(builder, YangInstanceIdentifier.of(), schema, treeConfig.getTreeType());
         final var mandatoryNodes = builder.build();
         return mandatoryNodes.isEmpty() ? null : new MandatoryLeafEnforcer(mandatoryNodes);
@@ -53,7 +87,7 @@ final class MandatoryLeafEnforcer implements Immutable {
         for (var path : mandatoryNodes) {
             if (!NormalizedNodes.findNode(data, path).isPresent()) {
                 throw new IllegalArgumentException(String.format("Node %s is missing mandatory descendant %s",
-                    data.name(), path));
+                    data.name(), YangInstanceIdentifier.of(path)));
             }
         }
     }
@@ -62,7 +96,7 @@ final class MandatoryLeafEnforcer implements Immutable {
         enforceOnData(tree.getData());
     }
 
-    private static void findMandatoryNodes(final Builder<YangInstanceIdentifier> builder,
+    private static void findMandatoryNodes(final Builder<ImmutableList<PathArgument>> builder,
             final YangInstanceIdentifier id, final DataNodeContainer schema, final TreeType type) {
         for (var child : schema.getChildNodes()) {
             if (SchemaAwareApplyOperation.belongsToTree(type, child)) {
@@ -86,9 +120,9 @@ final class MandatoryLeafEnforcer implements Immutable {
                             .orElse(Boolean.FALSE);
                     }
                     if (needEnforce) {
-                        final var desc = id.node(NodeIdentifier.create(child.getQName())).toOptimized();
+                        final var desc = id.node(NodeIdentifier.create(child.getQName()));
                         LOG.debug("Adding mandatory child {}", desc);
-                        builder.add(desc);
+                        builder.add(ImmutableList.copyOf(desc.getPathArguments()));
                     }
                 }
             }