From 13c93cabac51112a73f592e0fba12e515f643522 Mon Sep 17 00:00:00 2001 From: BrynCooke Date: Thu, 2 Mar 2017 19:07:28 +0000 Subject: [PATCH 01/19] TINKERPOP-1644 Improve script compilation syncronisation Script compilation is synchronised. Script compilation times are placed in to logs. Failed scripts will not be recompiled. Scripts that take over 5 seconds to compile are logged as a warning. --- CHANGELOG.asciidoc | 4 ++ .../jsr223/GremlinGroovyScriptEngine.java | 57 +++++++++++++++++-- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 6861d57bf6c..24e5e9ce305 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,10 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Script compilation is synchronised. +* Script compilation times are placed in to logs. +* Failed scripts will not be recompiled. +* Scripts that take over 5 seconds to compile are logged as a warning. * Fixed an `NullPointerException` in `GraphMLReader` that occurred when an `` didn't have an ID field and the base graph supported ID assignment. * Split `ComputerVerificationStrategy` into two strategies: `ComputerVerificationStrategy` and `ComputerFinalizationStrategy`. * Removed `HasTest.g_V_hasId_compilationEquality` from process test suite as it makes too many assumptions about provider compilation. diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 1fb2efcf5e0..a8365a269b0 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -56,6 +56,8 @@ import org.codehaus.groovy.runtime.MetaClassHelper; import org.codehaus.groovy.runtime.MethodClosure; import org.codehaus.groovy.util.ReferenceBundle; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.script.Bindings; import javax.script.CompiledScript; @@ -93,6 +95,7 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements DependencyManager, AutoCloseable, GremlinScriptEngine { + private static final Logger log = LoggerFactory.getLogger(GremlinGroovyScriptEngine.class); /** * An "internal" key for sandboxing the script engine - technically not for public use. */ @@ -153,6 +156,7 @@ protected Map initialValue() { * Script to generated Class map. */ private ManagedConcurrentValueMap classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private ManagedConcurrentValueMap failedClassMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); /** * Global closures map - this is used to simulate a single global functions namespace @@ -529,13 +533,56 @@ public T getInterface(final Object thiz, final Class clazz) { return makeInterface(thiz, clazz); } + private Class getScriptClassFromMap(String script) throws CompilationFailedException { + CompilationFailedException exception = failedClassMap.get(script); + if(exception != null) { + throw exception; + } + return classMap.get(script); + + } + Class getScriptClass(final String script) throws CompilationFailedException { - Class clazz = classMap.get(script); - if (clazz != null) return clazz; + Class clazz = getScriptClassFromMap(script); + if (clazz != null) { + return clazz; + } + synchronized (this) { + clazz = getScriptClassFromMap(script); + if (clazz != null) { + return clazz; + } - clazz = loader.parseClass(script, generateScriptName()); - classMap.put(script, clazz); - return clazz; + long start = System.currentTimeMillis(); + try { + clazz = loader.parseClass(script, generateScriptName()); + long time = System.currentTimeMillis() - start; + if(time > 5000) { + //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. + //Scripts with a large numbers of parameters often trigger this and should be avoided. + log.warn("Script compilation {} took {}ms", script, time); + } + else { + log.debug("Script compilation {} took {}ms", script, time); + } + + + } + catch(CompilationFailedException t) { + long finish = System.currentTimeMillis(); + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, t); + failedClassMap.put(script, t); + throw t; + } + catch(Throwable t) { + long finish = System.currentTimeMillis(); + log.error("Script compilation FAILED with throwable {} took {}ms {}", script, finish - start, t); + throw t; + } + classMap.put(script, clazz); + + return clazz; + } } boolean isCached(final String script) { From deb68280d986b086120d56a73659b1869c07a475 Mon Sep 17 00:00:00 2001 From: BrynCooke Date: Tue, 7 Mar 2017 16:54:58 +0000 Subject: [PATCH 02/19] TINKERPOP-1644 Use future instead of maintaining a separate map of failures. --- .../jsr223/GremlinGroovyScriptEngine.java | 69 +++++++------------ 1 file changed, 26 insertions(+), 43 deletions(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index a8365a269b0..67275f8e1b2 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -79,6 +79,8 @@ import java.util.Map; import java.util.ServiceLoader; import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -155,8 +157,7 @@ protected Map initialValue() { /** * Script to generated Class map. */ - private ManagedConcurrentValueMap classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); - private ManagedConcurrentValueMap failedClassMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private ManagedConcurrentValueMap> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); /** * Global closures map - this is used to simulate a single global functions namespace @@ -533,56 +534,38 @@ public T getInterface(final Object thiz, final Class clazz) { return makeInterface(thiz, clazz); } - private Class getScriptClassFromMap(String script) throws CompilationFailedException { - CompilationFailedException exception = failedClassMap.get(script); - if(exception != null) { - throw exception; - } - return classMap.get(script); - - } - Class getScriptClass(final String script) throws CompilationFailedException { - Class clazz = getScriptClassFromMap(script); - if (clazz != null) { - return clazz; - } - synchronized (this) { - clazz = getScriptClassFromMap(script); + Future clazz = classMap.get(script); + + long start = System.currentTimeMillis(); + try { if (clazz != null) { - return clazz; + return clazz.get(); } - long start = System.currentTimeMillis(); - try { - clazz = loader.parseClass(script, generateScriptName()); - long time = System.currentTimeMillis() - start; - if(time > 5000) { - //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. - //Scripts with a large numbers of parameters often trigger this and should be avoided. - log.warn("Script compilation {} took {}ms", script, time); - } - else { - log.debug("Script compilation {} took {}ms", script, time); - } - + clazz = CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName())); + classMap.put(script, clazz); + return clazz.get(); - } - catch(CompilationFailedException t) { + } catch (Exception e) { + if (e.getCause() instanceof CompilationFailedException) { long finish = System.currentTimeMillis(); - log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, t); - failedClassMap.put(script, t); - throw t; + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e.getCause()); + throw (CompilationFailedException) e.getCause(); + } else { + throw new AssertionError("Unexpected exception when compiling script", e); } - catch(Throwable t) { - long finish = System.currentTimeMillis(); - log.error("Script compilation FAILED with throwable {} took {}ms {}", script, finish - start, t); - throw t; + } finally { + long time = System.currentTimeMillis() - start; + if (time > 5000) { + //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. + //Scripts with a large numbers of parameters often trigger this and should be avoided. + log.warn("Script compilation {} took {}ms", script, time); + } else { + log.debug("Script compilation {} took {}ms", script, time); } - classMap.put(script, clazz); - - return clazz; } + } boolean isCached(final String script) { From 18778e405981523355319fa56bd04fd6cc07b845 Mon Sep 17 00:00:00 2001 From: BrynCooke Date: Wed, 8 Mar 2017 12:24:46 +0000 Subject: [PATCH 03/19] TINKERPOP-1644 Use Caffeine cache --- gremlin-groovy/pom.xml | 5 ++ .../jsr223/GremlinGroovyScriptEngine.java | 75 +++++++++++-------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/gremlin-groovy/pom.xml b/gremlin-groovy/pom.xml index 511952aee49..474b458fb16 100644 --- a/gremlin-groovy/pom.xml +++ b/gremlin-groovy/pom.xml @@ -70,6 +70,11 @@ limitations under the License. jbcrypt 0.4 + + com.github.ben-manes.caffeine + caffeine + 2.3.1 + org.slf4j diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 67275f8e1b2..51850adb3bc 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -18,6 +18,9 @@ */ package org.apache.tinkerpop.gremlin.groovy.jsr223; +import com.github.benmanes.caffeine.cache.CacheLoader; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; import groovy.grape.Grape; import groovy.lang.Binding; import groovy.lang.Closure; @@ -80,6 +83,8 @@ import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Pattern; @@ -154,17 +159,44 @@ protected Map initialValue() { } }; + private GremlinGroovyClassLoader loader; + /** * Script to generated Class map. */ - private ManagedConcurrentValueMap> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private LoadingCache> classMap = Caffeine.newBuilder().softValues().build(new CacheLoader>() { + @Override + public Future load(String script) throws Exception { + long start = System.currentTimeMillis(); + try { + return CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName()), command -> command.run()); + } catch (Exception e) { + if (e.getCause() instanceof CompilationFailedException) { + long finish = System.currentTimeMillis(); + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e.getCause()); + throw (CompilationFailedException) e.getCause(); + } else { + throw new AssertionError("Unexpected exception when compiling script", e); + } + } finally { + long time = System.currentTimeMillis() - start; + if (time > 5000) { + //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. + //Scripts with a large numbers of parameters often trigger this and should be avoided. + log.warn("Script compilation {} took {}ms", script, time); + } else { + log.debug("Script compilation {} took {}ms", script, time); + } + } + } + + }); /** * Global closures map - this is used to simulate a single global functions namespace */ private ManagedConcurrentValueMap globalClosures = new ManagedConcurrentValueMap<>(ReferenceBundle.getHardBundle()); - private GremlinGroovyClassLoader loader; private AtomicLong counter = new AtomicLong(0L); @@ -420,7 +452,7 @@ private void internalReset() { // must clear the local cache here because the classloader has been reset. therefore, classes previously // referenced before that might not have evaluated might cleanly evaluate now. - classMap.clear(); + classMap.invalidateAll(); globalClosures.clear(); final Set toReuse = new HashSet<>(artifactsToUse); @@ -535,41 +567,20 @@ public T getInterface(final Object thiz, final Class clazz) { } Class getScriptClass(final String script) throws CompilationFailedException { - Future clazz = classMap.get(script); - - long start = System.currentTimeMillis(); try { - if (clazz != null) { - return clazz.get(); - } - - clazz = CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName())); - classMap.put(script, clazz); - return clazz.get(); - - } catch (Exception e) { - if (e.getCause() instanceof CompilationFailedException) { - long finish = System.currentTimeMillis(); - log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e.getCause()); - throw (CompilationFailedException) e.getCause(); - } else { - throw new AssertionError("Unexpected exception when compiling script", e); - } - } finally { - long time = System.currentTimeMillis() - start; - if (time > 5000) { - //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. - //Scripts with a large numbers of parameters often trigger this and should be avoided. - log.warn("Script compilation {} took {}ms", script, time); - } else { - log.debug("Script compilation {} took {}ms", script, time); - } + return classMap.get(script).get(); + } catch (ExecutionException e) { + throw ((CompilationFailedException)e.getCause()); + } catch (InterruptedException e) { + //This should never happen as the future should completed before it is returned to the us. + throw new AssertionError(); } + } boolean isCached(final String script) { - return classMap.get(script) != null; + return classMap.getIfPresent(script) != null; } Object eval(final Class scriptClass, final ScriptContext context) throws ScriptException { From 17a72e0e1db6fe52d3a0821ef2bfae1eb39be856 Mon Sep 17 00:00:00 2001 From: BrynCooke Date: Wed, 8 Mar 2017 13:23:30 +0000 Subject: [PATCH 04/19] TINKERPOP-1644 Fix exception handling. --- .../jsr223/GremlinGroovyScriptEngine.java | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 51850adb3bc..2151a82a076 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -168,26 +168,25 @@ protected Map initialValue() { @Override public Future load(String script) throws Exception { long start = System.currentTimeMillis(); - try { - return CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName()), command -> command.run()); - } catch (Exception e) { - if (e.getCause() instanceof CompilationFailedException) { + + return CompletableFuture.supplyAsync(() -> { + try { + return loader.parseClass(script, generateScriptName()); + } catch (CompilationFailedException e) { long finish = System.currentTimeMillis(); log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e.getCause()); - throw (CompilationFailedException) e.getCause(); - } else { - throw new AssertionError("Unexpected exception when compiling script", e); - } - } finally { - long time = System.currentTimeMillis() - start; - if (time > 5000) { - //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. - //Scripts with a large numbers of parameters often trigger this and should be avoided. - log.warn("Script compilation {} took {}ms", script, time); - } else { - log.debug("Script compilation {} took {}ms", script, time); + throw e; + } finally { + long time = System.currentTimeMillis() - start; + if (time > 5000) { + //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. + //Scripts with a large numbers of parameters often trigger this and should be avoided. + log.warn("Script compilation {} took {}ms", script, time); + } else { + log.debug("Script compilation {} took {}ms", script, time); + } } - } + }, command -> command.run()); } }); From 1df71c81e032daa9c9db6f626d99c39b37d434ce Mon Sep 17 00:00:00 2001 From: BrynCooke Date: Wed, 8 Mar 2017 13:26:31 +0000 Subject: [PATCH 05/19] TINKERPOP-1644 Fix exception handling. --- .../gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 2151a82a076..cadc3b095c6 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -174,7 +174,7 @@ public Future load(String script) throws Exception { return loader.parseClass(script, generateScriptName()); } catch (CompilationFailedException e) { long finish = System.currentTimeMillis(); - log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e.getCause()); + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e); throw e; } finally { long time = System.currentTimeMillis() - start; From 608f024f7bb83eb168a6aa637d46ee0650674903 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Wed, 8 Mar 2017 13:31:52 -0500 Subject: [PATCH 06/19] TINKERPOP-1644 Remove gremlin-server caffeine dependency Caffeine is now down in gremlin-groovy. --- gremlin-server/pom.xml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/gremlin-server/pom.xml b/gremlin-server/pom.xml index adb41fddf8e..7a4cad9ab00 100644 --- a/gremlin-server/pom.xml +++ b/gremlin-server/pom.xml @@ -55,11 +55,6 @@ limitations under the License. log4j true - - com.github.ben-manes.caffeine - caffeine - 2.3.1 - com.codahale.metrics From de1d58ab33c3121e9bafb5e4908f3e0d76c8e26e Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Wed, 8 Mar 2017 13:37:42 -0500 Subject: [PATCH 07/19] TINKERPOP-1644 Minor code format updates --- .../groovy/jsr223/GremlinGroovyScriptEngine.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index cadc3b095c6..42d954acf19 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -164,20 +164,20 @@ protected Map initialValue() { /** * Script to generated Class map. */ - private LoadingCache> classMap = Caffeine.newBuilder().softValues().build(new CacheLoader>() { + private final LoadingCache> classMap = Caffeine.newBuilder().softValues().build(new CacheLoader>() { @Override - public Future load(String script) throws Exception { - long start = System.currentTimeMillis(); + public Future load(final String script) throws Exception { + final long start = System.currentTimeMillis(); return CompletableFuture.supplyAsync(() -> { try { return loader.parseClass(script, generateScriptName()); } catch (CompilationFailedException e) { - long finish = System.currentTimeMillis(); + final long finish = System.currentTimeMillis(); log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e); throw e; } finally { - long time = System.currentTimeMillis() - start; + final long time = System.currentTimeMillis() - start; if (time > 5000) { //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. //Scripts with a large numbers of parameters often trigger this and should be avoided. @@ -186,7 +186,7 @@ public Future load(String script) throws Exception { log.debug("Script compilation {} took {}ms", script, time); } } - }, command -> command.run()); + }, Runnable::run); } }); From 4bdeac4b796b38fb14d1be763e03cc837b57d3d7 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Wed, 8 Mar 2017 16:13:06 -0500 Subject: [PATCH 08/19] TINKERPOP-1644 Provided configuration options for GremlinGroovyScriptEngine Introduced new customizers to pass in configuration options to the GremlinGroovyScriptEngine. --- .../jsr223/CompilationOptionsCustomizer.java | 39 +++++++++ .../jsr223/GremlinGroovyScriptEngine.java | 36 +++++++-- .../jsr223/GroovyCompilerGremlinPlugin.java | 13 +++ .../CompilationOptionsCustomizerProvider.java | 48 +++++++++++ ...ovyScriptEngineCompilationOptionsTest.java | 80 +++++++++++++++++++ .../GroovyCompilerGremlinPluginTest.java | 24 ++++-- 6 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java create mode 100644 gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java create mode 100644 gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java new file mode 100644 index 00000000000..2a22eb33c05 --- /dev/null +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.groovy.jsr223; + +import org.apache.tinkerpop.gremlin.jsr223.Customizer; + +/** + * Provides some custom compilation options to the {@link GremlinGroovyScriptEngine}. + * + * @author Stephen Mallette (http://stephen.genoprime.com) + */ +class CompilationOptionsCustomizer implements Customizer { + + private final int expectedCompilationTime; + + public CompilationOptionsCustomizer(final int expectedCompilationTime) { + this.expectedCompilationTime = expectedCompilationTime; + } + + public int getExpectedCompilationTime() { + return expectedCompilationTime; + } +} diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 42d954acf19..51681307102 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -35,6 +35,7 @@ import org.apache.tinkerpop.gremlin.groovy.EmptyImportCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.ImportCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.NoImportCustomizerProvider; +import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ConfigurationCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.InterpreterModeCustomizerProvider; import org.apache.tinkerpop.gremlin.groovy.loaders.GremlinLoader; @@ -80,11 +81,11 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; -import java.util.concurrent.Executor; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Pattern; @@ -125,7 +126,6 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl */ public static final String REFERENCE_TYPE_PHANTOM = "phantom"; - /** * A value to the {@link #KEY_REFERENCE_TYPE} that marks the script as one that can be garbage collected * even when memory is abundant. @@ -178,26 +178,26 @@ public Future load(final String script) throws Exception { throw e; } finally { final long time = System.currentTimeMillis() - start; - if (time > 5000) { + if (time > expectedCompilationTime) { //We warn if a script took longer than a few seconds. Repeatedly seeing these warnings is a sign that something is wrong. //Scripts with a large numbers of parameters often trigger this and should be avoided. log.warn("Script compilation {} took {}ms", script, time); + longRunCompilationCount.incrementAndGet(); } else { log.debug("Script compilation {} took {}ms", script, time); } } }, Runnable::run); } - }); /** * Global closures map - this is used to simulate a single global functions namespace */ - private ManagedConcurrentValueMap globalClosures = new ManagedConcurrentValueMap<>(ReferenceBundle.getHardBundle()); - + private final ManagedConcurrentValueMap globalClosures = new ManagedConcurrentValueMap<>(ReferenceBundle.getHardBundle()); - private AtomicLong counter = new AtomicLong(0L); + private final AtomicLong counter = new AtomicLong(0L); + private final AtomicLong longRunCompilationCount = new AtomicLong(0L); /** * The list of loaded plugins for the console. @@ -219,6 +219,7 @@ public Future load(final String script) throws Exception { private final Set artifactsToUse = new HashSet<>(); private final boolean interpreterModeEnabled; + private final int expectedCompilationTime; /** * Creates a new instance using the {@link DefaultImportCustomizerProvider}. @@ -255,6 +256,12 @@ public GremlinGroovyScriptEngine(final Customizer... customizers) { .map(p -> ((GroovyCustomizer) p)) .collect(Collectors.toList()); + final Optional compilationOptionsCustomizerProvider = listOfCustomizers.stream() + .filter(p -> p instanceof CompilationOptionsCustomizer) + .map(p -> (CompilationOptionsCustomizer) p).findFirst(); + expectedCompilationTime = compilationOptionsCustomizerProvider.isPresent() ? + compilationOptionsCustomizerProvider.get().getExpectedCompilationTime() : 5000; + // determine if interpreter mode should be enabled interpreterModeEnabled = groovyCustomizers.stream() .anyMatch(p -> p.getClass().equals(InterpreterModeGroovyCustomizer.class)); @@ -286,9 +293,15 @@ public GremlinGroovyScriptEngine(final CompilerCustomizerProvider... compilerCus interpreterModeEnabled = providers.stream() .anyMatch(p -> p.getClass().equals(InterpreterModeCustomizerProvider.class)); + final Optional compilationOptionsCustomizerProvider = providers.stream() + .filter(p -> p instanceof CompilationOptionsCustomizerProvider) + .map(p -> (CompilationOptionsCustomizerProvider) p).findFirst(); + expectedCompilationTime = compilationOptionsCustomizerProvider.isPresent() ? + compilationOptionsCustomizerProvider.get().getExpectedCompilationTime() : 5000; + // remove used providers as the rest will be applied directly customizerProviders = providers.stream() - .filter(p -> p != null && !(p instanceof ImportCustomizerProvider)) + .filter(p -> p != null && !(p instanceof ImportCustomizerProvider) && !(p instanceof CompilationOptionsCustomizerProvider)) .collect(Collectors.toList()); // groovy customizers are not used here - set to empty list so that the customizerProviders get used @@ -565,6 +578,13 @@ public T getInterface(final Object thiz, final Class clazz) { return makeInterface(thiz, clazz); } + /** + * Gets the number of compilations that extended beyond the {@link #expectedCompilationTime}. + */ + public long getLongRunCompilationCount() { + return longRunCompilationCount.longValue(); + } + Class getScriptClass(final String script) throws CompilationFailedException { try { return classMap.get(script).get(); diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPlugin.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPlugin.java index 71f5dc71f43..5680a4ff027 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPlugin.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPlugin.java @@ -59,9 +59,19 @@ public static final class Builder { private long timeInMillis = 0; private Compilation compilation = Compilation.NONE; private String extensions = null; + private int expectedCompilationTime = 5000; private Map keyValues = Collections.emptyMap(); + /** + * If the time it takes to compile a script exceeds the specified time then a warning is written to the logs. + * Defaults to 5000ms. + */ + public Builder expectedCompilationTime(final int timeInMillis) { + this.expectedCompilationTime = timeInMillis; + return this; + } + public Builder enableInterpreterMode(final boolean interpreterMode) { this.interpreterMode = interpreterMode; return this; @@ -111,6 +121,9 @@ Customizer[] asCustomizers() { if (timeInMillis > 0) list.add(new TimedInterruptGroovyCustomizer(timeInMillis)); + if (expectedCompilationTime > 0) + list.add(new CompilationOptionsCustomizer(expectedCompilationTime)); + if (compilation == Compilation.COMPILE_STATIC) list.add(new CompileStaticGroovyCustomizer(extensions)); else if (compilation == Compilation.TYPE_CHECKED) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java new file mode 100644 index 00000000000..047551148c3 --- /dev/null +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.groovy.jsr223.customizer; + +import org.apache.tinkerpop.gremlin.groovy.CompilerCustomizerProvider; +import org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine; +import org.codehaus.groovy.control.customizers.CompilationCustomizer; + +/** + * Provides compilation configuration options to the {@link GremlinGroovyScriptEngine}. + * + * @author Stephen Mallette (http://stephen.genoprime.com) + * @deprecated As of release 3.2.5, not replaced by a public class. + */ +@Deprecated +public class CompilationOptionsCustomizerProvider implements CompilerCustomizerProvider { + + private final int expectedCompilationTime; + + public CompilationOptionsCustomizerProvider(final int expectedCompilationTime) { + this.expectedCompilationTime = expectedCompilationTime; + } + + public int getExpectedCompilationTime() { + return expectedCompilationTime; + } + + @Override + public CompilationCustomizer create() { + throw new UnsupportedOperationException("This is a marker implementation that does not create a CompilationCustomizer instance"); + } +} diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java new file mode 100644 index 00000000000..f5dd9c56efd --- /dev/null +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.groovy.jsr223; + +import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider; +import org.junit.Test; + +import javax.script.Bindings; +import javax.script.SimpleBindings; + +import static org.junit.Assert.assertEquals; + +/** + * @author Stephen Mallette (http://stephen.genoprime.com) + */ +public class GremlinGroovyScriptEngineCompilationOptionsTest { + @Test + public void shouldRegisterLongCompilationDeprecated() throws Exception { + final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine(new CompilationOptionsCustomizerProvider(10)); + + final int numberOfParameters = 3000; + final Bindings b = new SimpleBindings(); + + // generate a script that takes a long time to compile + String script = "x = 0"; + for (int ix = 0; ix < numberOfParameters; ix++) { + if (ix > 0 && ix % 100 == 0) { + script = script + ";" + System.lineSeparator() + "x = x"; + } + script = script + " + x" + ix; + b.put("x" + ix, ix); + } + + assertEquals(0, engine.getLongRunCompilationCount()); + + engine.eval(script, b); + + assertEquals(1, engine.getLongRunCompilationCount()); + } + + @Test + public void shouldRegisterLongCompilation() throws Exception { + final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine(new CompilationOptionsCustomizer(10)); + + final int numberOfParameters = 3000; + final Bindings b = new SimpleBindings(); + + // generate a script that takes a long time to compile + String script = "x = 0"; + for (int ix = 0; ix < numberOfParameters; ix++) { + if (ix > 0 && ix % 100 == 0) { + script = script + ";" + System.lineSeparator() + "x = x"; + } + script = script + " + x" + ix; + b.put("x" + ix, ix); + } + + assertEquals(0, engine.getLongRunCompilationCount()); + + engine.eval(script, b); + + assertEquals(1, engine.getLongRunCompilationCount()); + } +} diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java index f795fa76167..bacfc907534 100644 --- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java @@ -18,12 +18,6 @@ */ package org.apache.tinkerpop.gremlin.groovy.jsr223; -import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompileStaticCustomizerProvider; -import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ConfigurationCustomizerProvider; -import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.InterpreterModeCustomizerProvider; -import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ThreadInterruptCustomizerProvider; -import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.TimedInterruptCustomizerProvider; -import org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.TypeCheckedCustomizerProvider; import org.apache.tinkerpop.gremlin.jsr223.Customizer; import org.codehaus.groovy.control.CompilerConfiguration; import org.junit.Test; @@ -54,6 +48,7 @@ public void shouldConfigureForGroovyOnly() { @Test public void shouldConfigureWithCompileStatic() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(0). compilation(Compilation.COMPILE_STATIC).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); @@ -64,6 +59,7 @@ public void shouldConfigureWithCompileStatic() { @Test public void shouldConfigureWithTypeChecked() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(0). compilation(Compilation.TYPE_CHECKED).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); @@ -76,6 +72,7 @@ public void shouldConfigureWithCustomCompilerConfigurations() { final Map conf = new HashMap<>(); conf.put("Debug", true); final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(0). compilerConfigurationOptions(conf).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); @@ -94,6 +91,7 @@ public void shouldConfigureWithCustomCompilerConfigurations() { @Test public void shouldConfigureWithInterpreterMode() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(0). enableInterpreterMode(true).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); @@ -104,6 +102,7 @@ public void shouldConfigureWithInterpreterMode() { @Test public void shouldConfigureWithThreadInterrupt() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(0). enableThreadInterrupt(true).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); @@ -114,6 +113,7 @@ public void shouldConfigureWithThreadInterrupt() { @Test public void shouldConfigureWithTimedInterrupt() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(0). timedInterrupt(60000).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); @@ -121,8 +121,18 @@ public void shouldConfigureWithTimedInterrupt() { assertThat(customizers.get()[0], instanceOf(TimedInterruptGroovyCustomizer.class)); } + @Test + public void shouldConfigureWithCompilationOptions() { + final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). + expectedCompilationTime(30000).create(); + final Optional customizers = plugin.getCustomizers("gremlin-groovy"); + assertThat(customizers.isPresent(), is(true)); + assertEquals(1, customizers.get().length); + assertThat(customizers.get()[0], instanceOf(CompilationOptionsCustomizer.class)); + } + @Test(expected = IllegalStateException.class) public void shouldNotConfigureIfNoSettingsAreSupplied() { - GroovyCompilerGremlinPlugin.build().create(); + GroovyCompilerGremlinPlugin.build().expectedCompilationTime(0).create(); } } From b29ba12109e7e88bc3465d08f6177e5ded17ca59 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Wed, 8 Mar 2017 16:15:27 -0500 Subject: [PATCH 09/19] TINKERPOP-1644 Updated changelog --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 24e5e9ce305..9c3313e5d05 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Moved the `caffeine` dependency down to `gremlin-groovy` and out of `gremlin-server`. * Script compilation is synchronised. * Script compilation times are placed in to logs. * Failed scripts will not be recompiled. From a06072b0502f9b42fee4c68dba554b4991406c36 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Wed, 8 Mar 2017 16:17:40 -0500 Subject: [PATCH 10/19] TINKERPOP-1644 Made the "counter" in GremlinGroovyScriptEngine static It seems that this field should be static and shared across all instances as the script name seems to share space with other GroovyClassLoaders. Not sure what would happen in the case of collision, but there doesn't seem to be much harm in ensuring better uniqueness. This counter wasn't used for tracking the number of scripts actually processed or anything so it should be ok to make this change without fear of breaking anything. --- .../groovy/jsr223/GremlinGroovyScriptEngine.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 51681307102..3bcb06c0ca5 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -196,7 +196,15 @@ public Future load(final String script) throws Exception { */ private final ManagedConcurrentValueMap globalClosures = new ManagedConcurrentValueMap<>(ReferenceBundle.getHardBundle()); - private final AtomicLong counter = new AtomicLong(0L); + /** + * Ensures unique script names across all instances. + */ + private final static AtomicLong scriptNameCounter = new AtomicLong(0L); + + /** + * A counter for the instance that tracks the number of warnings issued during script compilations that exceeded + * the {@link #expectedCompilationTime}. + */ private final AtomicLong longRunCompilationCount = new AtomicLong(0L); /** @@ -799,7 +807,7 @@ private Object callGlobal(final String name, final Object args[], final ScriptCo } private synchronized String generateScriptName() { - return SCRIPT + counter.incrementAndGet() + DOT_GROOVY; + return SCRIPT + scriptNameCounter.incrementAndGet() + DOT_GROOVY; } @SuppressWarnings("unchecked") From 8ffa5af6ff56933c1955e88e8aa42c06cb69162b Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 9 Mar 2017 10:30:17 -0500 Subject: [PATCH 11/19] TINKERPOP-1644 Added metrics to GremlinGroovyScriptEngine --- CHANGELOG.asciidoc | 2 + .../jsr223/GremlinGroovyScriptEngine.java | 122 +++++++++++++++++- ...ovyScriptEngineCompilationOptionsTest.java | 8 +- .../jsr223/GremlinGroovyScriptEngineTest.java | 70 +++++++++- 4 files changed, 193 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 9c3313e5d05..37292710ed1 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,7 +26,9 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added various metrics to the `GremlinGroovyScriptEngine` around script compilation. * Moved the `caffeine` dependency down to `gremlin-groovy` and out of `gremlin-server`. +* Improved script compilation in `GremlinGroovyScriptEngine to use better caching, log long compile times and prevent failed compilations from recompiling on future requests. * Script compilation is synchronised. * Script compilation times are placed in to logs. * Failed scripts will not be recompiled. diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 3bcb06c0ca5..0ed8d84cbed 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -164,7 +164,10 @@ protected Map initialValue() { /** * Script to generated Class map. */ - private final LoadingCache> classMap = Caffeine.newBuilder().softValues().build(new CacheLoader>() { + private final LoadingCache> classMap = Caffeine.newBuilder(). + softValues(). + recordStats(). + build(new CacheLoader>() { @Override public Future load(final String script) throws Exception { final long start = System.currentTimeMillis(); @@ -175,6 +178,7 @@ public Future load(final String script) throws Exception { } catch (CompilationFailedException e) { final long finish = System.currentTimeMillis(); log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e); + failedCompilationCount.incrementAndGet(); throw e; } finally { final long time = System.currentTimeMillis() - start; @@ -207,6 +211,13 @@ public Future load(final String script) throws Exception { */ private final AtomicLong longRunCompilationCount = new AtomicLong(0L); + /** + * A counter for the instance that tracks the number of failed compilations. Note that the failures need to be + * tracked outside of cache failure load stats because the loading mechanism will always successfully return + * a future and won't trigger a failure. + */ + private final AtomicLong failedCompilationCount = new AtomicLong(0L); + /** * The list of loaded plugins for the console. */ @@ -589,10 +600,117 @@ public T getInterface(final Object thiz, final Class clazz) { /** * Gets the number of compilations that extended beyond the {@link #expectedCompilationTime}. */ - public long getLongRunCompilationCount() { + public long getClassCacheLongRunCompilationCount() { return longRunCompilationCount.longValue(); } + /** + * Gets the estimated size of the class cache for compiled scripts. + */ + public long getClassCacheEstimatedSize() { + return classMap.estimatedSize(); + } + + /** + * Gets the average time spent compiling new scripts. + */ + public double getClassCacheAverageLoadPenalty() { + return classMap.stats().averageLoadPenalty(); + } + + /** + * Gets the number of times a script compiled to a class has been evicted from the cache. + */ + public long getClassCacheEvictionCount() { + return classMap.stats().evictionCount(); + } + + /** + * Gets the sum of the weights of evicted entries from the class cache. + */ + public long getClassCacheEvictionWeight() { + return classMap.stats().evictionWeight(); + } + + /** + * Gets the number of times cache look up for a compiled script returned a cached value. + */ + public long getClassCacheHitCount() { + return classMap.stats().hitCount(); + } + + /** + * Gets the hit rate of the class cache. + */ + public double getClassCacheHitRate() { + return classMap.stats().hitRate(); + } + + /** + * Gets the total number of times the cache lookup method attempted to compile new scripts. + */ + public long getClassCacheLoadCount() { + return classMap.stats().loadCount(); + } + + /** + * Gets the total number of times the cache lookup method failed to compile a new script. + */ + public long getClassCacheLoadFailureCount() { + // don't use classMap.stats().loadFailureCount() because the load mechanism always succeeds with a future + // that will in turn contain the success or failure + return failedCompilationCount.longValue(); + } + + /** + * Gets the ratio of script compilation attempts that failed. + */ + public double getClassCacheLoadFailureRate() { + // don't use classMap.stats().loadFailureRate() because the load mechanism always succeeds with a future + // that will in turn contain the success or failure + long totalLoadCount = classMap.stats().loadCount(); + return (totalLoadCount == 0) + ? 0.0 + : (double) failedCompilationCount.longValue() / totalLoadCount; + } + + /** + * Gets the total number of times the cache lookup method succeeded to compile a new script. + */ + public long getClassCacheLoadSuccessCount() { + // don't use classMap.stats().loadSuccessCount() because the load mechanism always succeeds with a future + // that will in turn contain the success or failure + return classMap.stats().loadCount() - failedCompilationCount.longValue(); + } + + /** + * Gets the total number of times the cache lookup method returned a newly compiled script. + */ + public long getClassCacheMissCount() { + return classMap.stats().missCount(); + } + + /** + * Gets the ratio of script compilation attempts that were misses. + */ + public double getClassCacheMissRate() { + return classMap.stats().missRate(); + } + + /** + * Gets the total number of times the cache lookup method returned a cached or uncached value. + */ + public long getClassCacheRequestCount() { + return classMap.stats().missCount(); + } + + /** + * Gets the total number of nanoseconds that the cache spent compiling scripts. + */ + public long getClassCacheTotalLoadTime() { + return classMap.stats().totalLoadTime(); + } + Class getScriptClass(final String script) throws CompilationFailedException { try { return classMap.get(script).get(); diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java index f5dd9c56efd..b87d0acfc5f 100644 --- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineCompilationOptionsTest.java @@ -47,11 +47,11 @@ public void shouldRegisterLongCompilationDeprecated() throws Exception { b.put("x" + ix, ix); } - assertEquals(0, engine.getLongRunCompilationCount()); + assertEquals(0, engine.getClassCacheLongRunCompilationCount()); engine.eval(script, b); - assertEquals(1, engine.getLongRunCompilationCount()); + assertEquals(1, engine.getClassCacheLongRunCompilationCount()); } @Test @@ -71,10 +71,10 @@ public void shouldRegisterLongCompilation() throws Exception { b.put("x" + ix, ix); } - assertEquals(0, engine.getLongRunCompilationCount()); + assertEquals(0, engine.getClassCacheLongRunCompilationCount()); engine.eval(script, b); - assertEquals(1, engine.getLongRunCompilationCount()); + assertEquals(1, engine.getClassCacheLongRunCompilationCount()); } } diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java index eb0a44b701c..0606721158f 100644 --- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java @@ -497,7 +497,7 @@ public void shouldInvokeFunctionRedirectsOutputToContextWriter() throws Exceptio } @Test - public void testInvokeFunctionRedirectsOutputToContextOut() throws Exception { + public void shouldInvokeFunctionRedirectsOutputToContextOut() throws Exception { final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine(); StringWriter writer = new StringWriter(); final StringWriter unusedWriter = new StringWriter(); @@ -520,7 +520,7 @@ public void testInvokeFunctionRedirectsOutputToContextOut() throws Exception { } @Test - public void testEngineContextAccessibleToScript() throws Exception { + public void shouldEnableEngineContextAccessibleToScript() throws Exception { final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine(); final ScriptContext engineContext = engine.getContext(); engine.put("theEngineContext", engineContext); @@ -529,7 +529,7 @@ public void testEngineContextAccessibleToScript() throws Exception { } @Test - public void testContextBindingOverridesEngineContext() throws Exception { + public void shouldEnableContextBindingOverridesEngineContext() throws Exception { final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine(); final ScriptContext engineContext = engine.getContext(); final Map otherContext = new HashMap<>(); @@ -539,4 +539,68 @@ public void testContextBindingOverridesEngineContext() throws Exception { final String code = "[answer: context.is(theEngineContext) ? \"wrong\" : context.foo]"; assertEquals("bar", ((Map) engine.eval(code)).get("answer")); } + + @Test + public void shouldGetClassMapCacheBasicStats() throws Exception { + final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine(); + assertEquals(0, engine.getClassCacheEstimatedSize()); + assertEquals(0, engine.getClassCacheHitCount()); + assertEquals(0, engine.getClassCacheLoadCount()); + assertEquals(0, engine.getClassCacheLoadFailureCount()); + assertEquals(0, engine.getClassCacheLoadSuccessCount()); + + engine.eval("1+1"); + + assertEquals(1, engine.getClassCacheEstimatedSize()); + assertEquals(0, engine.getClassCacheHitCount()); + assertEquals(1, engine.getClassCacheLoadCount()); + assertEquals(0, engine.getClassCacheLoadFailureCount()); + assertEquals(1, engine.getClassCacheLoadSuccessCount()); + + for (int ix = 0; ix < 100; ix++) { + engine.eval("1+1"); + } + + assertEquals(1, engine.getClassCacheEstimatedSize()); + assertEquals(100, engine.getClassCacheHitCount()); + assertEquals(1, engine.getClassCacheLoadCount()); + assertEquals(0, engine.getClassCacheLoadFailureCount()); + assertEquals(1, engine.getClassCacheLoadSuccessCount()); + + for (int ix = 0; ix < 100; ix++) { + engine.eval("1+" + ix); + } + + assertEquals(100, engine.getClassCacheEstimatedSize()); + assertEquals(101, engine.getClassCacheHitCount()); + assertEquals(100, engine.getClassCacheLoadCount()); + assertEquals(0, engine.getClassCacheLoadFailureCount()); + assertEquals(100, engine.getClassCacheLoadSuccessCount()); + + try { + engine.eval("(me broken"); + fail("Should have tanked with compilation error"); + } catch (Exception ex) { + assertThat(ex, instanceOf(ScriptException.class)); + } + + assertEquals(101, engine.getClassCacheEstimatedSize()); + assertEquals(101, engine.getClassCacheHitCount()); + assertEquals(101, engine.getClassCacheLoadCount()); + assertEquals(1, engine.getClassCacheLoadFailureCount()); + assertEquals(100, engine.getClassCacheLoadSuccessCount()); + + try { + engine.eval("(me broken"); + fail("Should have tanked with compilation error"); + } catch (Exception ex) { + assertThat(ex, instanceOf(ScriptException.class)); + } + + assertEquals(101, engine.getClassCacheEstimatedSize()); + assertEquals(102, engine.getClassCacheHitCount()); + assertEquals(101, engine.getClassCacheLoadCount()); + assertEquals(1, engine.getClassCacheLoadFailureCount()); + assertEquals(100, engine.getClassCacheLoadSuccessCount()); + } } \ No newline at end of file From b689deb1dd1a0c745966a1ed5f8e7e3b2d543af2 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 9 Mar 2017 12:25:35 -0500 Subject: [PATCH 12/19] TINKERPOP-1644 Exposed GremlinScriptEngine metrics in Gremlin Server --- CHANGELOG.asciidoc | 2 +- .../gremlin/groovy/engine/ScriptEngines.java | 7 ++ .../gremlin/server/op/session/Session.java | 27 +++++++ .../gremlin/server/util/MetricManager.java | 79 ++++++++++++++++--- .../server/util/ServerGremlinExecutor.java | 15 ++++ 5 files changed, 118 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 37292710ed1..a306dead283 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,7 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -* Added various metrics to the `GremlinGroovyScriptEngine` around script compilation. +* Added various metrics to the `GremlinGroovyScriptEngine` around script compilation and exposed them in Gremlin Server. * Moved the `caffeine` dependency down to `gremlin-groovy` and out of `gremlin-server`. * Improved script compilation in `GremlinGroovyScriptEngine to use better caching, log long compile times and prevent failed compilations from recompiling on future requests. * Script compilation is synchronised. diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/ScriptEngines.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/ScriptEngines.java index 7dcfc5cf8ce..6dfd87a657d 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/ScriptEngines.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/ScriptEngines.java @@ -310,6 +310,13 @@ public Map> imports() { return m; } + /** + * Gets an {@link GremlinScriptEngine} by its name. + */ + public GremlinScriptEngine getEngineByName(final String engineName) { + return scriptEngines.get(engineName); + } + /** * Get the set of {@code ScriptEngine} that implement {@link DependencyManager} interface. */ diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java index 9b16a3b4caa..b8cb28c11e0 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java @@ -18,11 +18,16 @@ */ package org.apache.tinkerpop.gremlin.server.op.session; +import com.codahale.metrics.Metric; +import com.codahale.metrics.MetricFilter; import org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor; +import org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines; +import org.apache.tinkerpop.gremlin.jsr223.GremlinScriptEngine; import org.apache.tinkerpop.gremlin.server.Context; import org.apache.tinkerpop.gremlin.server.GraphManager; import org.apache.tinkerpop.gremlin.server.Settings; import org.apache.tinkerpop.gremlin.server.util.LifeCycleHook; +import org.apache.tinkerpop.gremlin.server.util.MetricManager; import org.apache.tinkerpop.gremlin.server.util.ThreadFactoryUtil; import org.apache.tinkerpop.gremlin.structure.Graph; import org.slf4j.Logger; @@ -30,6 +35,7 @@ import javax.script.Bindings; import javax.script.SimpleBindings; +import java.util.Collections; import java.util.HashSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; @@ -92,6 +98,15 @@ public Session(final String session, final Context context, final ConcurrentHash SessionOpProcessor.CONFIG_PER_GRAPH_CLOSE_TIMEOUT, SessionOpProcessor.DEFAULT_PER_GRAPH_CLOSE_TIMEOUT).toString()); this.gremlinExecutor = initializeGremlinExecutor().create(); + + settings.scriptEngines.keySet().forEach(engineName -> { + try { + gremlinExecutor.eval("1+1", engineName, Collections.emptyMap()).join(); + registerMetrics(engineName); + } catch (Exception ex) { + logger.warn(String.format("Could not initialize {} ScriptEngine as script could not be evaluated - %s", engineName), ex); + } + }); } public GremlinExecutor getGremlinExecutor() { @@ -207,6 +222,10 @@ public synchronized void kill(final boolean force) { executor.shutdownNow(); sessions.remove(session); + + // once a session is dead release the gauges in the registry for it + MetricManager.INSTANCE.getRegistry().removeMatching((s, metric) -> s.contains(session)); + logger.info("Session {} closed", session); } @@ -244,4 +263,12 @@ private GremlinExecutor.Builder initializeGremlinExecutor() { return gremlinExecutorBuilder; } + + private void registerMetrics(final String engineName) { + final ScriptEngines scriptEngines = gremlinExecutor.getScriptEngines(); + final GremlinScriptEngine engine = null == scriptEngines ? + gremlinExecutor.getScriptEngineManager().getEngineByName(engineName) : + scriptEngines.getEngineByName(engineName); + MetricManager.INSTANCE.registerGremlinScriptEngineMetrics(engine, "session", session, "class-cache"); + } } diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java index 2867aa752d6..06179d25ca3 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java @@ -33,6 +33,9 @@ import com.codahale.metrics.graphite.Graphite; import com.codahale.metrics.graphite.GraphiteReporter; import info.ganglia.gmetric4j.gmetric.GMetric; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine; +import org.apache.tinkerpop.gremlin.jsr223.GremlinScriptEngine; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -66,17 +69,16 @@ public enum MetricManager { private GraphiteReporter graphiteReporter = null; /** - * Return the Titan Metrics registry. + * Return the {@code MetricsRegistry}. * - * @return the single {@code MetricRegistry} used for all of Titan's Metrics - * monitoring + * @return the single {@code MetricRegistry} used for all of monitoring */ public MetricRegistry getRegistry() { return registry; } /** - * Create a {@link ConsoleReporter} attached to the Titan Metrics registry. + * Create a {@link ConsoleReporter} attached to the {@code MetricsRegistry}. * * @param reportIntervalInMS milliseconds to wait between dumping metrics to the console */ @@ -106,7 +108,7 @@ public synchronized void removeConsoleReporter() { } /** - * Create a {@link CsvReporter} attached to the Titan Metrics registry. + * Create a {@link CsvReporter} attached to the {@code MetricsRegistry}. *

* The {@code output} argument must be non-null but need not exist. If it * doesn't already exist, this method attempts to create it by calling @@ -153,7 +155,7 @@ public synchronized void removeCsvReporter() { } /** - * Create a {@link JmxReporter} attached to the Titan Metrics registry. + * Create a {@link JmxReporter} attached to the {@code MetricsRegistry}. *

* If {@code domain} or {@code agentId} is null, then Metrics's uses its own * internal default value(s). @@ -210,7 +212,7 @@ public synchronized void removeJmxReporter() { } /** - * Create a {@link Slf4jReporter} attached to the Titan Metrics registry. + * Create a {@link Slf4jReporter} attached to the {@code MetricsRegistry}. *

* If {@code loggerName} is null, or if it is non-null but * LoggerFactory.getLogger(loggerName) returns null, then Metrics's @@ -257,7 +259,7 @@ public synchronized void removeSlf4jReporter() { } /** - * Create a {@link GangliaReporter} attached to the Titan Metrics registry. + * Create a {@link GangliaReporter} attached to the {@code MetricsRegistry}. *

* {@code groupOrHost} and {@code addressingMode} must be non-null. The * remaining non-primitive arguments may be null. If {@code protocol31} is @@ -321,7 +323,7 @@ public synchronized void removeGangliaReporter() { } /** - * Create a {@link GraphiteReporter} attached to the Titan Metrics registry. + * Create a {@link GraphiteReporter} attached to the {@code MetricsRegistry}. *

* If {@code prefix} is null, then Metrics's internal default prefix is used * (empty string at the time this comment was written). @@ -367,8 +369,7 @@ public synchronized void removeGraphiteReporter() { } /** - * Remove all Titan Metrics reporters previously configured through the - * {@code add*} methods on this class. + * Remove all reporters previously configured through the {@code add*} methods on this class. */ public synchronized void removeAllReporters() { removeConsoleReporter(); @@ -418,4 +419,60 @@ public Histogram getHistogram(final String name) { public Histogram getHistogram(final String prefix, final String... names) { return getRegistry().histogram(MetricRegistry.name(prefix, names)); } + + /** + * Registers metrics from a {@link GremlinScriptEngine}. At this point, this only works for the + * {@link GremlinGroovyScriptEngine} as it is the only one that collects metrics at this point. As the + * {@link GremlinScriptEngine} implementations achieve greater parity these metrics will get expanded. + */ + public void registerGremlinScriptEngineMetrics(final GremlinScriptEngine engine, final String... prefix) { + if (engine instanceof GremlinGroovyScriptEngine) { + final GremlinGroovyScriptEngine gremlinGroovyScriptEngine = (GremlinGroovyScriptEngine) engine; + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "long-run-compilation-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheLongRunCompilationCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "estimated-size")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheEstimatedSize); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "average-load-penalty")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheAverageLoadPenalty); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "eviction-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheEvictionCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "eviction-weight")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheEvictionWeight); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "hit-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheHitCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "hit-rate")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheHitRate); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-failure-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadFailureCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-failure-rate")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadFailureRate); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-success-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadSuccessCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "miss-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheMissCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "miss-rate")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheMissRate); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "request-count")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheRequestCount); + MetricManager.INSTANCE.getRegistry().register( + MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "total-load-time")), + (Gauge) gremlinGroovyScriptEngine::getClassCacheTotalLoadTime); + } + } } diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java index b9b728058f5..d66e0e883bf 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java @@ -18,7 +18,11 @@ */ package org.apache.tinkerpop.gremlin.server.util; +import com.codahale.metrics.Gauge; import org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor; +import org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines; +import org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine; +import org.apache.tinkerpop.gremlin.jsr223.GremlinScriptEngine; import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource; import org.apache.tinkerpop.gremlin.server.Channelizer; import org.apache.tinkerpop.gremlin.server.GraphManager; @@ -39,6 +43,8 @@ import java.util.concurrent.ThreadFactory; import java.util.stream.Collectors; +import static com.codahale.metrics.MetricRegistry.name; + /** * The core of script execution in Gremlin Server. Given {@link Settings} and optionally other arguments, this * class will construct a {@link GremlinExecutor} to be used by Gremlin Server. A typical usage would be to @@ -152,6 +158,7 @@ public ServerGremlinExecutor(final Settings settings, final ExecutorService grem settings.scriptEngines.keySet().forEach(engineName -> { try { gremlinExecutor.eval("1+1", engineName, Collections.emptyMap()).join(); + registerMetrics(engineName); } catch (Exception ex) { logger.warn(String.format("Could not initialize {} ScriptEngine as script could not be evaluated - %s", engineName), ex); } @@ -179,6 +186,14 @@ public ServerGremlinExecutor(final Settings settings, final ExecutorService grem .collect(Collectors.toList()); } + private void registerMetrics(final String engineName) { + final ScriptEngines scriptEngines = gremlinExecutor.getScriptEngines(); + final GremlinScriptEngine engine = null == scriptEngines ? + gremlinExecutor.getScriptEngineManager().getEngineByName(engineName) : + scriptEngines.getEngineByName(engineName); + MetricManager.INSTANCE.registerGremlinScriptEngineMetrics(engine, "sessionless", "class-cache"); + } + public void addHostOption(final String key, final Object value) { hostOptions.put(key, value); } From 9eb248ef0f0f29e8cf11cd5391ef8d993e37f7af Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 9 Mar 2017 13:15:04 -0500 Subject: [PATCH 13/19] TINKERPOP-1644 Renamed metrics for GremlinScriptEngine instances. Included the name of the GremlinScriptEngine and prefixed each metric with GremlinServer class name to be consistent with other metrics. --- .../gremlin/server/op/session/Session.java | 2 +- .../gremlin/server/util/MetricManager.java | 31 ++++++++++--------- .../server/util/ServerGremlinExecutor.java | 2 +- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java index b8cb28c11e0..6961339be25 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/session/Session.java @@ -269,6 +269,6 @@ private void registerMetrics(final String engineName) { final GremlinScriptEngine engine = null == scriptEngines ? gremlinExecutor.getScriptEngineManager().getEngineByName(engineName) : scriptEngines.getEngineByName(engineName); - MetricManager.INSTANCE.registerGremlinScriptEngineMetrics(engine, "session", session, "class-cache"); + MetricManager.INSTANCE.registerGremlinScriptEngineMetrics(engine, engineName, "session", session, "class-cache"); } } diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java index 06179d25ca3..faec502c223 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/MetricManager.java @@ -36,6 +36,7 @@ import org.apache.commons.lang3.ArrayUtils; import org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine; import org.apache.tinkerpop.gremlin.jsr223.GremlinScriptEngine; +import org.apache.tinkerpop.gremlin.server.GremlinServer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -429,49 +430,49 @@ public void registerGremlinScriptEngineMetrics(final GremlinScriptEngine engine, if (engine instanceof GremlinGroovyScriptEngine) { final GremlinGroovyScriptEngine gremlinGroovyScriptEngine = (GremlinGroovyScriptEngine) engine; MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "long-run-compilation-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "long-run-compilation-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheLongRunCompilationCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "estimated-size")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "estimated-size")), (Gauge) gremlinGroovyScriptEngine::getClassCacheEstimatedSize); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "average-load-penalty")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "average-load-penalty")), (Gauge) gremlinGroovyScriptEngine::getClassCacheAverageLoadPenalty); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "eviction-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "eviction-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheEvictionCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "eviction-weight")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "eviction-weight")), (Gauge) gremlinGroovyScriptEngine::getClassCacheEvictionWeight); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "hit-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "hit-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheHitCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "hit-rate")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "hit-rate")), (Gauge) gremlinGroovyScriptEngine::getClassCacheHitRate); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "load-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-failure-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "load-failure-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadFailureCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-failure-rate")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "load-failure-rate")), (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadFailureRate); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "load-success-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "load-success-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheLoadSuccessCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "miss-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "miss-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheMissCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "miss-rate")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "miss-rate")), (Gauge) gremlinGroovyScriptEngine::getClassCacheMissRate); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "request-count")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "request-count")), (Gauge) gremlinGroovyScriptEngine::getClassCacheRequestCount); MetricManager.INSTANCE.getRegistry().register( - MetricRegistry.name(GremlinGroovyScriptEngine.class, ArrayUtils.add(prefix, "total-load-time")), + MetricRegistry.name(GremlinServer.class, ArrayUtils.add(prefix, "total-load-time")), (Gauge) gremlinGroovyScriptEngine::getClassCacheTotalLoadTime); } } diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java index d66e0e883bf..e3c0cae8b7c 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java @@ -191,7 +191,7 @@ private void registerMetrics(final String engineName) { final GremlinScriptEngine engine = null == scriptEngines ? gremlinExecutor.getScriptEngineManager().getEngineByName(engineName) : scriptEngines.getEngineByName(engineName); - MetricManager.INSTANCE.registerGremlinScriptEngineMetrics(engine, "sessionless", "class-cache"); + MetricManager.INSTANCE.registerGremlinScriptEngineMetrics(engine, engineName, "sessionless", "class-cache"); } public void addHostOption(final String key, final Object value) { From 37976526f1eac9fd06858f962f979aa98d3e5346 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 9 Mar 2017 13:35:44 -0500 Subject: [PATCH 14/19] TINKERPOP-1644 Docs for new metrics in Gremlin Server --- docs/src/reference/gremlin-applications.asciidoc | 5 +++++ docs/src/upgrade/release-3.2.x-incubating.asciidoc | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc index 8e0edc2d032..37244e053e5 100644 --- a/docs/src/reference/gremlin-applications.asciidoc +++ b/docs/src/reference/gremlin-applications.asciidoc @@ -1560,6 +1560,11 @@ and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th a * `op.traversal` - the number of `Traveral` executions, mean rate, 1, 5, and 15 minute rates, minimum, maximum, median, mean, and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th and 99.9th percentile evaluation times. +* `engine-name.session.session-id.*` - metrics related to different `GremlinScriptEngine` instanc configured for +session-based requests where "engine-name" will be the actual name of the engine, such as "gremlin-groovy" and +"session-id" will be the identifier for the session itself. +* `engine-name.sessionless.*` - metrics related to different `GremlinScriptEngine` instanc configured for sessionless +requests where "engine-name" will be the actual name of the engine, such as "gremlin-groovy". Best Practices ~~~~~~~~~~~~~~ diff --git a/docs/src/upgrade/release-3.2.x-incubating.asciidoc b/docs/src/upgrade/release-3.2.x-incubating.asciidoc index 78c276be0f9..d01f58fb590 100644 --- a/docs/src/upgrade/release-3.2.x-incubating.asciidoc +++ b/docs/src/upgrade/release-3.2.x-incubating.asciidoc @@ -32,6 +32,16 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.2.5/CHANGELOG.asc Upgrading for Users ~~~~~~~~~~~~~~~~~~~ +GremlinScriptEngine Metrics +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The `GremlinScriptEngine` has a number of new metrics about its cache size and script compilation times which should +be helpful in understanding usage problems. As `GremlinScriptEngine` instances are used in Gremlin Server these metrics +are naturally exposed as part of the standard link:http://tinkerpop.apache.org/docs/current/reference/#_metrics[metrics] +set. Note that metrics are captured for both sessionless requests as well as for each individual session that is opened. + +See: https://issues.apache.org/jira/browse/TINKERPOP-1644[TINKERPOP-1644] + Gremlin-Python Driver ^^^^^^^^^^^^^^^^^^^^^ Gremlin-Python now offers a more complete driver implementation that uses connection pooling and From 33db1a3893c91a2dde100c8a0289312d4550503c Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 9 Mar 2017 14:51:36 -0500 Subject: [PATCH 15/19] TINKERPOP-1644 Cleaned up a bad import --- .../groovy/jsr223/GroovyCompilerGremlinPluginTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java index bacfc907534..d8cf5719e92 100644 --- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyCompilerGremlinPluginTest.java @@ -26,7 +26,6 @@ import java.util.Map; import java.util.Optional; -import static org.apache.tinkerpop.gremlin.groovy.jsr223.GroovyCompilerGremlinPlugin.*; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; import static org.hamcrest.core.IsInstanceOf.instanceOf; @@ -40,7 +39,7 @@ public class GroovyCompilerGremlinPluginTest { @Test public void shouldConfigureForGroovyOnly() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). - compilation(Compilation.COMPILE_STATIC).create(); + compilation(GroovyCompilerGremlinPlugin.Compilation.COMPILE_STATIC).create(); final Optional customizers = plugin.getCustomizers("gremlin-not-real"); assertThat(customizers.isPresent(), is(false)); } @@ -49,7 +48,7 @@ public void shouldConfigureForGroovyOnly() { public void shouldConfigureWithCompileStatic() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). expectedCompilationTime(0). - compilation(Compilation.COMPILE_STATIC).create(); + compilation(GroovyCompilerGremlinPlugin.Compilation.COMPILE_STATIC).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); assertEquals(1, customizers.get().length); @@ -60,7 +59,7 @@ public void shouldConfigureWithCompileStatic() { public void shouldConfigureWithTypeChecked() { final GroovyCompilerGremlinPlugin plugin = GroovyCompilerGremlinPlugin.build(). expectedCompilationTime(0). - compilation(Compilation.TYPE_CHECKED).create(); + compilation(GroovyCompilerGremlinPlugin.Compilation.TYPE_CHECKED).create(); final Optional customizers = plugin.getCustomizers("gremlin-groovy"); assertThat(customizers.isPresent(), is(true)); assertEquals(1, customizers.get().length); From 85c53305f83b9915538289bcb7de596620b76d6c Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Tue, 14 Mar 2017 12:30:14 -0400 Subject: [PATCH 16/19] TINKERPOP-1644 Fixed spelling errors --- docs/src/reference/gremlin-applications.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc index 37244e053e5..884ce0476e9 100644 --- a/docs/src/reference/gremlin-applications.asciidoc +++ b/docs/src/reference/gremlin-applications.asciidoc @@ -1560,10 +1560,10 @@ and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th a * `op.traversal` - the number of `Traveral` executions, mean rate, 1, 5, and 15 minute rates, minimum, maximum, median, mean, and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th and 99.9th percentile evaluation times. -* `engine-name.session.session-id.*` - metrics related to different `GremlinScriptEngine` instanc configured for +* `engine-name.session.session-id.*` - metrics related to different `GremlinScriptEngine` instances configured for session-based requests where "engine-name" will be the actual name of the engine, such as "gremlin-groovy" and "session-id" will be the identifier for the session itself. -* `engine-name.sessionless.*` - metrics related to different `GremlinScriptEngine` instanc configured for sessionless +* `engine-name.sessionless.*` - metrics related to different `GremlinScriptEngine` instances configured for sessionless requests where "engine-name" will be the actual name of the engine, such as "gremlin-groovy". Best Practices From 7fced963db5b73b4bb8e943f7db02cf2a491cd12 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Tue, 14 Mar 2017 12:30:26 -0400 Subject: [PATCH 17/19] TINKERPOP-1644 Changed time settings to Long values. Using long is consistent with other customizer instances. --- .../groovy/jsr223/CompilationOptionsCustomizer.java | 6 +++--- .../groovy/jsr223/GremlinGroovyScriptEngine.java | 2 +- .../CompilationOptionsCustomizerProvider.java | 12 ++++++++---- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java index 2a22eb33c05..de39b62dd04 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java @@ -27,13 +27,13 @@ */ class CompilationOptionsCustomizer implements Customizer { - private final int expectedCompilationTime; + private final long expectedCompilationTime; - public CompilationOptionsCustomizer(final int expectedCompilationTime) { + public CompilationOptionsCustomizer(final long expectedCompilationTime) { this.expectedCompilationTime = expectedCompilationTime; } - public int getExpectedCompilationTime() { + public long getExpectedCompilationTime() { return expectedCompilationTime; } } diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java index 0ed8d84cbed..4b271808eaa 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java @@ -238,7 +238,7 @@ public Future load(final String script) throws Exception { private final Set artifactsToUse = new HashSet<>(); private final boolean interpreterModeEnabled; - private final int expectedCompilationTime; + private final long expectedCompilationTime; /** * Creates a new instance using the {@link DefaultImportCustomizerProvider}. diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java index 047551148c3..00a20f1c6e4 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java @@ -31,13 +31,17 @@ @Deprecated public class CompilationOptionsCustomizerProvider implements CompilerCustomizerProvider { - private final int expectedCompilationTime; + private final long expectedCompilationTime; - public CompilationOptionsCustomizerProvider(final int expectedCompilationTime) { - this.expectedCompilationTime = expectedCompilationTime; + public CompilationOptionsCustomizerProvider(final Integer expectedCompilationTime) { + this.expectedCompilationTime = expectedCompilationTime.longValue(); } - public int getExpectedCompilationTime() { + public CompilationOptionsCustomizerProvider(final Long expectedCompilationTime) { + this.expectedCompilationTime = expectedCompilationTime.intValue(); + } + + public long getExpectedCompilationTime() { return expectedCompilationTime; } From 33416ddd743f6c726ccb9d91d5959ae50c60bab3 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Tue, 14 Mar 2017 12:43:58 -0400 Subject: [PATCH 18/19] TINKERPOP-1644 Minor fixup of variable assignment --- .../jsr223/customizer/CompilationOptionsCustomizerProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java index 00a20f1c6e4..58b304f3f82 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java @@ -38,7 +38,7 @@ public CompilationOptionsCustomizerProvider(final Integer expectedCompilationTim } public CompilationOptionsCustomizerProvider(final Long expectedCompilationTime) { - this.expectedCompilationTime = expectedCompilationTime.intValue(); + this.expectedCompilationTime = expectedCompilationTime; } public long getExpectedCompilationTime() { From d7c2a221ecb56442a01ca2e5cf539b011ac05f94 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Tue, 14 Mar 2017 15:26:46 -0400 Subject: [PATCH 19/19] TINKERPOP-1644 Added some docs for CompilationOptionsCustomizerProvider --- docs/src/reference/gremlin-applications.asciidoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc index 884ce0476e9..3a2c4464fb0 100644 --- a/docs/src/reference/gremlin-applications.asciidoc +++ b/docs/src/reference/gremlin-applications.asciidoc @@ -1336,6 +1336,7 @@ There are a number of pre-packaged `CustomizerProvider` implementations: |========================================================= |Customizer |Description |`CompileStaticCustomizerProvider` |Applies `CompileStatic` annotations to incoming scripts thus removing dynamic dispatch. More information about static compilation can be found in the link:http://docs.groovy-lang.org/latest/html/documentation/#_static_compilation[Groovy Documentation]. It is possible to configure this `CustomizerProvider` by specifying a comma separated list of link:http://docs.groovy-lang.org/latest/html/documentation/#Typecheckingextensions-Workingwithextensions[type checking extensions] that can have the effect of securing calls to various methods. +|`CompilationOptionsCustomizerProvider` |The amount of time a script is allowed to compile before a warning message is sent to the logs. |`ConfigurationCustomizerProvider` |Allows configuration of the Groovy `CompilerConfiguration` object by taking a `Map` of key/value pairs where the "key" is a property to set on the `CompilerConfiguration`. |`ThreadInterruptCustomizerProvider` |Injects checks for thread interruption, thus allowing the thread to potentially respect calls to `Thread.interrupt()` |`TimedInterruptCustomizerProvider` |Injects checks into loops to interrupt them if they exceed the configured timeout in milliseconds. @@ -1354,6 +1355,7 @@ scriptEngines: { compilerCustomizerProviders: { "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ThreadInterruptCustomizerProvider":[], "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.TimedInterruptCustomizerProvider":[10000], + "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider":[8000], "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompileStaticCustomizerProvider":["org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.SimpleSandboxExtension"]}}}} NOTE: The above configuration could also use the `TypeCheckedCustomizerProvider` in place of the