From 026d1d9d66cf7c3bb1b60bff73ed058e7bbe01e1 Mon Sep 17 00:00:00 2001 From: kares Date: Fri, 27 Feb 2015 13:18:37 +0100 Subject: [PATCH 01/15] add toString on CacheEntry for debugging --- .../main/java/org/jruby/runtime/callsite/CacheEntry.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/src/main/java/org/jruby/runtime/callsite/CacheEntry.java b/core/src/main/java/org/jruby/runtime/callsite/CacheEntry.java index 70d358de183..b16c14fb615 100644 --- a/core/src/main/java/org/jruby/runtime/callsite/CacheEntry.java +++ b/core/src/main/java/org/jruby/runtime/callsite/CacheEntry.java @@ -21,4 +21,12 @@ public final boolean typeOk(RubyClass incomingType) { public static final boolean typeOk(CacheEntry entry, RubyClass incomingType) { return entry.token == incomingType.getGeneration(); } + + @Override + public String toString() { + return getClass().getName() + '@' + + Integer.toHexString(System.identityHashCode(this)) + + ""; + } + } From ac6a9de9fc89782c9fc6f073747a59d1d0cb6e7c Mon Sep 17 00:00:00 2001 From: kares Date: Fri, 27 Feb 2015 13:21:40 +0100 Subject: [PATCH 02/15] do not re-wrap RuntimeException + unnecessary elses --- .../org/jruby/javasupport/JavaSupport.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/jruby/javasupport/JavaSupport.java b/core/src/main/java/org/jruby/javasupport/JavaSupport.java index 159f523026f..6d5c4c6ec92 100644 --- a/core/src/main/java/org/jruby/javasupport/JavaSupport.java +++ b/core/src/main/java/org/jruby/javasupport/JavaSupport.java @@ -78,14 +78,14 @@ public IRubyObject allocateProxy(Object javaObject, RubyClass clazz) { private static final Constructor CLASS_VALUE_CONSTRUCTOR; static { - Constructor constructor = null; + Constructor constructor = null; if (Options.INVOKEDYNAMIC_CLASS_VALUES.load()) { try { // try to load the ClassValue class. If it succeeds, we can use our // ClassValue-based cache. Class.forName("java.lang.ClassValue"); - constructor = (Constructor)Class.forName("org.jruby.util.collections.Java7ClassValue").getConstructor(ClassValueCalculator.class); + constructor = Class.forName("org.jruby.util.collections.Java7ClassValue").getConstructor(ClassValueCalculator.class); } catch (Exception ex) { // fall through to Map version @@ -95,12 +95,14 @@ public IRubyObject allocateProxy(Object javaObject, RubyClass clazz) { if (constructor == null) { try { constructor = MapBasedClassValue.class.getConstructor(ClassValueCalculator.class); - } catch (Exception ex2) { - throw new RuntimeException(ex2); + } + catch (Exception ex) { + if ( ex instanceof RuntimeException ) throw (RuntimeException) ex; + throw new RuntimeException(ex); } } - CLASS_VALUE_CONSTRUCTOR = constructor; + CLASS_VALUE_CONSTRUCTOR = (Constructor) constructor; } private RubyModule javaModule; @@ -217,15 +219,17 @@ public RubyModule getProxyClassFromCache(Class clazz) { } public void handleNativeException(Throwable exception, Member target) { - if (exception instanceof RaiseException) { + if ( exception instanceof RaiseException ) { // allow RaiseExceptions to propagate throw (RaiseException) exception; - } else if (exception instanceof Unrescuable) { + } + if (exception instanceof Unrescuable) { // allow "unrescuable" flow-control exceptions to propagate - if (exception instanceof Error) { - throw (Error)exception; - } else if (exception instanceof RuntimeException) { - throw (RuntimeException)exception; + if ( exception instanceof Error ) { + throw (Error) exception; + } + if ( exception instanceof RuntimeException ) { + throw (RuntimeException) exception; } } throw createRaiseException(exception, target); From 317e9ba99fc8482ce9c219cfe83a2ab88dff58e6 Mon Sep 17 00:00:00 2001 From: kares Date: Fri, 27 Feb 2015 10:03:27 +0100 Subject: [PATCH 03/15] for easier multi-thread debugging trigger name calculation when proxy class ready --- core/src/main/java/org/jruby/RubyModule.java | 9 +++++---- core/src/main/java/org/jruby/javasupport/JavaClass.java | 2 -- .../org/jruby/javasupport/binding/ClassInitializer.java | 2 ++ .../jruby/javasupport/binding/InterfaceInitializer.java | 2 ++ 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java index c8a1999d2fe..28a9155b06d 100644 --- a/core/src/main/java/org/jruby/RubyModule.java +++ b/core/src/main/java/org/jruby/RubyModule.java @@ -438,24 +438,25 @@ public String getBaseName() { */ public void setBaseName(String name) { baseName = name; + cachedName = null; } /** * Generate a fully-qualified class name or a #-style name for anonymous and singleton classes. - * + * * Ruby C equivalent = "classname" - * + * * @return The generated class name */ public String getName() { if (cachedName != null) return cachedName; return calculateName(); } - + /** * Get the "simple" name for the class, which is either the "base" name or * the "anonymous" class name. - * + * * @return the "simple" name of the class */ public String getSimpleName() { diff --git a/core/src/main/java/org/jruby/javasupport/JavaClass.java b/core/src/main/java/org/jruby/javasupport/JavaClass.java index 8ad3f472bd3..f2e04099574 100644 --- a/core/src/main/java/org/jruby/javasupport/JavaClass.java +++ b/core/src/main/java/org/jruby/javasupport/JavaClass.java @@ -63,9 +63,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; -import java.util.HashMap; import java.util.List; -import java.util.Map; @JRubyClass(name="Java::JavaClass", parent="Java::JavaObject") public class JavaClass extends JavaObject { diff --git a/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java b/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java index 36042abeb96..5736b21097a 100644 --- a/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java +++ b/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java @@ -86,6 +86,8 @@ else if ( length > offset + 1 ) { // skip '$' installClassConstructors(proxyClass, state); installClassClasses(javaClass, proxyClass); + proxy.getName(); // trigger calculateName() + return proxy; } diff --git a/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java b/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java index 259717a13ea..8ec85472e66 100644 --- a/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java +++ b/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java @@ -65,6 +65,8 @@ public RubyModule initialize(RubyModule proxy) { installClassStaticMethods(proxy, state); installClassClasses(javaClass, proxy); + proxy.getName(); // trigger calculateName() + return proxy; } From a3d28b86fa5ae4781caf73b796878ff3403b7dff Mon Sep 17 00:00:00 2001 From: kares Date: Fri, 27 Feb 2015 10:05:12 +0100 Subject: [PATCH 04/15] no need to call getReturnType multiple times + unused check whether class is public --- .../org/jruby/javasupport/JavaMethod.java | 60 +++++++++++-------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/jruby/javasupport/JavaMethod.java b/core/src/main/java/org/jruby/javasupport/JavaMethod.java index 922751f2bd2..bd593e977c0 100644 --- a/core/src/main/java/org/jruby/javasupport/JavaMethod.java +++ b/core/src/main/java/org/jruby/javasupport/JavaMethod.java @@ -21,7 +21,7 @@ * Copyright (C) 2004 David Corbin * Copyright (C) 2005 Charles O Nutter * Copyright (C) 2006 Kresten Krab Thorup - * + * * Alternatively, the contents of this file may be used under the terms of * either of the GNU General Public License Version 2 or later (the "GPL"), * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), @@ -68,8 +68,9 @@ public class JavaMethod extends JavaCallable { private final static boolean USE_HANDLES = RubyInstanceConfig.USE_GENERATED_HANDLES; private final static boolean HANDLE_DEBUG = false; + private final Method method; - private final Class boxedReturnType; + private final Class boxedReturnType; private final boolean isFinal; private final JavaUtil.JavaConverter returnConverter; @@ -80,12 +81,12 @@ public Object getValue() { public static RubyClass createJavaMethodClass(Ruby runtime, RubyModule javaModule) { // TODO: NOT_ALLOCATABLE_ALLOCATOR is probably ok here, since we don't intend for people to monkey with // this type and it can't be marshalled. Confirm. JRUBY-415 - RubyClass result = + RubyClass result = javaModule.defineClassUnder("JavaMethod", runtime.getObject(), ObjectAllocator.NOT_ALLOCATABLE_ALLOCATOR); JavaAccessibleObject.registerRubyMethods(runtime, result); JavaCallable.registerRubyMethods(runtime, result); - + result.defineAnnotatedMethods(JavaMethod.class); return result; @@ -95,33 +96,31 @@ public JavaMethod(Ruby runtime, Method method) { super(runtime, runtime.getJavaSupport().getJavaMethodClass(), method.getParameterTypes()); this.method = method; this.isFinal = Modifier.isFinal(method.getModifiers()); - if (method.getReturnType().isPrimitive() && method.getReturnType() != void.class) { - this.boxedReturnType = CodegenUtils.getBoxType(method.getReturnType()); + final Class returnType = method.getReturnType(); + if (returnType.isPrimitive() && returnType != void.class) { + this.boxedReturnType = CodegenUtils.getBoxType(returnType); } else { - this.boxedReturnType = method.getReturnType(); + this.boxedReturnType = returnType; } - boolean methodIsPublic = Modifier.isPublic(method.getModifiers()); - boolean classIsPublic = Modifier.isPublic(method.getDeclaringClass().getModifiers()); - - // Special classes like Collections.EMPTY_LIST are inner classes that are private but - // implement public interfaces. Their methods are all public methods for the public + // Special classes like Collections.EMPTY_LIST are inner classes that are private but + // implement public interfaces. Their methods are all public methods for the public // interface. Let these public methods execute via setAccessible(true). if (JavaUtil.CAN_SET_ACCESSIBLE) { // we should be able to setAccessible ok... try { - if (methodIsPublic && - !Modifier.isPublic(method.getDeclaringClass().getModifiers())) { - accessibleObject().setAccessible(true); + if ( Modifier.isPublic(method.getModifiers()) && + ! Modifier.isPublic(method.getDeclaringClass().getModifiers()) ) { + method.setAccessible(true); } } catch (SecurityException se) { // we shouldn't get here if JavaClass.CAN_SET_ACCESSIBLE is doing // what it should, so we warn. - runtime.getWarnings().warn("failed to setAccessible: " + accessibleObject() + ", exception follows: " + se.getMessage()); + runtime.getWarnings().warn("failed to setAccessible: " + method + ", exception follows: " + se.getMessage()); } } - - returnConverter = JavaUtil.getJavaConverter(method.getReturnType()); + + returnConverter = JavaUtil.getJavaConverter(returnType); } public static JavaMethod create(Ruby runtime, Method method) { @@ -158,12 +157,12 @@ public static JavaMethod getMatchingDeclaredMethod(Ruby runtime, Class javaCl MethodSearch: for (Method method : javaClass.getDeclaredMethods()) { if (method.getName().equals(methodName)) { Class[] targetTypes = method.getParameterTypes(); - + // for zero args case we can stop searching if (targetTypes.length == 0 && argumentTypes.length == 0) { return create(runtime, method); } - + TypeScan: for (int i = 0; i < argumentTypes.length; i++) { if (i >= targetTypes.length) continue MethodSearch; @@ -183,13 +182,13 @@ public static JavaMethod getMatchingDeclaredMethod(Ruby runtime, Class javaCl // no matching method found return null; } - + @Override public boolean equals(Object other) { return other instanceof JavaMethod && this.method == ((JavaMethod)other).method; } - + @Override public int hashCode() { return method.hashCode(); @@ -201,6 +200,7 @@ public RubyString name() { return getRuntime().newString(method.getName()); } + @Override public int getArity() { return parameterTypes.length; } @@ -269,7 +269,7 @@ public IRubyObject invoke_static(IRubyObject[] args) { @JRubyMethod public IRubyObject return_type() { Class klass = method.getReturnType(); - + if (klass.equals(void.class)) { return getRuntime().getNil(); } @@ -544,30 +544,37 @@ private void convertArguments(IRubyObject[] argsIn, Object[] argsOut, int from) } } + @Override public Class[] getParameterTypes() { return parameterTypes; } + @Override public Class[] getExceptionTypes() { return method.getExceptionTypes(); } + @Override public Type[] getGenericParameterTypes() { return method.getGenericParameterTypes(); } + @Override public Type[] getGenericExceptionTypes() { return method.getGenericExceptionTypes(); } - + + @Override public Annotation[][] getParameterAnnotations() { return method.getParameterAnnotations(); } + @Override public boolean isVarArgs() { return method.isVarArgs(); } + @Override protected String nameOnInspection() { return "#<" + getType().toString() + "/" + method.getName() + "("; } @@ -576,7 +583,7 @@ protected String nameOnInspection() { public RubyBoolean static_p() { return getRuntime().newBoolean(isStatic()); } - + public RubyBoolean bridge_p() { return getRuntime().newBoolean(method.isBridge()); } @@ -585,14 +592,17 @@ private boolean isStatic() { return Modifier.isStatic(method.getModifiers()); } + @Override public int getModifiers() { return method.getModifiers(); } + @Override public String toGenericString() { return method.toGenericString(); } + @Override public AccessibleObject accessibleObject() { return method; } From 0d1c338bdccebd1b3b9066fade0d5cdbc40ebaf5 Mon Sep 17 00:00:00 2001 From: kares Date: Fri, 27 Feb 2015 23:34:12 +0100 Subject: [PATCH 05/15] test asserting that concurrent proxy initialization does dispatch methods correctly closing #2014 --- test/test_higher_javasupport.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/test_higher_javasupport.rb b/test/test_higher_javasupport.rb index e7551681e9c..23504347a14 100644 --- a/test/test_higher_javasupport.rb +++ b/test/test_higher_javasupport.rb @@ -861,7 +861,7 @@ def test_no_warnings_on_concurrent_package_const_initialization def test_no_warnings_on_concurrent_class_const_initialization Java.send :remove_const, :DefaultPackageClass if Java.const_defined? :DefaultPackageClass - + stderr = $stderr; require 'stringio' begin $stderr = StringIO.new @@ -879,4 +879,26 @@ def test_no_warnings_on_concurrent_class_const_initialization end end + # reproducing https://github.com/jruby/jruby/issues/2014 + def test_concurrent_proxy_class_initialization + abort_on_exception = Thread.abort_on_exception + begin + Thread.abort_on_exception = true + # some strange enough (un-initialized proxy) classes not used elsewhere + threads = (0..10).map do + Thread.new { Java::java.awt.Desktop::Action.valueOf('OPEN') } + end + threads.each { |thread| thread.join } + + # same but top (package) level class and using an aliased method : + threads = (0..10).map do + Thread.new { java.lang.management.MemoryType.value_of('HEAP') } + end + threads.each { |thread| thread.join } + ensure + Thread.abort_on_exception = abort_on_exception + end + + end + end From 2f5ff5cc939a21ae494c7eb90c4b6926d536ad89 Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 10:51:32 +0100 Subject: [PATCH 06/15] lazily initialize the deprecated (un-used) javaObjectVariables field in JavaSupport --- .../main/java/org/jruby/javasupport/JavaSupport.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/jruby/javasupport/JavaSupport.java b/core/src/main/java/org/jruby/javasupport/JavaSupport.java index 6d5c4c6ec92..69737ad4957 100644 --- a/core/src/main/java/org/jruby/javasupport/JavaSupport.java +++ b/core/src/main/java/org/jruby/javasupport/JavaSupport.java @@ -42,7 +42,6 @@ import java.util.HashMap; import java.util.Map; import java.util.Set; -import java.util.concurrent.locks.ReentrantLock; import org.jruby.Ruby; import org.jruby.RubyClass; @@ -374,7 +373,7 @@ public ClassValue> getInstanceAssignedNames() { } @Deprecated - private volatile Map javaObjectVariables = new WeakIdentityHashMap(); + private volatile Map javaObjectVariables; @Deprecated public Object getJavaObjectVariable(Object o, int i) { @@ -384,8 +383,6 @@ public Object getJavaObjectVariable(Object o, int i) { if (variables == null) return null; synchronized (this) { - if (variables == null) return null; - Object[] vars = variables.get(o); if (vars == null || vars.length <= i) return null; return vars[i]; @@ -399,13 +396,16 @@ public void setJavaObjectVariable(Object o, int i, Object v) { synchronized (this) { Map variables = javaObjectVariables; - if (variables == null) javaObjectVariables = variables = new WeakIdentityHashMap(); + if (variables == null) { + variables = javaObjectVariables = new WeakIdentityHashMap(); + } Object[] vars = variables.get(o); if (vars == null) { vars = new Object[i + 1]; variables.put(o, vars); - } else if (vars.length <= i) { + } + else if (vars.length <= i) { Object[] newVars = new Object[i + 1]; System.arraycopy(vars, 0, newVars, 0, vars.length); variables.put(o, newVars); From f857e70b7c921307f33e6ae67f98201b0c32bf7a Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 10:53:34 +0100 Subject: [PATCH 07/15] hide internal getMethods at package level & do less collection 2 array conversions --- .../javasupport/binding/ClassInitializer.java | 10 ++- .../javasupport/binding/Initializer.java | 80 ++++++++++--------- .../binding/InterfaceInitializer.java | 10 ++- .../jruby/javasupport/binding/Priority.java | 2 - 4 files changed, 55 insertions(+), 47 deletions(-) diff --git a/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java b/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java index 5736b21097a..a7c7d91f7d4 100644 --- a/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java +++ b/core/src/main/java/org/jruby/javasupport/binding/ClassInitializer.java @@ -10,6 +10,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.util.List; import java.util.Map; /** @@ -20,6 +21,7 @@ public ClassInitializer(Ruby runtime, Class javaClass) { super(runtime, javaClass); } + @Override public RubyModule initialize(RubyModule proxy) { RubyClass proxyClass = (RubyClass)proxy; Class superclass = javaClass.getSuperclass(); @@ -122,15 +124,15 @@ private static void setupClassFields(Class javaClass, Initializer.State state private void setupClassMethods(Class javaClass, State state) { // TODO: protected methods. this is going to require a rework of some of the mechanism. - Method[] methods = getMethods(javaClass); + final List methods = getMethods(javaClass); - for (int i = methods.length; --i >= 0;) { + for ( int i = methods.size(); --i >= 0; ) { // we need to collect all methods, though we'll only // install the ones that are named in this class - Method method = methods[i]; + Method method = methods.get(i); String name = method.getName(); - if (Modifier.isStatic(method.getModifiers())) { + if ( Modifier.isStatic( method.getModifiers() ) ) { prepareStaticMethod(javaClass, state, method, name); } else { prepareInstanceMethod(javaClass, state, method, name); diff --git a/core/src/main/java/org/jruby/javasupport/binding/Initializer.java b/core/src/main/java/org/jruby/javasupport/binding/Initializer.java index a93ec54cd75..0113316c208 100644 --- a/core/src/main/java/org/jruby/javasupport/binding/Initializer.java +++ b/core/src/main/java/org/jruby/javasupport/binding/Initializer.java @@ -268,9 +268,9 @@ protected static void handleScalaSingletons(final Class javaClass, final Stat final Object singleton = field.get(null); if ( singleton == null ) return; - final Method[] scalaMethods = getMethods(companionClass); - for ( int j = scalaMethods.length - 1; j >= 0; j-- ) { - final Method method = scalaMethods[j]; + final List scalaMethods = getMethods(companionClass); + for ( int j = scalaMethods.size() - 1; j >= 0; j-- ) { + final Method method = scalaMethods.get(j); String name = method.getName(); if (DEBUG_SCALA) LOG.debug("Companion object method {} for {}", name, companionClass); @@ -427,47 +427,47 @@ public static class State { } - public static Method[] getMethods(Class javaClass) { - HashMap> nameMethods = new HashMap>(30); + static List getMethods(final Class javaClass) { + HashMap> nameMethods = new HashMap>(32); // to better size the final ArrayList below - int total = 0; + int totalMethods = 0; // we scan all superclasses, but avoid adding superclass methods with // same name+signature as subclass methods (see JRUBY-3130) - for (Class c = javaClass; c != null; c = c.getSuperclass()) { + for ( Class klass = javaClass; klass != null; klass = klass.getSuperclass() ) { // only add class's methods if it's public or we can set accessible // (see JRUBY-4799) - if (Modifier.isPublic(c.getModifiers()) || JavaUtil.CAN_SET_ACCESSIBLE) { + if (Modifier.isPublic(klass.getModifiers()) || JavaUtil.CAN_SET_ACCESSIBLE) { // for each class, scan declared methods for new signatures try { // add methods, including static if this is the actual class, // and replacing child methods with equivalent parent methods - total += addNewMethods(nameMethods, c.getDeclaredMethods(), c == javaClass, true); - } catch (SecurityException e) { + totalMethods += addNewMethods(nameMethods, klass.getDeclaredMethods(), klass == javaClass, true); } + catch (SecurityException e) { /* ignored */ } } // then do the same for each interface - for (Class i : c.getInterfaces()) { + for ( Class iface : klass.getInterfaces() ) { try { // add methods, not including static (should be none on // interfaces anyway) and not replacing child methods with // parent methods - total += addNewMethods(nameMethods, i.getMethods(), false, false); - } catch (SecurityException e) { + totalMethods += addNewMethods(nameMethods, iface.getMethods(), false, false); } + catch (SecurityException e) { /* ignored */ } } } // now only bind the ones that remain - ArrayList finalList = new ArrayList(total); + ArrayList finalList = new ArrayList(totalMethods); - for (Map.Entry> entry : nameMethods.entrySet()) { - finalList.addAll(entry.getValue()); + for ( Map.Entry> entry : nameMethods.entrySet() ) { + finalList.addAll( entry.getValue() ); } - return finalList.toArray(new Method[finalList.size()]); + return finalList; } private static boolean methodsAreEquivalent(Method child, Method parent) { @@ -480,42 +480,49 @@ private static boolean methodsAreEquivalent(Method child, Method parent) { && Arrays.equals(child.getParameterTypes(), parent.getParameterTypes()); } - private static int addNewMethods(HashMap> nameMethods, Method[] methods, boolean includeStatic, boolean removeDuplicate) { + private static int addNewMethods( + final HashMap> nameMethods, + final Method[] methods, + final boolean includeStatic, + final boolean removeDuplicate) { + int added = 0; - Methods: for (Method m : methods) { + + Methods: for (final Method method : methods) { + final int mod = method.getModifiers(); // Skip private methods, since they may mess with dispatch - if (Modifier.isPrivate(m.getModifiers())) continue; + if ( Modifier.isPrivate(mod) ) continue; // ignore bridge methods because we'd rather directly call methods that this method // is bridging (and such methods are by definition always available.) - if ((m.getModifiers()&ACC_BRIDGE)!=0) - continue; + if ( ( mod & ACC_BRIDGE ) != 0 ) continue; - if (!includeStatic && Modifier.isStatic(m.getModifiers())) { + if ( ! includeStatic && Modifier.isStatic(mod) ) { // Skip static methods if we're not suppose to include them. // Generally for superclasses; we only bind statics from the actual // class. continue; } - List childMethods = nameMethods.get(m.getName()); + + List childMethods = nameMethods.get(method.getName()); if (childMethods == null) { // first method of this name, add a collection for it - childMethods = new ArrayList(1); - childMethods.add(m); - nameMethods.put(m.getName(), childMethods); - added++; - } else { - // we have seen other methods; check if we already have - // an equivalent one - for (ListIterator iter = childMethods.listIterator(); iter.hasNext();) { - Method m2 = iter.next(); - if (methodsAreEquivalent(m2, m)) { + childMethods = new ArrayList(4); + childMethods.add(method); added++; + nameMethods.put(method.getName(), childMethods); + } + else { + // we have seen other methods; check if we already have an equivalent one + final ListIterator childMethodsIterator = childMethods.listIterator(); + while ( childMethodsIterator.hasNext() ) { + final Method current = childMethodsIterator.next(); + if ( methodsAreEquivalent(current, method) ) { if (removeDuplicate) { // Replace the existing method, since the super call is more general // and virtual dispatch will call the subclass impl anyway. // Used for instance methods, for which we usually want to use the highest-up // callable implementation. - iter.set(m); + childMethodsIterator.set(method); } else { // just skip the new method, since we don't need it (already found one) // used for interface methods, which we want to add unconditionally @@ -525,8 +532,7 @@ private static int addNewMethods(HashMap> nameMethods, Meth } } // no equivalent; add it - childMethods.add(m); - added++; + childMethods.add(method); added++; } } return added; diff --git a/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java b/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java index 8ec85472e66..bb99e4986a3 100644 --- a/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java +++ b/core/src/main/java/org/jruby/javasupport/binding/InterfaceInitializer.java @@ -7,6 +7,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.util.List; import java.util.Map; /** @@ -18,6 +19,7 @@ public InterfaceInitializer(Ruby runtime, Class javaClass) { super(runtime, javaClass); } + @Override public RubyModule initialize(RubyModule proxy) { final State state = new State(runtime, null); @@ -72,14 +74,14 @@ public RubyModule initialize(RubyModule proxy) { private static void setupInterfaceMethods(Class javaClass, Initializer.State state) { // TODO: protected methods. this is going to require a rework of some of the mechanism. - Method[] methods = getMethods(javaClass); + final List methods = getMethods(javaClass); - for (int i = methods.length; --i >= 0;) { + for ( int i = methods.size(); --i >= 0; ) { // Java 8 introduced static methods on interfaces, so we just look for those - Method method = methods[i]; + Method method = methods.get(i); String name = method.getName(); - if (!Modifier.isStatic(method.getModifiers())) continue; + if ( ! Modifier.isStatic( method.getModifiers() ) ) continue; prepareStaticMethod(javaClass, state, method, name); } diff --git a/core/src/main/java/org/jruby/javasupport/binding/Priority.java b/core/src/main/java/org/jruby/javasupport/binding/Priority.java index c8798cd0be1..58f61beac30 100644 --- a/core/src/main/java/org/jruby/javasupport/binding/Priority.java +++ b/core/src/main/java/org/jruby/javasupport/binding/Priority.java @@ -1,7 +1,5 @@ package org.jruby.javasupport.binding; -import org.jruby.javasupport.JavaClass; - /** * Assigned names only override based priority of an assigned type, the type must be less than * or equal to the assigned type. For example, field name (FIELD) in a subclass will override From 61acccd0623796d0f760793bb1ac737ffa2208a5 Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 12:10:04 +0100 Subject: [PATCH 08/15] add a test (from #1621) for guarding against dead-locks in proxy-class initialization --- test/Bug1621A.java | 4 ++++ test/Bug1621B.java | 4 ++++ test/Bug1621C.java | 4 ++++ test/Bug1621D.java | 4 ++++ test/Bug1621E.java | 4 ++++ test/Bug1621F.java | 4 ++++ test/Bug1621G.java | 4 ++++ test/Bug1621H.java | 4 ++++ test/test_higher_javasupport.rb | 19 ++++++++++++++++++- 9 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 test/Bug1621A.java create mode 100644 test/Bug1621B.java create mode 100644 test/Bug1621C.java create mode 100644 test/Bug1621D.java create mode 100644 test/Bug1621E.java create mode 100644 test/Bug1621F.java create mode 100644 test/Bug1621G.java create mode 100644 test/Bug1621H.java diff --git a/test/Bug1621A.java b/test/Bug1621A.java new file mode 100644 index 00000000000..7125cb17b5c --- /dev/null +++ b/test/Bug1621A.java @@ -0,0 +1,4 @@ +public class Bug1621A { + static final Bug1621A INSTANCE = new Bug1621A(); + public static final Bug1621B NEXT = Bug1621B.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621B.java b/test/Bug1621B.java new file mode 100644 index 00000000000..cfd280c10a7 --- /dev/null +++ b/test/Bug1621B.java @@ -0,0 +1,4 @@ +public class Bug1621B { + static final Bug1621B INSTANCE = new Bug1621B(); + public static final Bug1621C NEXT = Bug1621C.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621C.java b/test/Bug1621C.java new file mode 100644 index 00000000000..f5cd4ba438e --- /dev/null +++ b/test/Bug1621C.java @@ -0,0 +1,4 @@ +public class Bug1621C { + static final Bug1621C INSTANCE = new Bug1621C(); + public static final Bug1621D NEXT = Bug1621D.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621D.java b/test/Bug1621D.java new file mode 100644 index 00000000000..5204de5a471 --- /dev/null +++ b/test/Bug1621D.java @@ -0,0 +1,4 @@ +public class Bug1621D { + static final Bug1621D INSTANCE = new Bug1621D(); + public static final Bug1621E NEXT = Bug1621E.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621E.java b/test/Bug1621E.java new file mode 100644 index 00000000000..79f0906c30b --- /dev/null +++ b/test/Bug1621E.java @@ -0,0 +1,4 @@ +public class Bug1621E { + static final Bug1621E INSTANCE = new Bug1621E(); + public static final Bug1621F NEXT = Bug1621F.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621F.java b/test/Bug1621F.java new file mode 100644 index 00000000000..95c4087bc32 --- /dev/null +++ b/test/Bug1621F.java @@ -0,0 +1,4 @@ +public class Bug1621F { + static final Bug1621F INSTANCE = new Bug1621F(); + public static final Bug1621G NEXT = Bug1621G.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621G.java b/test/Bug1621G.java new file mode 100644 index 00000000000..fbfa3338f16 --- /dev/null +++ b/test/Bug1621G.java @@ -0,0 +1,4 @@ +public class Bug1621G { + static final Bug1621G INSTANCE = new Bug1621G(); + public static final Bug1621H NEXT = Bug1621H.INSTANCE; +} \ No newline at end of file diff --git a/test/Bug1621H.java b/test/Bug1621H.java new file mode 100644 index 00000000000..16cd3229b02 --- /dev/null +++ b/test/Bug1621H.java @@ -0,0 +1,4 @@ +public class Bug1621H { + static final Bug1621H INSTANCE = new Bug1621H(); + public static final Bug1621A NEXT = Bug1621A.INSTANCE; +} \ No newline at end of file diff --git a/test/test_higher_javasupport.rb b/test/test_higher_javasupport.rb index 23504347a14..c30605130f5 100644 --- a/test/test_higher_javasupport.rb +++ b/test/test_higher_javasupport.rb @@ -880,7 +880,7 @@ def test_no_warnings_on_concurrent_class_const_initialization end # reproducing https://github.com/jruby/jruby/issues/2014 - def test_concurrent_proxy_class_initialization + def test_concurrent_proxy_class_initialization_invalid_method_dispatch abort_on_exception = Thread.abort_on_exception begin Thread.abort_on_exception = true @@ -898,7 +898,24 @@ def test_concurrent_proxy_class_initialization ensure Thread.abort_on_exception = abort_on_exception end + end + # reproducing https://github.com/jruby/jruby/issues/1621 + def test_concurrent_proxy_class_initialization_dead_lock + timeout = 0.5; threads_to_kill = [] + begin + threads = %w{ A B C D E F G H }.map do |sym| + Thread.new { Java::Default.const_get "Bug1621#{sym}" } + end + threads.each do |thread| + threads_to_kill << thread if thread.join(timeout).nil? + end + if threads_to_kill.any? + fail "threads: #{threads_to_kill.inspect} dead-locked!" + end + ensure + threads_to_kill.each { |thread| thread.exit rescue nil } + end end end From db7268b6a331b99009665f41dcfdb49730cec41a Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 12:37:55 +0100 Subject: [PATCH 09/15] avoid unnecessary arraycopy in JavaMethod invoke_static + re-format some code pieces --- .../org/jruby/javasupport/JavaMethod.java | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/jruby/javasupport/JavaMethod.java b/core/src/main/java/org/jruby/javasupport/JavaMethod.java index bd593e977c0..d89bbe9208e 100644 --- a/core/src/main/java/org/jruby/javasupport/JavaMethod.java +++ b/core/src/main/java/org/jruby/javasupport/JavaMethod.java @@ -152,7 +152,8 @@ public static JavaMethod getMatchingDeclaredMethod(Ruby runtime, Class javaCl // we're running under a restrictive security policy. try { return create(runtime, javaClass.getDeclaredMethod(methodName, argumentTypes)); - } catch (NoSuchMethodException e) { + } + catch (NoSuchMethodException e) { // search through all declared methods to find a closest match MethodSearch: for (Method method : javaClass.getDeclaredMethods()) { if (method.getName().equals(methodName)) { @@ -219,11 +220,11 @@ public RubyBoolean final_p() { @JRubyMethod(rest = true) public IRubyObject invoke(IRubyObject[] args) { checkArity(args.length - 1); - Object[] arguments = new Object[args.length - 1]; - convertArguments(args, arguments, 1); - IRubyObject invokee = args[0]; - if(invokee.isNil()) { + final IRubyObject invokee = args[0]; + final Object[] arguments = convertArguments(args, 1); + + if ( invokee.isNil() ) { return invokeWithExceptionHandling(method, null, arguments); } @@ -232,24 +233,23 @@ public IRubyObject invoke(IRubyObject[] args) { if (!isStatic()) { javaInvokee = JavaUtil.unwrapJavaValue(getRuntime(), invokee, "invokee not a java object"); - if (! method.getDeclaringClass().isInstance(javaInvokee)) { - throw getRuntime().newTypeError("invokee not instance of method's class (" + - "got" + javaInvokee.getClass().getName() + " wanted " + - method.getDeclaringClass().getName() + ")"); + if ( ! method.getDeclaringClass().isInstance(javaInvokee) ) { + throw getRuntime().newTypeError( + "invokee not instance of method's class" + + " (got" + javaInvokee.getClass().getName() + + " wanted " + method.getDeclaringClass().getName() + ")"); } // // this test really means, that this is a ruby-defined subclass of a java class // - if (javaInvokee instanceof InternalJavaProxy && - // don't bother to check if final method, it won't - // be there (not generated, can't be!) - !Modifier.isFinal(method.getModifiers())) { - JavaProxyClass jpc = ((InternalJavaProxy) javaInvokee) - .___getProxyClass(); - JavaProxyMethod jpm; - if ((jpm = jpc.getMethod(method.getName(), parameterTypes)) != null && - jpm.hasSuperImplementation()) { + if ( javaInvokee instanceof InternalJavaProxy && + // don't bother to check if final method, it won't + // be there (not generated, can't be!) + ! isFinal ) { + JavaProxyClass jpc = ((InternalJavaProxy) javaInvokee).___getProxyClass(); + JavaProxyMethod jpm = jpc.getMethod( method.getName(), parameterTypes ); + if ( jpm != null && jpm.hasSuperImplementation() ) { return invokeWithExceptionHandling(jpm.getSuperMethod(), javaInvokee, arguments); } } @@ -260,9 +260,8 @@ public IRubyObject invoke(IRubyObject[] args) { @JRubyMethod(rest = true) public IRubyObject invoke_static(IRubyObject[] args) { checkArity(args.length); - Object[] arguments = new Object[args.length]; - System.arraycopy(args, 0, arguments, 0, arguments.length); - convertArguments(args, arguments, 0); + + final Object[] arguments = convertArguments(args, 0); return invokeWithExceptionHandling(method, null, arguments); } @@ -537,11 +536,13 @@ private IRubyObject handlelIllegalArgumentEx(Method method, IllegalArgumentExcep iae.getMessage()); } - private void convertArguments(IRubyObject[] argsIn, Object[] argsOut, int from) { - Class[] types = parameterTypes; - for (int i = argsOut.length; --i >= 0; ) { - argsOut[i] = argsIn[i+from].toJava(types[i]); + private Object[] convertArguments(final IRubyObject[] args, int offset) { + final Object[] arguments = new Object[ args.length - offset ]; + final Class[] types = getParameterTypes(); + for ( int i = arguments.length; --i >= 0; ) { + arguments[i] = args[ i + offset ].toJava(types[i]); } + return arguments; } @Override From 679277d1ec3816552817363cc596f5ea86450b10 Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 13:02:31 +0100 Subject: [PATCH 10/15] help Java MethodInvoker's createCallable inline faster --- .../org/jruby/java/invokers/MethodInvoker.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/jruby/java/invokers/MethodInvoker.java b/core/src/main/java/org/jruby/java/invokers/MethodInvoker.java index 33f13bc0ca0..3ed475f1ed6 100644 --- a/core/src/main/java/org/jruby/java/invokers/MethodInvoker.java +++ b/core/src/main/java/org/jruby/java/invokers/MethodInvoker.java @@ -9,33 +9,34 @@ import org.jruby.javasupport.JavaMethod; public abstract class MethodInvoker extends RubyToJavaInvoker { + MethodInvoker(RubyModule host, List methods) { super(host, methods.toArray(new Method[methods.size()])); trySetAccessible(getAccessibleObjects()); } MethodInvoker(RubyModule host, Method method) { - super(host, new Method[] {method}); + super(host, new Method[] { method }); trySetAccessible(getAccessibleObjects()); } @Override - protected JavaCallable createCallable(Ruby ruby, Member member) { - return JavaMethod.create(ruby, (Method)member); + protected final JavaCallable createCallable(Ruby runtime, Member member) { + return JavaMethod.create(runtime, (Method) member); } @Override - protected JavaCallable[] createCallableArray(JavaCallable callable) { - return new JavaMethod[] {(JavaMethod)callable}; + protected final JavaCallable[] createCallableArray(JavaCallable callable) { + return new JavaMethod[] { (JavaMethod) callable }; } @Override - protected JavaCallable[] createCallableArray(int size) { + protected final JavaCallable[] createCallableArray(int size) { return new JavaMethod[size]; } @Override - protected JavaCallable[][] createCallableArrayArray(int size) { + protected final JavaCallable[][] createCallableArrayArray(int size) { return new JavaMethod[size][]; } From 342571abf759ef0159f2d02bb33baf1b8efc2ddd Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 13:03:45 +0100 Subject: [PATCH 11/15] avoid redundant array instance creations in Java method invoker impls --- .../java/org/jruby/java/invokers/InstanceMethodInvoker.java | 4 ++-- .../java/org/jruby/java/invokers/SingletonMethodInvoker.java | 4 ++-- .../java/org/jruby/java/invokers/StaticMethodInvoker.java | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/jruby/java/invokers/InstanceMethodInvoker.java b/core/src/main/java/org/jruby/java/invokers/InstanceMethodInvoker.java index 60666e97deb..56bcc3e6040 100644 --- a/core/src/main/java/org/jruby/java/invokers/InstanceMethodInvoker.java +++ b/core/src/main/java/org/jruby/java/invokers/InstanceMethodInvoker.java @@ -3,7 +3,6 @@ import java.lang.reflect.Method; import java.util.List; -import org.jruby.Ruby; import org.jruby.RubyModule; import org.jruby.RubyProc; import org.jruby.java.proxies.JavaProxy; @@ -26,7 +25,7 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz JavaProxy proxy = castJavaProxy(self); int len = args.length; - Object[] convertedArgs = new Object[len]; + final Object[] convertedArgs; JavaMethod method = (JavaMethod)findCallable(self, name, args, len); if (method.isVarArgs()) { len = method.getParameterTypes().length - 1; @@ -82,6 +81,7 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz return method.invokeDirect(proxy.getObject(), cArg0, cArg1, cArg2); } + @Override public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) { if (block.isGiven()) { JavaProxy proxy = castJavaProxy(self); diff --git a/core/src/main/java/org/jruby/java/invokers/SingletonMethodInvoker.java b/core/src/main/java/org/jruby/java/invokers/SingletonMethodInvoker.java index b64a22e7a39..b4dbe7ec187 100644 --- a/core/src/main/java/org/jruby/java/invokers/SingletonMethodInvoker.java +++ b/core/src/main/java/org/jruby/java/invokers/SingletonMethodInvoker.java @@ -3,7 +3,6 @@ import java.lang.reflect.Method; import java.util.List; -import org.jruby.Ruby; import org.jruby.RubyClass; import org.jruby.RubyModule; import org.jruby.RubyProc; @@ -28,7 +27,7 @@ public SingletonMethodInvoker(Object singleton, RubyClass host, Method method) { @Override public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args) { int len = args.length; - Object[] convertedArgs = new Object[len]; + final Object[] convertedArgs; JavaMethod method = (JavaMethod)findCallable(self, name, args, len); if (method.isVarArgs()) { len = method.getParameterTypes().length - 1; @@ -85,6 +84,7 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz return method.invokeDirect(singleton, cArg0, cArg1, cArg2); } + @Override public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) { if (block.isGiven()) { int len = args.length; diff --git a/core/src/main/java/org/jruby/java/invokers/StaticMethodInvoker.java b/core/src/main/java/org/jruby/java/invokers/StaticMethodInvoker.java index d5304e3234a..514c2862872 100644 --- a/core/src/main/java/org/jruby/java/invokers/StaticMethodInvoker.java +++ b/core/src/main/java/org/jruby/java/invokers/StaticMethodInvoker.java @@ -3,7 +3,6 @@ import java.lang.reflect.Method; import java.util.List; -import org.jruby.Ruby; import org.jruby.RubyClass; import org.jruby.RubyModule; import org.jruby.RubyProc; @@ -13,6 +12,7 @@ import org.jruby.runtime.builtin.IRubyObject; public class StaticMethodInvoker extends MethodInvoker { + public StaticMethodInvoker(RubyClass host, List methods) { super(host, methods); } @@ -24,7 +24,7 @@ public StaticMethodInvoker(RubyClass host, Method method) { @Override public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args) { int len = args.length; - Object[] convertedArgs = new Object[len]; + final Object[] convertedArgs; JavaMethod method = (JavaMethod)findCallable(self, name, args, len); if (method.isVarArgs()) { len = method.getParameterTypes().length - 1; @@ -81,6 +81,7 @@ public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule claz return method.invokeStaticDirect(cArg0, cArg1, cArg2); } + @Override public IRubyObject call(ThreadContext context, IRubyObject self, RubyModule clazz, String name, IRubyObject[] args, Block block) { if (block.isGiven()) { int len = args.length; From 0f3c822900e7c9037700ddd518983f768daab6a0 Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 13:05:19 +0100 Subject: [PATCH 12/15] actually use the runtime field we cache & hide it since it's never used in sub-classes --- .../java/invokers/RubyToJavaInvoker.java | 62 +++++++++---------- 1 file changed, 30 insertions(+), 32 deletions(-) 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 8c69b16772f..4d1df102931 100644 --- a/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java +++ b/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java @@ -25,14 +25,19 @@ public abstract class RubyToJavaInvoker extends JavaMethod { protected static final Object[] EMPTY_OBJECT_ARRAY = new Object[0]; + protected final JavaCallable javaCallable; protected final JavaCallable[][] javaCallables; protected final JavaCallable[] javaVarargsCallables; protected final int minVarargsArity; - protected final Map cache; - protected final Ruby runtime; - private Member[] members; - + + // initialize cache of parameter types to method + // FIXME: No real reason to use CHM, is there? + protected final Map cache = new ConcurrentHashMap(0, 0.75f, 1); + + private final Ruby runtime; + private final Member[] members; + RubyToJavaInvoker(RubyModule host, Member[] members) { super(host, Visibility.PUBLIC, CallConfiguration.FrameNoneScopeNone); this.members = members; @@ -40,14 +45,12 @@ public abstract class RubyToJavaInvoker extends JavaMethod { // we set all Java methods to optional, since many/most have overloads setArity(Arity.OPTIONAL); - Ruby runtime = host.getRuntime(); - // initialize all the callables for this method JavaCallable callable = null; JavaCallable[][] callables = null; JavaCallable[] varargsCallables = null; int varargsArity = Integer.MAX_VALUE; - + if (members.length == 1) { callable = createCallable(runtime, members[0]); if (callable.isVarArgs()) { @@ -65,7 +68,7 @@ public abstract class RubyToJavaInvoker extends JavaMethod { methodsForArity = new ArrayList(); methodsMap.put(currentArity,methodsForArity); } - JavaCallable javaMethod = createCallable(runtime,method); + JavaCallable javaMethod = createCallable(runtime, method); methodsForArity.add(javaMethod); if (isMemberVarArgs(method)) { @@ -88,17 +91,12 @@ public abstract class RubyToJavaInvoker extends JavaMethod { varargsMethods.toArray(varargsCallables); } } - members = null; - - // initialize cache of parameter types to method - // FIXME: No real reason to use CHM, is there? - cache = new ConcurrentHashMap(0, 0.75f, 1); this.javaCallable = callable; this.javaCallables = callables; this.javaVarargsCallables = varargsCallables; this.minVarargsArity = varargsArity; - + // if it's not overloaded, set up a NativeCall if (javaCallable != null) { // no constructor support yet @@ -203,27 +201,27 @@ protected JavaCallable findCallable(IRubyObject self, String name, IRubyObject[] if (javaVarargsCallables != null) { callable = CallableSelector.matchingCallableArityN(runtime, cache, javaVarargsCallables, args, arity); if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, javaVarargsCallables, (Object[])args); + throw CallableSelector.argTypesDoNotMatch(runtime, self, javaVarargsCallables, (Object[])args); } return callable; } else { - throw self.getRuntime().newArgumentError(args.length, javaCallables.length - 1); + throw runtime.newArgumentError(args.length, javaCallables.length - 1); } } callable = CallableSelector.matchingCallableArityN(runtime, cache, callablesForArity, args, arity); if (callable == null && javaVarargsCallables != null) { callable = CallableSelector.matchingCallableArityN(runtime, cache, javaVarargsCallables, args, arity); if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, javaVarargsCallables, (Object[])args); + throw CallableSelector.argTypesDoNotMatch(runtime, self, javaVarargsCallables, (Object[])args); } return callable; } if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, callablesForArity, (Object[])args); + throw CallableSelector.argTypesDoNotMatch(runtime, self, callablesForArity, (Object[])args); } } else { if (!callable.isVarArgs() && callable.getParameterTypes().length != args.length) { - throw self.getRuntime().newArgumentError(args.length, callable.getParameterTypes().length); + throw runtime.newArgumentError(args.length, callable.getParameterTypes().length); } } return callable; @@ -240,7 +238,7 @@ protected JavaCallable findCallableArityZero(IRubyObject self, String name) { callable = callablesForArity[0]; } else { if (callable.getParameterTypes().length != 0) { - throw self.getRuntime().newArgumentError(0, callable.getParameterTypes().length); + throw runtime.newArgumentError(0, callable.getParameterTypes().length); } } return callable; @@ -252,15 +250,15 @@ protected JavaCallable findCallableArityOne(IRubyObject self, String name, IRuby // TODO: varargs? JavaCallable[] callablesForArity = null; if (javaCallables.length <= 1 || (callablesForArity = javaCallables[1]) == null) { - throw self.getRuntime().newArgumentError(1, javaCallables.length - 1); + throw runtime.newArgumentError(1, javaCallables.length - 1); } callable = CallableSelector.matchingCallableArityOne(runtime, cache, callablesForArity, arg0); if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, callablesForArity, arg0); + throw CallableSelector.argTypesDoNotMatch(runtime, self, callablesForArity, arg0); } } else { if (callable.getParameterTypes().length != 1) { - throw self.getRuntime().newArgumentError(1, callable.getParameterTypes().length); + throw runtime.newArgumentError(1, callable.getParameterTypes().length); } } return callable; @@ -272,15 +270,15 @@ protected JavaCallable findCallableArityTwo(IRubyObject self, String name, IRuby // TODO: varargs? JavaCallable[] callablesForArity = null; if (javaCallables.length <= 2 || (callablesForArity = javaCallables[2]) == null) { - throw self.getRuntime().newArgumentError(2, javaCallables.length - 1); + throw runtime.newArgumentError(2, javaCallables.length - 1); } callable = CallableSelector.matchingCallableArityTwo(runtime, cache, callablesForArity, arg0, arg1); if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, callablesForArity, arg0, arg1); + throw CallableSelector.argTypesDoNotMatch(runtime, self, callablesForArity, arg0, arg1); } } else { if (callable.getParameterTypes().length != 2) { - throw self.getRuntime().newArgumentError(2, callable.getParameterTypes().length); + throw runtime.newArgumentError(2, callable.getParameterTypes().length); } } return callable; @@ -292,15 +290,15 @@ protected JavaCallable findCallableArityThree(IRubyObject self, String name, IRu // TODO: varargs? JavaCallable[] callablesForArity = null; if (javaCallables.length <= 3 || (callablesForArity = javaCallables[3]) == null) { - throw self.getRuntime().newArgumentError(3, javaCallables.length - 1); + throw runtime.newArgumentError(3, javaCallables.length - 1); } callable = CallableSelector.matchingCallableArityThree(runtime, cache, callablesForArity, arg0, arg1, arg2); if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, callablesForArity, arg0, arg1, arg2); + throw CallableSelector.argTypesDoNotMatch(runtime, self, callablesForArity, arg0, arg1, arg2); } } else { if (callable.getParameterTypes().length != 3) { - throw self.getRuntime().newArgumentError(3, callable.getParameterTypes().length); + throw runtime.newArgumentError(3, callable.getParameterTypes().length); } } return callable; @@ -312,15 +310,15 @@ protected JavaCallable findCallableArityFour(IRubyObject self, String name, IRub // TODO: varargs? JavaCallable[] callablesForArity = null; if (javaCallables.length <= 4 || (callablesForArity = javaCallables[4]) == null) { - throw self.getRuntime().newArgumentError(4, javaCallables.length - 1); + throw runtime.newArgumentError(4, javaCallables.length - 1); } callable = CallableSelector.matchingCallableArityFour(runtime, cache, callablesForArity, arg0, arg1, arg2, arg3); if (callable == null) { - throw CallableSelector.argTypesDoNotMatch(self.getRuntime(), self, callablesForArity, arg0, arg1, arg2, arg3); + throw CallableSelector.argTypesDoNotMatch(runtime, self, callablesForArity, arg0, arg1, arg2, arg3); } } else { if (callable.getParameterTypes().length != 4) { - throw self.getRuntime().newArgumentError(4, callable.getParameterTypes().length); + throw runtime.newArgumentError(4, callable.getParameterTypes().length); } } return callable; From 875b25a13fb3c197c98682faa5d36602bc8027b7 Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 14:27:59 +0100 Subject: [PATCH 13/15] IntHashMap's transients are confusing - left-overs from copying from HashMap impl --- .../jruby/util/collections/IntHashMap.java | 78 +++++++++---------- 1 file changed, 37 insertions(+), 41 deletions(-) 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 d6537ce2d0e..5928817e78a 100644 --- a/core/src/main/java/org/jruby/util/collections/IntHashMap.java +++ b/core/src/main/java/org/jruby/util/collections/IntHashMap.java @@ -8,24 +8,23 @@ import java.util.Set; public class IntHashMap { - - private transient Entry[] table; - - private transient int count; - + + private Entry[] table; + private int count; + transient volatile Set keySet = null; transient volatile Collection values = null; private int threshold; - + private final float loadFactor; - + public static class Entry { final int hash; final int key; V value; Entry next; - + protected Entry(int hash, int key, V value, Entry next) { this.hash = hash; this.key = key; @@ -41,15 +40,16 @@ public V getValue() { return value; } } - + public IntHashMap() { this(20, 0.75f); } - + public IntHashMap(int initialCapacity) { this(initialCapacity, 0.75f); } - + + @SuppressWarnings("unchecked") public IntHashMap(int initialCapacity, float loadFactor) { super(); if (initialCapacity < 0) { @@ -58,15 +58,11 @@ public IntHashMap(int initialCapacity, float loadFactor) { if (loadFactor <= 0) { throw new IllegalArgumentException("Illegal Load: " + loadFactor); } - if (initialCapacity == 0) { - initialCapacity = 1; - } - + if (initialCapacity == 0) initialCapacity = 1; + this.loadFactor = loadFactor; this.threshold = (int) (initialCapacity * loadFactor); - @SuppressWarnings("unchecked") - Entry[] table = new Entry[initialCapacity]; - this.table = table; + this.table = new Entry[initialCapacity]; } public int size() { @@ -76,12 +72,12 @@ public int size() { public boolean isEmpty() { return count == 0; } - + public boolean contains(Object value) { if (value == null) { throw new NullPointerException(); } - + Entry tab[] = table; for (int i = tab.length; i-- > 0;) { for (Entry e = tab[i]; e != null; e = e.next) { @@ -92,11 +88,11 @@ public boolean contains(Object value) { } return false; } - + public boolean containsValue(Object value) { return contains(value); } - + public boolean containsKey(int key) { Entry[] tab = table; int hash = key; @@ -108,7 +104,7 @@ public boolean containsKey(int key) { } return false; } - + public V get(int key) { Entry[] tab = table; int hash = key; @@ -120,23 +116,23 @@ public V get(int key) { } return null; } - + protected void rehash() { int oldCapacity = table.length; Entry[] oldMap = table; - + int newCapacity = oldCapacity * 2 + 1; @SuppressWarnings("unchecked") Entry[] newMap = new Entry[newCapacity]; - + threshold = (int) (newCapacity * loadFactor); table = newMap; - + for (int i = oldCapacity; i-- > 0;) { for (Entry old = oldMap[i]; old != null;) { Entry e = old; old = old.next; - + int index = (e.hash & 0x7FFFFFFF) % newCapacity; e.next = newMap[index]; newMap[index] = e; @@ -154,7 +150,7 @@ Entry getEntry(int key) { } } return null; - } + } public V put(int key, V value) { // Makes sure the key is not already in the hashtable. @@ -168,22 +164,22 @@ public V put(int key, V value) { return old; } } - + if (count >= threshold) { // Rehash the table if the threshold is exceeded rehash(); - + tab = table; index = (hash & 0x7FFFFFFF) % tab.length; } - + // Creates the new entry. Entry e = new Entry(hash, key, value, tab[index]); tab[index] = e; count++; return null; } - + public V remove(int key) { Entry[] tab = table; int hash = key; @@ -203,7 +199,7 @@ public V remove(int key) { } return null; } - + public synchronized void clear() { Entry[] tab = table; for (int index = tab.length; --index >= 0;) { @@ -211,7 +207,7 @@ public synchronized void clear() { } count = 0; } - + private abstract class HashIterator implements Iterator { Entry next; // next entry to return int index; // current slot @@ -292,7 +288,7 @@ public Set keySet() { } private class KeySet extends AbstractSet { - + public Iterator iterator() { return newKeyIterator(); } @@ -326,7 +322,7 @@ public Collection values() { } private class Values extends AbstractCollection { - + public Iterator iterator() { return newValueIterator(); } @@ -352,7 +348,7 @@ public Set> entrySet() { } private class EntrySet extends AbstractSet> { - + public Iterator> iterator() { return newEntryIterator(); } @@ -382,7 +378,7 @@ public void clear() { IntHashMap.this.clear(); } } - + @Override public String toString() { Iterator> i = entrySet().iterator(); @@ -398,7 +394,7 @@ public String toString() { sb.append(value == this ? "(this IntHashMap)" : value); if (! i.hasNext()) return sb.append('}').toString(); sb.append(", "); - } + } } - + } From 45f9045f2e867a3011267989601f86e06cce80e2 Mon Sep 17 00:00:00 2001 From: kares Date: Mon, 2 Mar 2015 17:09:35 +0100 Subject: [PATCH 14/15] minor (micro-bm) improvements in RubyToJavaInvoker's consturctor - less allocations - use IntHashMap (internally) to avoid integer "boxing" - instantiate varargs list on demand if it's needed --- .../java/invokers/RubyToJavaInvoker.java | 100 +++++++++--------- 1 file changed, 50 insertions(+), 50 deletions(-) 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 4d1df102931..d7d799c4cb4 100644 --- a/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java +++ b/core/src/main/java/org/jruby/java/invokers/RubyToJavaInvoker.java @@ -7,7 +7,6 @@ import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -22,13 +21,13 @@ import org.jruby.runtime.Arity; import org.jruby.runtime.Visibility; import org.jruby.runtime.builtin.IRubyObject; +import org.jruby.util.collections.IntHashMap; public abstract class RubyToJavaInvoker extends JavaMethod { - protected static final Object[] EMPTY_OBJECT_ARRAY = new Object[0]; - protected final JavaCallable javaCallable; - protected final JavaCallable[][] javaCallables; - protected final JavaCallable[] javaVarargsCallables; + protected final JavaCallable javaCallable; /* null if multiple callable members */ + protected final JavaCallable[][] javaCallables; /* != null if javaCallable == null */ + protected final JavaCallable[] javaVarargsCallables; /* != null if any var args callables */ protected final int minVarargsArity; // initialize cache of parameter types to method @@ -46,49 +45,54 @@ public abstract class RubyToJavaInvoker extends JavaMethod { setArity(Arity.OPTIONAL); // initialize all the callables for this method - JavaCallable callable = null; - JavaCallable[][] callables = null; + final JavaCallable callable; + final JavaCallable[][] callables; JavaCallable[] varargsCallables = null; int varargsArity = Integer.MAX_VALUE; if (members.length == 1) { callable = createCallable(runtime, members[0]); - if (callable.isVarArgs()) { + if ( callable.isVarArgs() ) { varargsCallables = createCallableArray(callable); } - } else { - Map> methodsMap = new HashMap>(); - List varargsMethods = new ArrayList(); + callables = null; + } + else { + callable = null; + + IntHashMap> arityMap = new IntHashMap>(members.length); + ArrayList varArgs = null; int maxArity = 0; - for (Member method: members) { - int currentArity = getMemberParameterTypes(method).length; + for ( final Member method : members ) { + final int currentArity = getMemberParameterTypes(method).length; maxArity = Math.max(currentArity, maxArity); - List methodsForArity = methodsMap.get(currentArity); + + List methodsForArity = arityMap.get(currentArity); if (methodsForArity == null) { - methodsForArity = new ArrayList(); - methodsMap.put(currentArity,methodsForArity); + methodsForArity = new ArrayList(8); + arityMap.put(currentArity, methodsForArity); } - JavaCallable javaMethod = createCallable(runtime, method); + + final JavaCallable javaMethod = createCallable(runtime, method); methodsForArity.add(javaMethod); - if (isMemberVarArgs(method)) { + if ( isMemberVarArgs(method) ) { varargsArity = Math.min(currentArity - 1, varargsArity); - varargsMethods.add(javaMethod); + if (varArgs == null) varArgs = new ArrayList(); + varArgs.add(javaMethod); } } callables = createCallableArrayArray(maxArity + 1); - for (Map.Entry> entry : methodsMap.entrySet()) { - List methodsForArity = (List)entry.getValue(); + for (IntHashMap.Entry> entry : arityMap.entrySet()) { + List methodsForArity = entry.getValue(); JavaCallable[] methodsArray = methodsForArity.toArray(createCallableArray(methodsForArity.size())); - callables[((Integer)entry.getKey()).intValue()] = methodsArray; + callables[ entry.getKey() ] = methodsArray; } - if (varargsMethods.size() > 0) { - // have at least one varargs, build that map too - varargsCallables = createCallableArray(varargsMethods.size()); - varargsMethods.toArray(varargsCallables); + if (varArgs != null /* && varargsMethods.size() > 0 */) { + varargsCallables = varArgs.toArray( createCallableArray(varArgs.size()) ); } } @@ -101,38 +105,34 @@ public abstract class RubyToJavaInvoker extends JavaMethod { if (javaCallable != null) { // no constructor support yet if (javaCallable instanceof org.jruby.javasupport.JavaMethod) { - org.jruby.javasupport.JavaMethod javaMethod = (org.jruby.javasupport.JavaMethod)javaCallable; - Method method = (Method)javaMethod.getValue(); - // only public, since non-public don't bind - if (Modifier.isPublic(method.getModifiers()) && Modifier.isPublic(method.getDeclaringClass().getModifiers())) { - setNativeCall(method.getDeclaringClass(), method.getName(), method.getReturnType(), method.getParameterTypes(), Modifier.isStatic(method.getModifiers()), true); - } + setNativeCallIfPublic( (Method) ((org.jruby.javasupport.JavaMethod) javaCallable).getValue() ); } - } else { - // use the lowest-arity non-overload + } else { // use the lowest-arity non-overload for (JavaCallable[] callablesForArity : javaCallables) { - if (callablesForArity != null - && callablesForArity.length == 1 - && callablesForArity[0] instanceof org.jruby.javasupport.JavaMethod) { - Method method = (Method)((org.jruby.javasupport.JavaMethod)callablesForArity[0]).getValue(); - // only public, since non-public don't bind - if (Modifier.isPublic(method.getModifiers()) && Modifier.isPublic(method.getDeclaringClass().getModifiers())) { - setNativeCall(method.getDeclaringClass(), method.getName(), method.getReturnType(), method.getParameterTypes(), Modifier.isStatic(method.getModifiers()), true); - } + if ( callablesForArity == null || callablesForArity.length != 1 ) continue; + if ( callablesForArity[0] instanceof org.jruby.javasupport.JavaMethod ) { + setNativeCallIfPublic( (Method) ((org.jruby.javasupport.JavaMethod) callablesForArity[0]).getValue() ); } } } } - protected Member[] getMembers() { + private void setNativeCallIfPublic(final Method method) { + final int mod = method.getModifiers(); // only public, since non-public don't bind + if ( Modifier.isPublic(mod) && Modifier.isPublic(method.getDeclaringClass().getModifiers()) ) { + setNativeCall(method.getDeclaringClass(), method.getName(), method.getReturnType(), method.getParameterTypes(), Modifier.isStatic(mod), true); + } + } + + protected final Member[] getMembers() { return members; } protected AccessibleObject[] getAccessibleObjects() { - return (AccessibleObject[])getMembers(); + return (AccessibleObject[]) getMembers(); } - protected abstract JavaCallable createCallable(Ruby ruby, Member member); + protected abstract JavaCallable createCallable(Ruby runtime, Member member); protected abstract JavaCallable[] createCallableArray(JavaCallable callable); @@ -185,12 +185,12 @@ static void trySetAccessible(AccessibleObject[] accObjs) { } void raiseNoMatchingCallableError(String name, IRubyObject proxy, Object... args) { - int len = args.length; - Class[] argTypes = new Class[args.length]; - for (int i = 0; i < len; i++) { + final int len = args.length; + Class[] argTypes = new Class[len]; + for ( int i = 0; i < len; i++ ) { argTypes[i] = args[i].getClass(); } - throw proxy.getRuntime().newArgumentError("no " + name + " with arguments matching " + Arrays.toString(argTypes) + " on object " + proxy.getMetaClass()); + throw runtime.newArgumentError("no " + name + " with arguments matching " + Arrays.toString(argTypes) + " on object " + proxy.getMetaClass()); } protected JavaCallable findCallable(IRubyObject self, String name, IRubyObject[] args, int arity) { @@ -233,7 +233,7 @@ protected JavaCallable findCallableArityZero(IRubyObject self, String name) { // TODO: varargs? JavaCallable[] callablesForArity = null; if (javaCallables.length == 0 || (callablesForArity = javaCallables[0]) == null) { - raiseNoMatchingCallableError(name, self, EMPTY_OBJECT_ARRAY); + raiseNoMatchingCallableError(name, self); } callable = callablesForArity[0]; } else { From e61b737724dbe90bebc46f71fb4cc80e964a6768 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Mon, 2 Mar 2015 11:23:04 -0600 Subject: [PATCH 15/15] Only consider breaks that originated from this proc's frame. The old logic here was turning all breaks that pass through an escaped proc into LongJumpError, even if they originated in a different frame/proc that could still validly handle the break. This commit modifies the logic to ignore the exception altogether (re-throwing it) if it is not associated with this proc/frame (by checking jumpTarget). Fixes #1270 --- core/src/main/java/org/jruby/RubyProc.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyProc.java b/core/src/main/java/org/jruby/RubyProc.java index d520c2aa230..94518c73a16 100644 --- a/core/src/main/java/org/jruby/RubyProc.java +++ b/core/src/main/java/org/jruby/RubyProc.java @@ -298,15 +298,17 @@ public IRubyObject call(ThreadContext context, IRubyObject[] args, IRubyObject s } private IRubyObject handleBreakJump(Ruby runtime, Block newBlock, JumpException.BreakJump bj, int jumpTarget) { - switch(newBlock.type) { - case LAMBDA: - if (bj.getTarget() == jumpTarget) return (IRubyObject) bj.getValue(); - - throw runtime.newLocalJumpError(RubyLocalJumpError.Reason.BREAK, (IRubyObject)bj.getValue(), "unexpected break"); - case PROC: - if (newBlock.isEscaped()) throw runtime.newLocalJumpError(RubyLocalJumpError.Reason.BREAK, (IRubyObject)bj.getValue(), "break from proc-closure"); + if (bj.getTarget() == jumpTarget) { + switch (newBlock.type) { + case LAMBDA: + return (IRubyObject) bj.getValue(); + case PROC: + if (newBlock.isEscaped()) { + throw runtime.newLocalJumpError(RubyLocalJumpError.Reason.BREAK, (IRubyObject) bj.getValue(), "break from proc-closure"); + } + } } - + throw bj; }