Mailing-List: contact commits-help@tinkerpop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@tinkerpop.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: spmallette@apache.org
To: commits@tinkerpop.apache.org
Message-Id: <a03f9b9b3f8c4c2882453f97bfbff674@git.apache.org>
Subject: tinkerpop git commit: TINKERPOP-1560 Used ManagedConcurrentValueMap
 in GremlinGroovyClassLoader
Date: Fri, 20 Jan 2017 17:35:14 +0000 (UTC)
archived-at: Fri, 20 Jan 2017 17:35:16 -0000

Repository: tinkerpop
Updated Branches:
  refs/heads/TINKERPOP-1560 [created] 061bbc42c


TINKERPOP-1560 Used ManagedConcurrentValueMap in GremlinGroovyClassLoader

By configuring the ManagedConcurrentValueMap to have weak references, the GremlinGroovyClassLoader, and therefore the GremlinGroovyScriptEngine, are now able to "forget" classes that are no longer used. It was determined that the cache of these classes would grow indefinitely for each script passed to the GremlinGroovyScriptEngine, thus allowing the metaspace to continue to grow. It isn't really possible to write tests to verify that this change works, but I did test manually by watching memory usage in a profiler and could see that metaspace memory stayed stable and that classes were unloading from the classloader over time.


Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/061bbc42
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/061bbc42
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/061bbc42

Branch: refs/heads/TINKERPOP-1560
Commit: 061bbc42cda701b521c742a319f762d9af5741db
Parents: 97cc07d
Author: Stephen Mallette <spmva@genoprime.com>
Authored: Fri Jan 20 11:54:45 2017 -0500
Committer: Stephen Mallette <spmva@genoprime.com>
Committed: Fri Jan 20 11:54:45 2017 -0500

----------------------------------------------------------------------
 CHANGELOG.asciidoc                              |  1 +
 .../groovy/jsr223/GremlinGroovyClassLoader.java | 31 ++++++++++-
 .../jsr223/ManagedConcurrentValueMap.java       |  4 ++
 .../GremlinGroovyScriptEngineIntegrateTest.java | 55 ++++++++++++++++++++
 4 files changed, 89 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/061bbc42/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 4f3f9ce..feb03c6 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -29,6 +29,7 @@ TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET)
 * SASL negotiation supports both a byte array and Base64 encoded bytes as a string for authentication to Gremlin Server.
 * Deprecated `TinkerIoRegistry` replacing it with the more consistently named `TinkerIoRegistryV1d0`.
 * Made error messaging more consistent during result iteration timeouts in Gremlin Server.
+* Fixed a memory leak in the classloader for the `GremlinGroovyScriptEngine` where classes in the loader were not releasing from memory as a strong reference was always maintained.
 * `PathRetractionStrategy` does not add a `NoOpBarrierStep` to the end of local children as its wasted computation in 99% of traversals.
 * Fixed a bug in `AddVertexStartStep` where if a side-effect was being used in the parametrization, an NPE occurred.
 * Fixed a bug in `LazyBarrierStrategy` where `profile()` was deactivating it accidentally.

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/061bbc42/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyClassLoader.java
----------------------------------------------------------------------
diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyClassLoader.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyClassLoader.java
index e987ce0..1894372 100644
--- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyClassLoader.java
+++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyClassLoader.java
@@ -20,19 +20,46 @@ package org.apache.tinkerpop.gremlin.groovy.jsr223;
 
 import groovy.lang.GroovyClassLoader;
 import org.codehaus.groovy.control.CompilerConfiguration;
+import org.codehaus.groovy.util.ReferenceBundle;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentMap;
 
 /**
- * A {@code GroovyClassLoader} extension that provides access to the {@code removeClassCacheEntry(String)} method.
+ * A {@code GroovyClassLoader} extension that uses a {@link ManagedConcurrentValueMap} configured with soft references.
+ * In this way, the classloader will "forget" scripts allowing them to be garbage collected and thus prevent memory
+ * issues in JVM metaspace.
  *
  * @author Stephen Mallette (http://stephen.genoprime.com)
  */
 class GremlinGroovyClassLoader extends GroovyClassLoader {
+    private final ManagedConcurrentValueMap<String, Class> classSoftCache;
+
     public GremlinGroovyClassLoader(final ClassLoader parent, final CompilerConfiguration conf) {
         super(parent, conf);
+        classSoftCache = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
     }
 
     @Override
     protected void removeClassCacheEntry(final String name) {
-        super.removeClassCacheEntry(name);
+        synchronized(this.classSoftCache) {
+            this.classSoftCache.remove(name);
+        }
+    }
+
+    @Override
+    protected void setClassCacheEntry(final Class cls) {
+        synchronized(this.classSoftCache) {
+            this.classSoftCache.put(cls.getName(), cls);
+        }
+    }
+
+    @Override
+    protected Class getClassCacheEntry(final String name) {
+        if(null == name)  return null;
+
+        synchronized(this.classSoftCache) {
+            return this.classSoftCache.get(name);
+        }
     }
 }

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/061bbc42/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/ManagedConcurrentValueMap.java
----------------------------------------------------------------------
diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/ManagedConcurrentValueMap.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/ManagedConcurrentValueMap.java
index dce056b..f1ee00d 100644
--- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/ManagedConcurrentValueMap.java
+++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/ManagedConcurrentValueMap.java
@@ -76,6 +76,10 @@ class ManagedConcurrentValueMap<K, V> {
         internalMap.put(key, ref);
     }
 
+    public void remove(final K key) {
+        internalMap.remove(key);
+    }
+
     /**
      * Clear the map.
      */

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/061bbc42/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinGroovyScriptEngineIntegrateTest.java
----------------------------------------------------------------------
diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinGroovyScriptEngineIntegrateTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinGroovyScriptEngineIntegrateTest.java
new file mode 100644
index 0000000..5242d3b
--- /dev/null
+++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinGroovyScriptEngineIntegrateTest.java
@@ -0,0 +1,55 @@
+/*
+ * 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.jsr223;
+
+import org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine;
+import org.javatuples.Pair;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author Stephen Mallette (http://stephen.genoprime.com)
+ */
+public class GremlinGroovyScriptEngineIntegrateTest {
+    @Test
+    @Ignore("This is not a test that needs to run on build - it's more for profiling the GremlinGroovyScriptEngine")
+    public void shouldTest() throws Exception {
+        final Random r = new Random();
+        final List<Pair<String, Integer>> scripts = new ArrayList<>();
+        final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine();
+        for (int ix = 0; ix < 1000000; ix++) {
+            final String script = "1 + " + ix;
+            final int output = (int) engine.eval(script);
+            assertEquals(1 + ix, output);
+
+            if (ix % 1000 == 0) scripts.add(Pair.with(script, output));
+
+            if (ix % 25 == 0) {
+                final Pair<String,Integer> p = scripts.get(r.nextInt(scripts.size()));
+                assertEquals(p.getValue1().intValue(), (int) engine.eval(p.getValue0()));
+            }
+        }
+    }
+}