Do not try to finish already completed phase

Profiling shows that completion of substatement phases is the most
expensive operation we are performing. This stems from the fact
that inference is reactive and eager -- hence at we can attempt
to complete a particular phase against a statement multiple times.

Each time we will end up also walking all children (and their
children), attempting to complete the phase -- even when a particular
child has already successfully completed it.

This exposes a slight issue in our logic, as we have always assumed
child statements need to go through all the phases with us -- which
has not been true due to 'typedef' statements reuse, which would
end up being churned through phase complete (but always end with
EFFECTIVE_MODEL completed).

Fix both issues by checking whether the statement has executed
requested (or a subsequent) phase before going into actual phase
execution. This also requires that any substatements added to
a statement transition to the parent's completed phase before they
are made visible -- otherwise we could could end up with a statement
tree whose root has completed EFFECTIVE_MODEL, but not all of its
substatements have, triggering state violation assertions.

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

diff --git a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java
index cf74e088acd5d33bd373bb839aca70b247b89ca2..36a4bea5612838ffb9deb0c07c6c0fcd588da944 100644 (file)
--- a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java
+++ b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java
@@ -68,6 +68,7 @@ import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContext.Mutable;
 import org.opendaylight.yangtools.yang.parser.spi.meta.StmtContextUtils;
 import org.opendaylight.yangtools.yang.parser.spi.source.ImplicitSubstatement;
 import org.opendaylight.yangtools.yang.parser.spi.source.SourceException;
+import org.opendaylight.yangtools.yang.parser.spi.source.StatementSourceReference;
 import org.opendaylight.yangtools.yang.parser.spi.source.SupportedFeaturesNamespace;
 import org.opendaylight.yangtools.yang.parser.spi.source.SupportedFeaturesNamespace.SupportedFeatures;
 import org.opendaylight.yangtools.yang.parser.stmt.reactor.NamespaceBehaviourWithListeners.KeyedValueAddedListener;
@@ -471,7 +472,13 @@ public abstract class StatementContextBase<A, D extends DeclaredStatement<A>, E
     public void addEffectiveSubstatement(final Mutable<?, ?, ?> substatement) {
         verifyStatement(substatement);
         beforeAddEffectiveStatement(1);
-        effective.add((StatementContextBase<?, ?, ?>) substatement);
+
+        final StatementContextBase<?, ?, ?> stmt = (StatementContextBase<?, ?, ?>) substatement;
+        final ModelProcessingPhase phase = completedPhase;
+        if (phase != null) {
+            ensureCompletedPhase(stmt, phase);
+        }
+        effective.add(stmt);
     }
 
     /**
@@ -487,19 +494,41 @@ public abstract class StatementContextBase<A, D extends DeclaredStatement<A>, E
         if (!statements.isEmpty()) {
             statements.forEach(StatementContextBase::verifyStatement);
             beforeAddEffectiveStatement(statements.size());
-            effective.addAll((Collection<? extends StatementContextBase<?, ?, ?>>) statements);
+
+            final Collection<? extends StatementContextBase<?, ?, ?>> casted =
+                    (Collection<? extends StatementContextBase<?, ?, ?>>) statements;
+            final ModelProcessingPhase phase = completedPhase;
+            if (phase != null) {
+                for (StatementContextBase<?, ?, ?> stmt : casted) {
+                    ensureCompletedPhase(stmt, phase);
+                }
+            }
+
+            effective.addAll(casted);
         }
     }
 
+    // Make sure target statement has transitioned at least to specified phase. This method is just before we take
+    // allow a statement to become our substatement. This is needed to ensure that every statement tree does not contain
+    // any statements which did not complete the same phase as the root statement.
+    private static void ensureCompletedPhase(final StatementContextBase<?, ?, ?> stmt,
+            final ModelProcessingPhase phase) {
+        verify(stmt.tryToCompletePhase(phase), "Statement %s cannot complete phase %s", stmt, phase);
+    }
+
     private static void verifyStatement(final Mutable<?, ?, ?> stmt) {
         verify(stmt instanceof StatementContextBase, "Unexpected statement %s", stmt);
     }
 
     private void beforeAddEffectiveStatement(final int toAdd) {
+        // We cannot allow statement to be further mutated
+        final StatementSourceReference ref = getStatementSourceReference();
+        verify(completedPhase != ModelProcessingPhase.EFFECTIVE_MODEL, "Cannot modify finished statement at %s", ref);
+
         final ModelProcessingPhase inProgressPhase = getRoot().getSourceContext().getInProgressPhase();
         checkState(inProgressPhase == ModelProcessingPhase.FULL_DECLARATION
                 || inProgressPhase == ModelProcessingPhase.EFFECTIVE_MODEL,
-                "Effective statement cannot be added in declared phase at: %s", getStatementSourceReference());
+                "Effective statement cannot be added in declared phase at: %s", ref);
 
         if (effective.isEmpty()) {
             effective = new ArrayList<>(toAdd);
@@ -527,13 +556,18 @@ public abstract class StatementContextBase<A, D extends DeclaredStatement<A>, E
     }
 
     /**
-     * Try to execute current {@link ModelProcessingPhase} of source parsing.
+     * Try to execute current {@link ModelProcessingPhase} of source parsing. If the phase has already been executed,
+     * this method does nothing.
      *
      * @param phase to be executed (completed)
-     * @return if phase was successfully completed
+     * @return true if phase was successfully completed
      * @throws SourceException when an error occurred in source parsing
      */
     final boolean tryToCompletePhase(final ModelProcessingPhase phase) {
+        return phase.isCompletedBy(completedPhase) || doTryToCompletePhase(phase);
+    }
+
+    private boolean doTryToCompletePhase(final ModelProcessingPhase phase) {
         final boolean finished = phaseMutation.isEmpty() ? true : runMutations(phase);
         if (completeChildren(phase) && finished) {
             onPhaseCompleted(phase);

diff --git a/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/ModelProcessingPhase.java b/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/ModelProcessingPhase.java
index 918163148970d5b6086882f3a5598a0713471589..f47b1ef06d524316549fae2c4c8b2571283dd557 100644 (file)
--- a/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/ModelProcessingPhase.java
+++ b/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/ModelProcessingPhase.java
@@ -45,7 +45,24 @@ public enum ModelProcessingPhase {
         this.previousPhase = previous;
     }
 
+    /**
+     * Return the preceding phase, or null if this phase is the first one.
+     *
+     * @return Preceding phase, if there is one
+     */
     public @Nullable ModelProcessingPhase getPreviousPhase() {
         return previousPhase;
     }
+
+    /**
+     * Determine whether this processing phase is implied to have completed by completion of some other phase.
+     * Algebraically this means that other is not null and is either this phase or its {@link #getPreviousPhase()} chain
+     * contains this phase.
+     *
+     * @param other Other phase
+     * @return True if this phase completes no later than specified phase.
+     */
+    public boolean isCompletedBy(final @Nullable ModelProcessingPhase other) {
+        return other != null && ordinal() <= other.ordinal();
+    }
 }