[tinkerpop] branch TINKERPOP-2438 created (now f41fa10)

[sp...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

spmallette pushed a change to branch TINKERPOP-2438
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git.


      at f41fa10  TINKERPOP-2438 Improved performance of GremlinASTChecker

This branch includes the following new commits:

     new f41fa10  TINKERPOP-2438 Improved performance of GremlinASTChecker

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

[tinkerpop] 01/01: TINKERPOP-2438 Improved performance of GremlinASTChecker

[sp...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

spmallette pushed a commit to branch TINKERPOP-2438
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git

commit f41fa10a0dc6275ef35d2d84dd9e33dd4d864476
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Wed Dec 2 16:10:47 2020 -0500

    TINKERPOP-2438 Improved performance of GremlinASTChecker
    
    Used CompileStatic annotation and introduced a lightweight check to validate that there was reason to accept the expense of parsing. It's a bit of a hack in a sense but after some analysis it seems that there are no easy fixes. Everything ends in fairly advanced refactoring and extension of GremlinGroovyScriptEngine and GremlinGroovyClassLoader (with possible pull requests to Groovy). That seems like a fair bit of effort for what this feature is and I'm not sure it's worth it. The typ [...]
---
 CHANGELOG.asciidoc                                         |  1 +
 .../gremlin/groovy/jsr223/ast/GremlinASTChecker.groovy     | 14 +++++++++-----
 .../tinkerpop/gremlin/groovy/engine/GremlinExecutor.java   |  7 +++++--
 .../groovy/engine/GremlinExecutorOverGraphTest.java        |  1 -
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 96e04af..ba20a37 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -48,6 +48,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 * Fixed bug in `:bytecode` command preventing translations with whitespace from working properly.
 * Added `reset` and `config` options to the `:bytecode` command to allow for greater customization options.
 * Added GraphSON extension module and the `TinkerIoRegistry` to the default `GraphSONMapper` configuration used by the `:bytecode` command.
+* Added `GremlinASTChecker` to provide a way to extract properties of scripts before doing an actual `eval()`.
 * Avoided creating unnecessary detached objects in JVM.
 * Added support for `TraversalStrategy` usage in Javascript.
 * Added `Traversal.getTraverserSetSupplier()` to allow providers to supply their own `TraverserSet` instances.
diff --git a/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/jsr223/ast/GremlinASTChecker.groovy b/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/jsr223/ast/GremlinASTChecker.groovy
index a65dbc5..f048a9d 100644
--- a/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/jsr223/ast/GremlinASTChecker.groovy
+++ b/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/jsr223/ast/GremlinASTChecker.groovy
@@ -38,7 +38,10 @@ import org.codehaus.groovy.control.CompilePhase
 final class GremlinASTChecker {
 
     private static final AstBuilder astBuilder = new AstBuilder()
-    public static final Result EMPTY_RESULT = new Result(0);
+    public static final Result EMPTY_RESULT = new Result(0)
+
+    private static final List<String> tokens = ["evaluationTimeout", "scriptEvaluationTimeout",
+                                                "ARGS_EVAL_TIMEOUT", "ARGS_SCRIPT_EVAL_TIMEOUT"]
 
     /**
      * Parses a Gremlin script and extracts a {@code Result} containing properties that are relevant to the checker.
@@ -46,6 +49,10 @@ final class GremlinASTChecker {
     static Result parse(String gremlin) {
         if (gremlin.empty) return EMPTY_RESULT
 
+        // do a cheap check for tokens we care about - no need to parse unless one of these tokens is present in
+        // the string.
+        if (!tokens.any{ gremlin.contains(it) }) return EMPTY_RESULT
+
         List<ASTNode> ast = astBuilder.buildFromString(CompilePhase.CONVERSION, true, gremlin)
         TimeoutCheck tocheck = new TimeoutCheck()
         ast[0].visit(tocheck)
@@ -96,10 +103,7 @@ final class GremlinASTChecker {
         private static isTimeoutKey(def k) {
             // can't use Tokens object from here as it's in gremlin-driver. would rather not put a
             // groovy build in gremlin-driver so we're just stuck with strings for now.
-            return k == "evaluationTimeout" ||
-                    k == "scriptEvaluationTimeout" ||
-                    k == "ARGS_EVAL_TIMEOUT" ||
-                    k == "ARGS_SCRIPT_EVAL_TIMEOUT"
+            return tokens.contains(k)
         }
 
         private static getArgument(Expression expr) {
diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java
index 14a29a4..ad11748 100644
--- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java
+++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java
@@ -256,8 +256,11 @@ public class GremlinExecutor implements AutoCloseable {
         bindings.putAll(globalBindings);
         bindings.putAll(boundVars);
 
-        // override the timeout if the lifecycle has a value assigned.
-        final long scriptEvalTimeOut = lifeCycle.getEvaluationTimeoutOverride().orElse(evaluationTimeout);
+        // override the timeout if the lifecycle has a value assigned. if the script contains with(timeout)
+        // options then allow that value to override what's provided on the lifecycle
+        final Optional<Long> timeoutDefinedInScript = GremlinASTChecker.parse(script).getTimeout();
+        final long scriptEvalTimeOut = timeoutDefinedInScript.orElse(
+                lifeCycle.getEvaluationTimeoutOverride().orElse(evaluationTimeout));
 
         final CompletableFuture<Object> evaluationFuture = new CompletableFuture<>();
         final FutureTask<Void> evalFuture = new FutureTask<>(() -> {
diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorOverGraphTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorOverGraphTest.java
index e442185..3e0ad87 100644
--- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorOverGraphTest.java
+++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutorOverGraphTest.java
@@ -47,7 +47,6 @@ import static org.junit.Assert.fail;
 public class GremlinExecutorOverGraphTest {
     private final BasicThreadFactory testingThreadFactory = new BasicThreadFactory.Builder().namingPattern("test-gremlin-executor-%d").build();
 
-    @Ignore("GremlinASTChecker introduced some performance isssue on low latency traversals - resolve then re-enable this test")
     @Test
     public void shouldOverrideTimeoutWithinScript() throws Exception {
         final TinkerGraph graph = TinkerFactory.createModern();