Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1644

Improve script compilation process and include metrics

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Done
    • Affects Version/s: 3.2.4
    • Fix Version/s: 3.2.5
    • Component/s: groovy
    • Labels:
      None

      Description

      Currently there is no synchronisation around script compilation. This means that if a particularly heavy script is in use, many threads may end up compiling the same script.

      It would seem like a good idea to have some some sort of synchronisation to prevent ever getting to this stage.

      In addition, there will be cases where users will repeatedly submit broken scripts to the server. In this case it is useful to log the error the first time the script compilation is attempted and then cache the error for subsequent runs.

      Finally I have found some scripts take in excess of 30 seconds to compile. To aid performance debugging the script compilation times should be included in the logs.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/570

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/570
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/570

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/570 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/570

          Works now.

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/570 Works now. VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/570

          Should work if you use the `CompilationOptionsCustomizerProvider` in your gremlin-server.yaml as you tried to do with the `CompilationOptionsCustomizer` previously:

          ```text
          "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider":[10],
          ```

          I just pushed another commit with a few lines of additional docs related to that configuration option.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/570 Should work if you use the `CompilationOptionsCustomizerProvider` in your gremlin-server.yaml as you tried to do with the `CompilationOptionsCustomizer` previously: ```text "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider": [10] , ``` I just pushed another commit with a few lines of additional docs related to that configuration option.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/570

          @spmallette If you could just clarify if compilation timeout is configurable in 3.2? if so, how?

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/570 @spmallette If you could just clarify if compilation timeout is configurable in 3.2? if so, how?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/570

          Changes have been pushed - hopefully I have all issues addressed now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/570 Changes have been pushed - hopefully I have all issues addressed now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105962373

          — Diff: 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();
              • End diff –

          ffs

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105962373 — Diff: 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(); End diff – ffs
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105959819

          — Diff: 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();
              • End diff –

          Did you mean `.longValue()`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105959819 — Diff: 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(); End diff – Did you mean `.longValue()`?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105959185

          — Diff: 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) {
          — End diff –

          Ok. I'm just trying to figure out how to test the compilation timeout without having a 5-second script. I would like to set the timeout to 1ms. Even using the deprecated config doesn't seem to work for me.
          ```
          compilerCustomizerProviders:

          { "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider": [1]}

          ```
          Error:
          ```
          [WARN] ScriptEngines - Could not instantiate CompilerCustomizerProvider implementation [org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider]. It will not be applied.
          java.lang.IllegalStateException: Could not configure org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ConfigurationCustomizerProvider with the supplied options [1]
          at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.lambda$createScriptEngine$15(ScriptEngines.java:418)
          at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684)
          at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.createScriptEngine(ScriptEngines.java:399)
          at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.reload(ScriptEngines.java:187)
          at org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$createScriptEngines$13(GremlinExecutor.java:512)
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105959185 — Diff: 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) { — End diff – Ok. I'm just trying to figure out how to test the compilation timeout without having a 5-second script. I would like to set the timeout to 1ms. Even using the deprecated config doesn't seem to work for me. ``` compilerCustomizerProviders: { "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider": [1]} ``` Error: ``` [WARN] ScriptEngines - Could not instantiate CompilerCustomizerProvider implementation [org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider] . It will not be applied. java.lang.IllegalStateException: Could not configure org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ConfigurationCustomizerProvider with the supplied options [1] at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.lambda$createScriptEngine$15(ScriptEngines.java:418) at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684) at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.createScriptEngine(ScriptEngines.java:399) at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.reload(ScriptEngines.java:187) at org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$createScriptEngines$13(GremlinExecutor.java:512) ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105952425

          — Diff: 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) {
          — End diff –

          I'm converting the integers to longs just to be consistent with other times being in long milliseconds. The new plugin system doesn't work in the same way as the old so trying to push a `CompilationOptionsCustomizer` into the list of `compilerCustomizerProviders` won't work. I don't really think we should promote the new plugin system until 3.3.0, but if you really want to give it a shot you'll need to look at the SNAPSHOT docs and revised config files for gremlin-server on the master branch that show how these new configurations go in place. You can get an idea of how this will work under this new model here:

          https://github.com/apache/tinkerpop/blob/master/gremlin-server/conf/gremlin-server-secure.yaml#L36

          You would basically add a config option to that line for `expectedCompilationTime` and it will configure the `CompilationOptionsCustomizer` into the `GremlinScriptEngine`.

          When I go to merge this to master, I'll need to update docs to reflect how this configuration setting works. I will comment back here as soon as I've pushed the latest changed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105952425 — Diff: 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) { — End diff – I'm converting the integers to longs just to be consistent with other times being in long milliseconds. The new plugin system doesn't work in the same way as the old so trying to push a `CompilationOptionsCustomizer` into the list of `compilerCustomizerProviders` won't work. I don't really think we should promote the new plugin system until 3.3.0, but if you really want to give it a shot you'll need to look at the SNAPSHOT docs and revised config files for gremlin-server on the master branch that show how these new configurations go in place. You can get an idea of how this will work under this new model here: https://github.com/apache/tinkerpop/blob/master/gremlin-server/conf/gremlin-server-secure.yaml#L36 You would basically add a config option to that line for `expectedCompilationTime` and it will configure the `CompilationOptionsCustomizer` into the `GremlinScriptEngine`. When I go to merge this to master, I'll need to update docs to reflect how this configuration setting works. I will comment back here as soon as I've pushed the latest changed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105764358

          — Diff: 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) {
          — End diff –

          I believe `int` should be `Integer` looking at the other customizers. However, there seems to be issues with customizers in general where in 3.2.5 they are deprecated and no longer public classes. So when I modify this class with Integer, (or try to use the newer, non 'customizer' package customizers) I get the following error.

          ```
          compilerCustomizerProviders:

          { "org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer":[1]}

          ```
          Error:
          ```
          [WARN] ScriptEngines - Could not instantiate CompilerCustomizerProvider implementation [org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer]. It will not be applied.
          java.lang.IllegalAccessException: Class org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines can not access a member of class org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer with modifiers "public"
          at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
          at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
          at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
          at java.lang.reflect.Constructor.newInstance(Constructor.java:413)
          at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.lambda$createScriptEngine$15(ScriptEngines.java:416)
          at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684)
          at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.createScriptEngine(ScriptEngines.java:399)
          at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.reload(ScriptEngines.java:187)
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105764358 — Diff: 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) { — End diff – I believe `int` should be `Integer` looking at the other customizers. However, there seems to be issues with customizers in general where in 3.2.5 they are deprecated and no longer public classes. So when I modify this class with Integer, (or try to use the newer, non 'customizer' package customizers) I get the following error. ``` compilerCustomizerProviders: { "org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer":[1]} ``` Error: ``` [WARN] ScriptEngines - Could not instantiate CompilerCustomizerProvider implementation [org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer] . It will not be applied. java.lang.IllegalAccessException: Class org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines can not access a member of class org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer with modifiers "public" at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102) at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296) at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288) at java.lang.reflect.Constructor.newInstance(Constructor.java:413) at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.lambda$createScriptEngine$15(ScriptEngines.java:416) at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684) at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.createScriptEngine(ScriptEngines.java:399) at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.reload(ScriptEngines.java:187) ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105759976

          — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java —
          @@ -149,19 +159,64 @@
          }
          };

          + private GremlinGroovyClassLoader loader;
          +
          /**

          • Script to generated Class map.
            */
          • private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
            + private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder().
            + softValues().
            + recordStats().
            + build(new CacheLoader<String, Future<Class>>() {
            + @Override
            + public Future<Class> load(final String script) throws Exception {
            + final long start = System.currentTimeMillis();
            +
            + return CompletableFuture.supplyAsync(() -> {
            + try { + return loader.parseClass(script, generateScriptName()); + }

            catch (CompilationFailedException e) {
            + final long finish = System.currentTimeMillis();
            + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e);
            + failedCompilationCount.incrementAndGet();

              • End diff –

          That does the trick. Making a separate comment about compilation timeout...

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105759976 — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java — @@ -149,19 +159,64 @@ } }; + private GremlinGroovyClassLoader loader; + /** Script to generated Class map. */ private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder(). + softValues(). + recordStats(). + build(new CacheLoader<String, Future<Class>>() { + @Override + public Future<Class> load(final String script) throws Exception { + final long start = System.currentTimeMillis(); + + return CompletableFuture.supplyAsync(() -> { + try { + return loader.parseClass(script, generateScriptName()); + } catch (CompilationFailedException e) { + final long finish = System.currentTimeMillis(); + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e); + failedCompilationCount.incrementAndGet(); End diff – That does the trick. Making a separate comment about compilation timeout...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105754194

          — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java —
          @@ -149,19 +159,64 @@
          }
          };

          + private GremlinGroovyClassLoader loader;
          +
          /**

          • Script to generated Class map.
            */
          • private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
            + private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder().
            + softValues().
            + recordStats().
            + build(new CacheLoader<String, Future<Class>>() {
            + @Override
            + public Future<Class> load(final String script) throws Exception {
            + final long start = System.currentTimeMillis();
            +
            + return CompletableFuture.supplyAsync(() -> {
            + try { + return loader.parseClass(script, generateScriptName()); + }

            catch (CompilationFailedException e) {
            + final long finish = System.currentTimeMillis();
            + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e);
            + failedCompilationCount.incrementAndGet();

              • End diff –

          This is a syntactically correct script. Did you try something like `g.V().butthead)(` or a huge script that runs into a compilation timeout?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105754194 — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java — @@ -149,19 +159,64 @@ } }; + private GremlinGroovyClassLoader loader; + /** Script to generated Class map. */ private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder(). + softValues(). + recordStats(). + build(new CacheLoader<String, Future<Class>>() { + @Override + public Future<Class> load(final String script) throws Exception { + final long start = System.currentTimeMillis(); + + return CompletableFuture.supplyAsync(() -> { + try { + return loader.parseClass(script, generateScriptName()); + } catch (CompilationFailedException e) { + final long finish = System.currentTimeMillis(); + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e); + failedCompilationCount.incrementAndGet(); End diff – This is a syntactically correct script. Did you try something like `g.V().butthead)(` or a huge script that runs into a compilation timeout?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105747173

          — Diff: 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
              • End diff –

          instances

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105747173 — Diff: 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 End diff – instances
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105747128

          — Diff: 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
              • End diff –

          instances

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105747128 — Diff: 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 End diff – instances
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/570#discussion_r105750603

          — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java —
          @@ -149,19 +159,64 @@
          }
          };

          + private GremlinGroovyClassLoader loader;
          +
          /**

          • Script to generated Class map.
            */
          • private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
            + private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder().
            + softValues().
            + recordStats().
            + build(new CacheLoader<String, Future<Class>>() {
            + @Override
            + public Future<Class> load(final String script) throws Exception {
            + final long start = System.currentTimeMillis();
            +
            + return CompletableFuture.supplyAsync(() -> {
            + try { + return loader.parseClass(script, generateScriptName()); + }

            catch (CompilationFailedException e) {
            + final long finish = System.currentTimeMillis();
            + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e);
            + failedCompilationCount.incrementAndGet();

              • End diff –

          I'm not sure how to trigger this. I can't get `gremlin-groovy.sessionless.class-cache.load-failure-count` to increment.
          Tried
          `curl "http://localhost:8182?gremlin=g.V().beavis()"`
          and
          ```
          :remote connect tinkerpop.server conf/remote.yaml
          :> g.V().beavis()
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/570#discussion_r105750603 — Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java — @@ -149,19 +159,64 @@ } }; + private GremlinGroovyClassLoader loader; + /** Script to generated Class map. */ private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle()); + private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder(). + softValues(). + recordStats(). + build(new CacheLoader<String, Future<Class>>() { + @Override + public Future<Class> load(final String script) throws Exception { + final long start = System.currentTimeMillis(); + + return CompletableFuture.supplyAsync(() -> { + try { + return loader.parseClass(script, generateScriptName()); + } catch (CompilationFailedException e) { + final long finish = System.currentTimeMillis(); + log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e); + failedCompilationCount.incrementAndGet(); End diff – I'm not sure how to trigger this. I can't get `gremlin-groovy.sessionless.class-cache.load-failure-count` to increment. Tried `curl "http://localhost:8182?gremlin=g.V().beavis()"` and ``` :remote connect tinkerpop.server conf/remote.yaml :> g.V().beavis() ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/570

          TINKERPOP-1644 Improve script compilation process and include metrics

          https://issues.apache.org/jira/browse/TINKERPOP-1644

          This PR was started on #567 which was submitted by @BrynCooke. I've made some basic modifications and included various metrics about compilation that will be helpful in debugging things. Those metrics are reported up through Gremlin Server through the standard metric reporting system.

          Builds to success with `docker/build.sh -t -i -n`.

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1644

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/570.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #570


          commit 13c93cabac51112a73f592e0fba12e515f643522
          Author: BrynCooke <bryncooke@gmail.com>
          Date: 2017-03-02T19:07:28Z

          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.

          commit deb68280d986b086120d56a73659b1869c07a475
          Author: BrynCooke <bryncooke@gmail.com>
          Date: 2017-03-07T16:54:58Z

          TINKERPOP-1644
          Use future instead of maintaining a separate map of failures.

          commit 18778e405981523355319fa56bd04fd6cc07b845
          Author: BrynCooke <bryncooke@gmail.com>
          Date: 2017-03-08T12:24:46Z

          TINKERPOP-1644
          Use Caffeine cache

          commit 17a72e0e1db6fe52d3a0821ef2bfae1eb39be856
          Author: BrynCooke <bryncooke@gmail.com>
          Date: 2017-03-08T13:23:30Z

          TINKERPOP-1644
          Fix exception handling.

          commit 1df71c81e032daa9c9db6f626d99c39b37d434ce
          Author: BrynCooke <bryncooke@gmail.com>
          Date: 2017-03-08T13:26:31Z

          TINKERPOP-1644
          Fix exception handling.

          commit 608f024f7bb83eb168a6aa637d46ee0650674903
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-08T18:31:52Z

          TINKERPOP-1644 Remove gremlin-server caffeine dependency

          Caffeine is now down in gremlin-groovy.

          commit de1d58ab33c3121e9bafb5e4908f3e0d76c8e26e
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-08T18:37:42Z

          TINKERPOP-1644 Minor code format updates

          commit 4bdeac4b796b38fb14d1be763e03cc837b57d3d7
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-08T21:13:06Z

          TINKERPOP-1644 Provided configuration options for GremlinGroovyScriptEngine

          Introduced new customizers to pass in configuration options to the GremlinGroovyScriptEngine.

          commit b29ba12109e7e88bc3465d08f6177e5ded17ca59
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-08T21:15:27Z

          TINKERPOP-1644 Updated changelog

          commit a06072b0502f9b42fee4c68dba554b4991406c36
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-08T21:17:40Z

          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.

          commit 8ffa5af6ff56933c1955e88e8aa42c06cb69162b
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-09T15:30:17Z

          TINKERPOP-1644 Added metrics to GremlinGroovyScriptEngine

          commit b689deb1dd1a0c745966a1ed5f8e7e3b2d543af2
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-09T17:25:35Z

          TINKERPOP-1644 Exposed GremlinScriptEngine metrics in Gremlin Server

          commit 9eb248ef0f0f29e8cf11cd5391ef8d993e37f7af
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-09T18:15:04Z

          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.

          commit 37976526f1eac9fd06858f962f979aa98d3e5346
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-09T18:35:44Z

          TINKERPOP-1644 Docs for new metrics in Gremlin Server

          commit 33db1a3893c91a2dde100c8a0289312d4550503c
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-03-09T19:51:36Z

          TINKERPOP-1644 Cleaned up a bad import


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/570 TINKERPOP-1644 Improve script compilation process and include metrics https://issues.apache.org/jira/browse/TINKERPOP-1644 This PR was started on #567 which was submitted by @BrynCooke. I've made some basic modifications and included various metrics about compilation that will be helpful in debugging things. Those metrics are reported up through Gremlin Server through the standard metric reporting system. Builds to success with `docker/build.sh -t -i -n`. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1644 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/570.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #570 commit 13c93cabac51112a73f592e0fba12e515f643522 Author: BrynCooke <bryncooke@gmail.com> Date: 2017-03-02T19:07:28Z 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. commit deb68280d986b086120d56a73659b1869c07a475 Author: BrynCooke <bryncooke@gmail.com> Date: 2017-03-07T16:54:58Z TINKERPOP-1644 Use future instead of maintaining a separate map of failures. commit 18778e405981523355319fa56bd04fd6cc07b845 Author: BrynCooke <bryncooke@gmail.com> Date: 2017-03-08T12:24:46Z TINKERPOP-1644 Use Caffeine cache commit 17a72e0e1db6fe52d3a0821ef2bfae1eb39be856 Author: BrynCooke <bryncooke@gmail.com> Date: 2017-03-08T13:23:30Z TINKERPOP-1644 Fix exception handling. commit 1df71c81e032daa9c9db6f626d99c39b37d434ce Author: BrynCooke <bryncooke@gmail.com> Date: 2017-03-08T13:26:31Z TINKERPOP-1644 Fix exception handling. commit 608f024f7bb83eb168a6aa637d46ee0650674903 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-08T18:31:52Z TINKERPOP-1644 Remove gremlin-server caffeine dependency Caffeine is now down in gremlin-groovy. commit de1d58ab33c3121e9bafb5e4908f3e0d76c8e26e Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-08T18:37:42Z TINKERPOP-1644 Minor code format updates commit 4bdeac4b796b38fb14d1be763e03cc837b57d3d7 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-08T21:13:06Z TINKERPOP-1644 Provided configuration options for GremlinGroovyScriptEngine Introduced new customizers to pass in configuration options to the GremlinGroovyScriptEngine. commit b29ba12109e7e88bc3465d08f6177e5ded17ca59 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-08T21:15:27Z TINKERPOP-1644 Updated changelog commit a06072b0502f9b42fee4c68dba554b4991406c36 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-08T21:17:40Z 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. commit 8ffa5af6ff56933c1955e88e8aa42c06cb69162b Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-09T15:30:17Z TINKERPOP-1644 Added metrics to GremlinGroovyScriptEngine commit b689deb1dd1a0c745966a1ed5f8e7e3b2d543af2 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-09T17:25:35Z TINKERPOP-1644 Exposed GremlinScriptEngine metrics in Gremlin Server commit 9eb248ef0f0f29e8cf11cd5391ef8d993e37f7af Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-09T18:15:04Z 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. commit 37976526f1eac9fd06858f962f979aa98d3e5346 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-09T18:35:44Z TINKERPOP-1644 Docs for new metrics in Gremlin Server commit 33db1a3893c91a2dde100c8a0289312d4550503c Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-03-09T19:51:36Z TINKERPOP-1644 Cleaned up a bad import
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user BrynCooke closed the pull request at:

          https://github.com/apache/tinkerpop/pull/567

          Show
          githubbot ASF GitHub Bot added a comment - Github user BrynCooke closed the pull request at: https://github.com/apache/tinkerpop/pull/567
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          I've merged these changes to a branch so that I can continue with the development needed on this one:

          https://github.com/apache/tinkerpop/tree/TINKERPOP-1644

          @BrynCooke you can feel free to close this PR now. I'll issue a new one from the TinkerPop branch when I'm done.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/567 I've merged these changes to a branch so that I can continue with the development needed on this one: https://github.com/apache/tinkerpop/tree/TINKERPOP-1644 @BrynCooke you can feel free to close this PR now. I'll issue a new one from the TinkerPop branch when I'm done.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          `ManagedConcurrentValueMap` allows you to configure a `Map` with weak/soft/phantom/hard references. Groovy has "Managed" classes for lots of different types of Lists/Maps.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/567 `ManagedConcurrentValueMap` allows you to configure a `Map` with weak/soft/phantom/hard references. Groovy has "Managed" classes for lots of different types of Lists/Maps.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user BrynCooke commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          1. ManagedConcurrentValueMap is not a map. I don't really know why we use this. but computeIfAbsent is not present on this class.
          2. Guava LoadingCache and MoreExecutors.directExecutor() is the correct way of solving this problem. It allows soft values and does all the syncronisation stuff for us. Is there a reason why guava is not used by Tinkerpop?

          If we want avoid guava we need to have a striped lock on script, and to create our own direct executor.

          Show
          githubbot ASF GitHub Bot added a comment - Github user BrynCooke commented on the issue: https://github.com/apache/tinkerpop/pull/567 1. ManagedConcurrentValueMap is not a map. I don't really know why we use this. but computeIfAbsent is not present on this class. 2. Guava LoadingCache and MoreExecutors.directExecutor() is the correct way of solving this problem. It allows soft values and does all the syncronisation stuff for us. Is there a reason why guava is not used by Tinkerpop? If we want avoid guava we need to have a striped lock on script, and to create our own direct executor.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          I like this way better than the synchronization. I have some reservations about:

          ```java
          CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName()));
          ```

          That's going to use the `ForkJoinPool` to execute the compilation, which is a shared thread pool for the JVM that isn't configurable and basically outside of our control. Of course, getting rid of it means adding a new `ExecutorService` that we can configure and monitor in `GremlinGroovyScriptEngine`. Making that kind of change goes pretty deep into various aspects of TinkerPop. If @BrynCooke is ok with the changes I can merge what he has to a branch since it seems like we have general consensus on the approach and then make some additional changes and re-issue the PR for a final VOTE. If there's no objections I'll start work on that first thing tomorrow.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/567 I like this way better than the synchronization. I have some reservations about: ```java CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName())); ``` That's going to use the `ForkJoinPool` to execute the compilation, which is a shared thread pool for the JVM that isn't configurable and basically outside of our control. Of course, getting rid of it means adding a new `ExecutorService` that we can configure and monitor in `GremlinGroovyScriptEngine`. Making that kind of change goes pretty deep into various aspects of TinkerPop. If @BrynCooke is ok with the changes I can merge what he has to a branch since it seems like we have general consensus on the approach and then make some additional changes and re-issue the PR for a final VOTE. If there's no objections I'll start work on that first thing tomorrow.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          Yes. `computeIfAbsent()` is much better than synchronized `get()`, check, `put()`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/567 Yes. `computeIfAbsent()` is much better than synchronized `get()`, check, `put()`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          This part:

          ```
          if (clazz != null)

          { return clazz.get(); }

          clazz = CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName()));
          classMap.put(script, clazz);
          return clazz.get();
          ```

          should be more like:

          ```
          classMap.computeIfAbsent(script, s ->
          CompletableFuture.supplyAsync(() -> loader.parseClass(s, generateScriptName())))
          return classMap.get(script).get();
          ```

          No?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/567 This part: ``` if (clazz != null) { return clazz.get(); } clazz = CompletableFuture.supplyAsync(() -> loader.parseClass(script, generateScriptName())); classMap.put(script, clazz); return clazz.get(); ``` should be more like: ``` classMap.computeIfAbsent(script, s -> CompletableFuture.supplyAsync(() -> loader.parseClass(s, generateScriptName()))) return classMap.get(script).get(); ``` No?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/567 VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user BrynCooke commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          I like the idea of using a Future.

          Show
          githubbot ASF GitHub Bot added a comment - Github user BrynCooke commented on the issue: https://github.com/apache/tinkerpop/pull/567 I like the idea of using a Future.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          That sounds like a good alternative.

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/567 That sounds like a good alternative.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          Hmm, I haven't done any benchmarks, but this seem to be valid points. Just a thought: Would it help to have the class map be like `ManagedConcurrentValueMap<String, Future<Class>> classMap`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/567 Hmm, I haven't done any benchmarks, but this seem to be valid points. Just a thought: Would it help to have the class map be like `ManagedConcurrentValueMap<String, Future<Class>> classMap`?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          I have a concern on overall performance. I agree that "Script compilation is synchronised" helps in the rare case when two or more identical scripts are submitted concurrently. However, in the more likely case of new, unique scripts being submitted concurrently, it would seem that this effectively serializes script compilation creating a bottleneck. Unless I'm missing something, parallel compilation seems to be the better choice.

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/567 I have a concern on overall performance. I agree that "Script compilation is synchronised" helps in the rare case when two or more identical scripts are submitted concurrently. However, in the more likely case of new, unique scripts being submitted concurrently, it would seem that this effectively serializes script compilation creating a bottleneck. Unless I'm missing something, parallel compilation seems to be the better choice.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          Yep, missing some spaces here and there, but the code in general looks good.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/567 Yep, missing some spaces here and there, but the code in general looks good. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/567

          I will clean up a few code formatting things on merge, but this looks good to me. All tests pass with `docker/build.sh -t -n -i`

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/567 I will clean up a few code formatting things on merge, but this looks good to me. All tests pass with `docker/build.sh -t -n -i` VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user BrynCooke opened a pull request:

          https://github.com/apache/tinkerpop/pull/567

          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.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/BrynCooke/incubator-tinkerpop TINKERPOP-1644

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/567.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #567


          commit ca229694dc036210ad9e7ae2fc32134720b48d1b
          Author: BrynCooke <bryncooke@gmail.com>
          Date: 2017-03-02T19:07:28Z

          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.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user BrynCooke opened a pull request: https://github.com/apache/tinkerpop/pull/567 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. You can merge this pull request into a Git repository by running: $ git pull https://github.com/BrynCooke/incubator-tinkerpop TINKERPOP-1644 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/567.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #567 commit ca229694dc036210ad9e7ae2fc32134720b48d1b Author: BrynCooke <bryncooke@gmail.com> Date: 2017-03-02T19:07:28Z 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.

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              bryncooke Bryn Cooke
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development