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.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/13c93cab Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/13c93cab Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/13c93cab Branch: refs/heads/TINKERPOP-1642 Commit: 13c93cabac51112a73f592e0fba12e515f643522 Parents: 60a3fb3 Author: BrynCooke <[email protected]> Authored: Thu Mar 2 19:07:28 2017 +0000 Committer: Stephen Mallette <[email protected]> Committed: Fri Mar 10 11:12:36 2017 -0500 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 4 ++ .../jsr223/GremlinGroovyScriptEngine.java | 57 ++++++++++++++++++-- 2 files changed, 56 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/13c93cab/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 6861d57..24e5e9c 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 `<edge>` 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. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/13c93cab/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java ---------------------------------------------------------------------- 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 1fb2efc..a8365a2 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.InvokerHelper; 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 @@ import java.util.stream.Collectors; 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 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl * Script to generated Class map. */ private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private ManagedConcurrentValueMap<String, CompilationFailedException> failedClassMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); /** * Global closures map - this is used to simulate a single global functions namespace @@ -529,13 +533,56 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl 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) {
