TINKERPOP-2855 Refactored strategy application

CHANGELOG.asciidoc

@@ -36,11 +36,14 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 * Bumped to Groovy 2.5.21.
 * Fixed bug in parsing of `math()` expressions where variables were not being identified if they contained a text associated with a function.
 * Refactored `FilterRankingStrategy` to improve performance for deeply nested traversals.
+* Refactored strategy application to improve performance by avoiding some excessive recursion.
+* Added `Traversal.lock()` to provide an explicit way to finalize a traversal object.
+* Changed `Traversal.getGraph()` to get its `Graph` object from itself or, if not available, its parent.
 * Added `AuthInfoProvider` interface and `NewDynamicAuth()` to gremlin-go for dynamic authentication support.
 * Bumped to `snakeyaml` 2.0 to fix security vulnerability.
 * Bumped to Apache `commons-configuration` 2.9.0 to fix security vulnerability.
 * Improved `count` step optimization for negative values in input for 'eq' comparison.
-* Fixed performance issue when using SampleGlobalStep with a traverser that has either a LABELED_PATH or PATH requirement.
+* Fixed performance issue when using `SampleGlobalStep` with a traverser that has either a `LABELED_PATH` or `PATH` requirement.
 
 [[release-3-5-5]]
 === TinkerPop 3.5.5 (Release Date: January 16, 2023)

docs/src/upgrade/release-3.5.x.asciidoc

@@ -31,6 +31,23 @@ complete list of all the modifications that are part of this release.
 === Upgrading for Users
 
 
+=== Upgrading for Providers
+
+==== Graph System Providers
+
+===== TraversalStrategy Expectations
+
+Given some important performance improvements in this version, providers may need to make alterations to their
+`TraversalStrategy` implementations as certain assumptions about the data available to a strategy may have changed.
+If a `TraversalStrategy` implementation requires access to the `Graph` implementation, side-effects, and similar data
+stored on a `Traversal`, it is best to get that information from the root of the traversal hierarchy rather than from
+the current traversal that the strategy is executing on as the strategy application process no longer take the
+expensive step to propagate that data throughout the traversal in between each strategy application. Use the
+`TraversalHelper.getRootTraversal()` helper function to get to the root traversal. Note also that
+`Traversal.getGraph()` will traverse through the parent traversals now when trying to find a `Graph`.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2855[TINKERPOP-2855],
+link:https://issues.apache.org/jira/browse/TINKERPOP-2888[TINKERPOP-2888]
 
 == TinkerPop 3.5.5
 
@@ -104,6 +121,7 @@ more likely now to note blocking behavior when a connection cannot be obtained.
 See: link:https://issues.apache.org/jira/browse/TINKERPOP-2813[TINKERPOP-2813]
 
 ==== Added User Agent to Gremlin drivers
+
 Previously, a server does not distinguish amongst the different types of clients connecting to it. We have now added
 user agent to web socket handshake in all the drivers, each with their own configuration to enable or disable user agent.
 User agent is enabled by default for all drivers.
@@ -117,6 +135,7 @@ User agent is enabled by default for all drivers.
 See: link:https://issues.apache.org/jira/browse/TINKERPOP-2480[TINKERPOP-2480]
 
 ==== Update to SSL Handshake Timeout Configuration
+
 Previously, the java driver relies on the default 10 second SSL handshake timeout defined by Netty. We have removed
 the default SSL handshake timeout. The SSL handshake timeout will instead be capped by setting `connectionSetupTimeoutMillis`.
 

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java

@@ -127,6 +127,11 @@ public boolean isLocked() {
         throw new UnsupportedOperationException("Remote traversals do not support this method");
     }
 
+    @Override
+    public void lock() {
+        throw new UnsupportedOperationException("Remote traversals do not support this method");
+    }
+
     @Override
     public Optional<Graph> getGraph() {
         throw new UnsupportedOperationException("Remote traversals do not support this method");

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java

@@ -543,7 +543,12 @@ public default boolean isRoot() {
         public boolean isLocked();
 
         /**
-         * Gets the {@link Graph} instance associated to this {@link Traversal}.
+         * Lock the traversal and perform any final adjustments to it after strategy application.
+         */
+        public void lock();
+
+        /**
+         * Gets the {@link Graph} instance associated directly to this {@link Traversal} or through its parent.
          */
         public Optional<Graph> getGraph();
 

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java

@@ -31,6 +31,7 @@
 import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
 import org.apache.tinkerpop.gremlin.process.traversal.util.EmptyTraversalSideEffects;
 import org.apache.tinkerpop.gremlin.process.traversal.util.EmptyTraversalStrategies;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
 import org.apache.tinkerpop.gremlin.structure.Graph;
 
 import java.util.Collections;
@@ -170,6 +171,11 @@ public boolean isLocked() {
         return null == this.bypassTraversal || this.bypassTraversal.isLocked();
     }
 
+    @Override
+    public void lock() {
+       if (this.bypassTraversal != null) bypassTraversal.lock();
+    }
+
     /**
      * Implementations of this class can never be a root-level traversal as they are specialized implementations
      * intended to be child traversals by design.

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java

@@ -121,63 +121,20 @@ public TraverserGenerator getTraverserGenerator() {
     @Override
     public void applyStrategies() throws IllegalStateException {
         if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
-        TraversalHelper.reIdSteps(this.stepPosition, this);
-        final boolean hasGraph = null != this.graph;
-
-        // we only want to apply strategies on the top-level step or if we got some graphcomputer stuff going on.
-        // seems like in that case, the "top-level" of the traversal is really held by the VertexProgramStep which
-        // needs to have strategies applied on "pure" copies of the traversal it is holding (i think). it further
-        // seems that we need three recursions over the traversal hierarchy to ensure everything "works", where
-        // strategy application requires top-level strategies and side-effects pushed into each child and then after
-        // application of the strategies we need to call applyStrategies() on all the children to ensure that their
-        // steps get reId'd and traverser requirements are set.
-        if (isRoot() || this.getParent() instanceof VertexProgramStep) {
 
-            // prepare the traversal and all its children for strategy application
-            TraversalHelper.applyTraversalRecursively(t -> {
-                if (hasGraph) t.setGraph(this.graph);
-                t.setStrategies(this.strategies);
-                t.setSideEffects(this.sideEffects);
-            }, this);
-
-            // note that prior to applying strategies to children we used to set side-effects and strategies of all
-            // children to that of the parent. under this revised model of strategy application from TINKERPOP-1568
-            // it doesn't appear to be necessary to do that (at least from the perspective of the test suite). by,
-            // moving side-effect setting after actual recursive strategy application we save a loop and by
-            // consequence also fix a problem where strategies might reset something in sideeffects which seems to
-            // happen in TranslationStrategy.
+        // apply strategies in order on a root traversal only or a traversal that is logically considered a root
+        // for GraphComputer as a child of VertexProgramStep.
+        if (isRoot() || this.getParent() instanceof VertexProgramStep) {
             final Iterator<TraversalStrategy<?>> strategyIterator = this.strategies.iterator();
             while (strategyIterator.hasNext()) {
                 final TraversalStrategy<?> strategy = strategyIterator.next();
-                TraversalHelper.applyTraversalRecursively(t -> {
-                    strategy.apply(t);
-
-                    // after the strategy is applied, it may have modified the traversal where a new traversal object
-                    // was added. if the strategy didn't set the Graph object it could leave that new traversal in a
-                    // state where another strategy might fail if that dependency is not satisfied
-                    TraversalHelper.applyTraversalRecursively(i -> {
-                        if (hasGraph) i.setGraph(this.graph);
-                    }, this);
-                }, this);
+                TraversalHelper.applyTraversalRecursively(strategy::apply, this);
             }
-
-            // don't need to re-apply strategies to "this" - leads to endless recursion in GraphComputer.
-            TraversalHelper.applyTraversalRecursively(t -> {
-                if (hasGraph) t.setGraph(this.graph);
-                if(!(t.isRoot()) && t != this && !t.isLocked()) {
-                    t.setSideEffects(this.sideEffects);
-                    t.applyStrategies();
-                }
-            }, this);
         }
-
-        this.finalEndStep = this.getEndStep();
 
-        // finalize requirements
-        if (this.isRoot()) {
-            resetTraverserRequirements();
-        }
-        this.locked = true;
+        // lock the traversal and its children for execution, finalizing the copy of any data from parent to child
+        // as needed
+        this.lock();
     }
 
     private void resetTraverserRequirements() {
@@ -337,6 +294,45 @@ public boolean isLocked() {
         return this.locked;
     }
 
+    /**
+     * Allow the locked property of the traversal to be directly set by those who know what they are doing. Are you
+     * sure you know what you're doing?
+     */
+    public void setLocked(final boolean locked) {
+        this.locked = locked;
+    }
+
+    @Override
+    public void lock() {
+        TraversalHelper.reIdSteps(stepPosition, this);
+
+        // normalize strategies, side-effects, and Graph through children. obviously ignore the root traversal, since
+        // there is no parent to draw data from and ignore stuff wrapped for GraphComputer execution and the root
+        // from which to draw child data from is logically the current traversal.
+        if (!isRoot() && !(parent instanceof VertexProgramStep)) {
+            final TraversalParent parent = getParent();
+            final Traversal.Admin parentTraversal = parent.asStep().getTraversal().asAdmin();
+            this.setStrategies(parentTraversal.getStrategies());
+            this.setSideEffects(parentTraversal.getSideEffects());
+
+            // not sure why java is complaining about generics here that i have to do this??
+            if (parentTraversal.getGraph().isPresent())
+                this.setGraph((Graph) parentTraversal.getGraph().get());
+        }
+
+        this.finalEndStep = this.getEndStep();
+
+        if (this.isRoot()) {
+            resetTraverserRequirements();
+        }
+
+        // lock the parent before the children
+        this.locked = true;
+
+        // now lock all the children
+        TraversalHelper.applyTraversalRecursively(Admin::lock, this, true);
+    }
+
     /**
      * Determines if the traversal has been fully iterated and resources released.
      */
@@ -408,7 +404,13 @@ public TraversalParent getParent() {
 
     @Override
     public Optional<Graph> getGraph() {
-        return Optional.ofNullable(this.graph);
+        final Optional<Graph> optionalGraph =  Optional.ofNullable(this.graph);
+        if (!optionalGraph.isPresent() || optionalGraph.get() == EmptyGraph.instance()) {
+            final TraversalParent parent = getParent();
+            if (parent != EmptyStep.instance())
+                return parent.asStep().getTraversal().getGraph();
+        }
+        return optionalGraph;
     }
 
     @Override
@@ -420,7 +422,7 @@ public Optional<TraversalSource> getTraversalSource() {
     public void setGraph(final Graph graph) {
         this.graph = graph;
     }
-    
+
     @Override
     public boolean equals(final Object other) {
         return other != null && other.getClass().equals(this.getClass()) && this.equals(((Traversal.Admin) other));

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java

@@ -112,6 +112,11 @@ public boolean isLocked() {
         return true;
     }
 
+    @Override
+    public void lock() {
+        // nothing to do here as this type of traversal is always in a locked state
+    }
+
     @Override
     public TraverserGenerator getTraverserGenerator() {
         return null;

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java

@@ -488,7 +488,19 @@ public static boolean anyStepRecursively(final Predicate<Step> predicate, final
      * @param traversal the root traversal to start application
      */
     public static void applyTraversalRecursively(final Consumer<Traversal.Admin<?, ?>> consumer, final Traversal.Admin<?, ?> traversal) {
-        consumer.accept(traversal);
+        applyTraversalRecursively(consumer, traversal, false);
+    }
+
+    /**
+     * Apply the provider {@link Consumer} function to the provided {@link Traversal} and all of its children.
+     *
+     * @param consumer  the function to apply to the each traversal in the tree
+     * @param traversal the root traversal to start application
+     */
+    public static void applyTraversalRecursively(final Consumer<Traversal.Admin<?, ?>> consumer, final Traversal.Admin<?, ?> traversal,
+                                                 final boolean applyToChildrenOnly) {
+        if (!applyToChildrenOnly)
+            consumer.accept(traversal);
 
         // we get accused of concurrentmodification if we try a for(Iterable)
         final List<Step> steps = traversal.getSteps();

gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java

@@ -360,6 +360,11 @@ public boolean isLocked() {
             return false;
         }
 
+        @Override
+        public void lock() {
+
+        }
+
         @Override
         public Optional<Graph> getGraph() {
             return null;

gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java

@@ -2122,8 +2122,12 @@ public void shouldSupportJobChaining() throws Exception {
         assertEquals(6, graph2.traversal().V().values(PageRankVertexProgram.PAGE_RANK).count().next().intValue());
         assertEquals(24, graph2.traversal().V().values().count().next().intValue());
         //
-        final ComputerResult result3 = graph2.compute(graphProvider.getGraphComputer(graph2).getClass())
-                .program(TraversalVertexProgram.build().traversal(g.V().groupCount("m").by(__.values(PageRankVertexProgram.PAGE_RANK).count()).label().asAdmin()).create(graph2)).persist(GraphComputer.Persist.EDGES).result(GraphComputer.ResultGraph.NEW).submit().get();
+        final ComputerResult result3 = graph2.compute(graphProvider.getGraphComputer(graph2).getClass()).
+                program(TraversalVertexProgram.build().traversal(
+                        g.V().groupCount("m").
+                                by(__.values(PageRankVertexProgram.PAGE_RANK).count()).
+                              label().asAdmin()).create(graph2)).
+                persist(GraphComputer.Persist.EDGES).result(GraphComputer.ResultGraph.NEW).submit().get();
         final Graph graph3 = result3.graph();
         final Memory memory3 = result3.memory();
         assertTrue(memory3.keys().contains("m"));