Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Java invoker cache logic to allow encapsulation, sync. #3395

Merged
merged 4 commits into from
Oct 14, 2015

Conversation

headius
Copy link
Member

@headius headius commented Oct 14, 2015

The move to IntHashMap in RubyToJavaInvoker improved throughput
for multiple-arity calls, but since IntHashMap is not synchronized
this could cause corrupted data under concurrent load as seen in
CallableSelector to encapsulate the cache and synchronize access
to it. I believe this is a minimum effort to make the cache access
safe and fix #3394.

I have not measured the impact of this synchronization in a high
concurrency setting. It may indeed turn out to be more overhead
and a worse bottleneck than using ConcurrentHashMap with Integer
objects. Unfortunately there's no middle ground here unless we
build or adopt some concurrency-friendly int-based map.

Another heavy option would be to maintain two references to
IntHashMap in RubyToJavaInvoker, copying from a synchronized
writable copy to a read-only copy after every write. This would
double the size of the cache structures in memory but would provide
a fast, unsynchronized cache for read-mostly signature lookups.
This pattern is also easier to support with the refactoring in
this commit.

The move to IntHashMap in RubyToJavaInvoker improved throughput
for multiple-arity calls, but since IntHashMap is not synchronized
this could cause corrupted data under concurrent load as seen in
CallableSelector to encapsulate the cache and synchronize access
to it. I believe this is a minimum effort to make the cache access
safe and fix #3394.

I have not measured the impact of this synchronization in a high
concurrency setting. It may indeed turn out to be more overhead
and a worse bottleneck than using ConcurrentHashMap with Integer
objects. Unfortunately there's no middle ground here unless we
build or adopt some concurrency-friendly int-based map.

Another heavy option would be to maintain two references to
IntHashMap in RubyToJavaInvoker, copying from a synchronized
writable copy to a read-only copy after every write. This would
double the size of the cache structures in memory but would provide
a fast, unsynchronized cache for read-mostly signature lookups.
This pattern is also easier to support with the refactoring in
this commit.
@headius
Copy link
Member Author

headius commented Oct 14, 2015

FWIW here's the diff that would implement a copy-on-write strategy here to avoid synchronization on reads. I suspect most of these caches are reasonably small, but I have done no measurement of either the performance impact of synchronization nor the memory impact (and cold cache startup impact) of copy-on-write.

diff --git a/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java b/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java
index 222636f..29ac1a4 100644
--- a/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java
+++ b/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java
@@ -35,7 +35,8 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth

     // in case multiple callables (overloaded Java method - same name different args)
     // for the invoker exists  CallableSelector caches resolution based on args here
-    final IntHashMap<T> cache;
+    final IntHashMap<T> writeCache;
+    volatile IntHashMap<T> readCache;

     private final Ruby runtime;

@@ -54,7 +55,8 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
             minVarArgsArity = getMemberArity(member) - 1;
         }

-        cache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
+        writeCache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
+        readCache = writeCache;

         this.javaCallable = callable;
         this.javaCallables = null;
@@ -84,7 +86,8 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
             }
             callables = null;

-            cache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
+            writeCache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
+            readCache = writeCache;
         }
         else {
             callable = null; maxArity = -1; minArity = Integer.MAX_VALUE;
@@ -139,7 +142,8 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
                 varargsCallables = varArgs.toArray( createCallableArray(varArgs.size()) );
             }

-            cache = newCallableCache();
+            writeCache = newCallableCache();
+            readCache = (IntHashMap<T>)writeCache.clone();
         }

         this.javaCallable = callable;
@@ -192,14 +196,13 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
     }

     public T getSignature(int signatureCode) {
-        synchronized (cache) {
-            return cache.get(signatureCode);
-        }
+        return readCache.get(signatureCode);
     }

     public void putSignature(int signatureCode, T callable) {
-        synchronized (cache) {
-            cache.put(signatureCode, callable);
+        synchronized (writeCache) {
+            writeCache.put(signatureCode, callable);
+            readCache = (IntHashMap<T>)writeCache.clone();
         }
     }

diff --git a/core/src/main/java/org/jruby/util/collections/IntHashMap.java b/core/src/main/java/org/jruby/util/collections/IntHashMap.java
index fb9a16c..1a09297 100644
--- a/core/src/main/java/org/jruby/util/collections/IntHashMap.java
+++ b/core/src/main/java/org/jruby/util/collections/IntHashMap.java
@@ -397,6 +397,19 @@ public class IntHashMap<V> {
        }
     }

+    @Override
+    public Object clone() {
+        IntHashMap<V> newMap = new IntHashMap<>(table.length, loadFactor);
+        for (int i = 0; i < table.length; i++) {
+            Entry<V> entry = table[i];
+            while (entry != null) {
+                newMap.put(entry.getKey(), entry.getValue());
+                entry = entry.next;
+            }
+        }
+        return newMap;
+    }
+
     @SuppressWarnings("unchecked")
     public static <U> IntHashMap<U> nullMap() { return NullMap.INSTANCE; }

@kares
Copy link
Member

kares commented Oct 14, 2015

👍 looks good. unless the user constantly comes up with a different parameters signature (very unlikely) the write cache won't be used, so it should be good.
think there's another place where IntHashMap is used as a cache the same way, thus it might need this.

@headius
Copy link
Member Author

headius commented Oct 14, 2015

For the record, I tried using a read/write lock instead of copy-on-write, and the results indicate that COW is still probably a better option.

Based on bench/bench_java_overloaded_invocation.rb with an additional case for contended output, I got these results.

COW of IntHashMap:

                                                    user     system      total        real
Control                                         0.380000   0.010000   0.390000 (  0.386502)
Math.abs with byte-ranged Fixnum                1.710000   0.020000   1.730000 (  1.644227)
Math.abs with short-ranged Fixnum               1.750000   0.010000   1.760000 (  1.733172)
Math.abs with int-ranged Fixnum                 1.830000   0.010000   1.840000 (  1.815107)
Math.abs with long-ranged Fixnum                1.690000   0.010000   1.700000 (  1.672152)
Math.abs with float-ranged Float                1.920000   0.020000   1.940000 (  1.925157)
Math.abs with double-ranged Float               1.820000   0.010000   1.830000 (  1.804220)
Contended Math.abs with double-ranged Float     3.070000   0.030000   3.100000 (  0.442268)

Single IntHashMap with read/write lock:

Control                                         0.420000   0.000000   0.420000 (  0.417004)
Math.abs with byte-ranged Fixnum                1.940000   0.010000   1.950000 (  1.936122)
Math.abs with short-ranged Fixnum               1.870000   0.010000   1.880000 (  1.860634)
Math.abs with int-ranged Fixnum                 1.920000   0.010000   1.930000 (  1.907168)
Math.abs with long-ranged Fixnum                1.880000   0.020000   1.900000 (  1.862109)
Math.abs with float-ranged Float                2.010000   0.010000   2.020000 (  2.002120)
Math.abs with double-ranged Float               1.960000   0.020000   1.980000 (  1.953830)
Contended Math.abs with double-ranged Float    16.040000   0.070000  16.110000 (  2.183073)

So the read/write lock is slightly slower in the uncontended case but considerably slower in the contended case. I think we'll go with the COW version for now.

The patch, for future reference:

diff --git a/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java b/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java
index 29ac1a4..b80af7a 100644
--- a/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java
+++ b/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java
@@ -6,6 +6,9 @@ import java.lang.reflect.Member;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;

 import org.jruby.Ruby;
 import org.jruby.RubyModule;
@@ -35,8 +38,8 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth

     // in case multiple callables (overloaded Java method - same name different args)
     // for the invoker exists  CallableSelector caches resolution based on args here
-    final IntHashMap<T> writeCache;
-    volatile IntHashMap<T> readCache;
+    final IntHashMap<T> cache;
+    final ReadWriteLock lock = new ReentrantReadWriteLock();

     private final Ruby runtime;

@@ -55,8 +58,7 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
             minVarArgsArity = getMemberArity(member) - 1;
         }

-        writeCache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
-        readCache = writeCache;
+        cache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used

         this.javaCallable = callable;
         this.javaCallables = null;
@@ -86,8 +88,7 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
             }
             callables = null;

-            writeCache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
-            readCache = writeCache;
+            cache = NULL_CACHE; // if there's a single callable - matching (and thus the cache) won't be used
         }
         else {
             callable = null; maxArity = -1; minArity = Integer.MAX_VALUE;
@@ -142,8 +143,7 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
                 varargsCallables = varArgs.toArray( createCallableArray(varArgs.size()) );
             }

-            writeCache = newCallableCache();
-            readCache = (IntHashMap<T>)writeCache.clone();
+            cache = newCallableCache();
         }

         this.javaCallable = callable;
@@ -196,13 +196,20 @@ public abstract class RubyToJavaInvoker<T extends JavaCallable> extends JavaMeth
     }

     public T getSignature(int signatureCode) {
-        return readCache.get(signatureCode);
+        lock.readLock().lock();
+        try {
+            return cache.get(signatureCode);
+        } finally {
+            lock.readLock().unlock();
+        }
     }

     public void putSignature(int signatureCode, T callable) {
-        synchronized (writeCache) {
-            writeCache.put(signatureCode, callable);
-            readCache = (IntHashMap<T>)writeCache.clone();
+        lock.writeLock().lock();
+        try {
+            cache.put(signatureCode, callable);
+        } finally {
+            lock.writeLock().unlock();
         }
     }

@headius
Copy link
Member Author

headius commented Oct 14, 2015

Also FYI, the numbers for COW IntHashMap versus unsynchronized IntHashMap are roughly identical, as expected. I did not do a comparison with ConcurrentHashMap.

Another note: CHM is big. Having two copies of the comparatively tiny IntHashMap probably isn't anywhere near as large in memory.

headius added a commit that referenced this pull request Oct 14, 2015
Refactor Java invoker cache logic to allow encapsulation, sync.
@headius headius merged commit d0bcfee into master Oct 14, 2015
@headius headius deleted the sync_signature_caches branch October 14, 2015 14:52
@kares
Copy link
Member

kares commented Oct 14, 2015

very nice - thank you for attempting and sharing this,

@enebo enebo modified the milestone: Non-Release May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JRuby thread stuck on IntHashMap.rehash
3 participants