diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index fa18b671b84..8d7cfb99a72 100644 --- a/CHANGELOG.asciidoc +++ b/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) diff --git a/docs/src/upgrade/release-3.5.x.asciidoc b/docs/src/upgrade/release-3.5.x.asciidoc index cbf93718103..cf4d704afd6 100644 --- a/docs/src/upgrade/release-3.5.x.asciidoc +++ b/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`. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java index 363de6eea32..d4583d39811 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/remote/traversal/AbstractRemoteTraversal.java +++ b/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 getGraph() { throw new UnsupportedOperationException("Remote traversals do not support this method"); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java index da221d5ed61..494ba58f606 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traversal.java +++ b/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 getGraph(); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java index ac8919fff57..7f84469ee12 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java +++ b/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. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java index 4f314dcf8b6..17bec6b2af8 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java +++ b/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> 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 getGraph() { - return Optional.ofNullable(this.graph); + final Optional 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 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)); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java index d3f6215a5a5..2b94b852767 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/EmptyTraversal.java +++ b/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; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java index 2099ff85b6a..9625a561747 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java @@ -488,7 +488,19 @@ public static boolean anyStepRecursively(final Predicate predicate, final * @param traversal the root traversal to start application */ public static void applyTraversalRecursively(final Consumer> 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> 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 steps = traversal.getSteps(); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java index 05b1f6a17db..b7c352e23a9 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalTest.java +++ b/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 getGraph() { return null; diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java index 1cd5de5b933..ce039e8ea07 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java +++ b/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"));