Re: Loading classes with many methods is very expensive
Hi all, I revamped the Class.getMethods() implementation so that it is faster for common cases and O(n) at the same time, which makes Martin's test happy (at least in part that measures getMethods() speed - the class loading / linkage in VM is a separate issue). With the following test that loads all classes from rt.jar and calls getMethods() on each of them: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java And system property 'sun.reflect.noCaches=true' (so that we exercise the logic in every loop - not just 1st), original code prints: 19657 classes loaded in 1.987373401 seconds. 494141 methods obtained in 1.02493941 seconds. 494141 methods obtained in 0.905235658 seconds. 494141 methods obtained in 0.914434303 seconds. 494141 methods obtained in 0.887212805 seconds. 494141 methods obtained in 0.888929483 seconds. 494141 methods obtained in 0.883309141 seconds. 494141 methods obtained in 0.88341098 seconds. 494141 methods obtained in 0.897397146 seconds. 494141 methods obtained in 0.885677466 seconds. 494141 methods obtained in 0.895834176 seconds. Patched code does the same about 10% faster: 19657 classes loaded in 2.084409717 seconds. 494124 methods obtained in 0.915928578 seconds. 494124 methods obtained in 0.785342465 seconds. 494124 methods obtained in 0.784852619 seconds. 494124 methods obtained in 0.793450205 seconds. 494124 methods obtained in 0.849915078 seconds. 494124 methods obtained in 0.77835511 seconds. 494124 methods obtained in 0.764144701 seconds. 494124 methods obtained in 0.754122383 seconds. 494124 methods obtained in 0.747961897 seconds. 494124 methods obtained in 0.7489937 seconds. Martin's test prints on my computer with original code the following: Base class load time: 177.80 ms getDeclaredMethods: Methods: 65521, Total time: 35.79 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 50.15 ms, Time per method: 0.0008 ms Derived class load time: 34015.70 ms getDeclaredMethods: Methods: 65521, Total time: 33.82 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 8122.00 ms, Time per method: 0.1239 ms And with patched code this: Base class load time: 157.16 ms getDeclaredMethods: Methods: 65521, Total time: 65.77 ms, Time per method: 0.0010 ms getMethods: Methods: 65530, Total time: 44.64 ms, Time per method: 0.0007 ms Derived class load time: 33996.69 ms getDeclaredMethods: Methods: 65521, Total time: 32.63 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 92.12 ms, Time per method: 0.0014 ms Here's a webrev of the patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.01/ Patched code is simpler (65 lines gone) and I hope, easier to understand and change (I think a change in spec is coming in JDK9 which will handle abstract interface methods the same way as default, right Joel?) I also took the liberty to eliminate some redundant array and Field/Method/Constructor copies. get[Method0,Field0,Counstuctor0] now return 'root' objects. Copying is performed in methods that call them and expose the objects to any code outside java.lang.Class. Also, findFields() and findMethods() don't do copying of Field/Method objects themselves, but rather live that to methods that call them. getInterfaces() method now delegates to getInterfaces(boolean copyArray) so that internally, not array copying is performed. All 55 java/lang/reflect jtreg tests pass with this patch. Regards, Peter
Re: Lower overhead String encoding/decoding
Hi Alan, I think this looks better but I have a few comments on the API. Thanks for taking the time to look at this - most appreciated. I've pushed the latest iteration to http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-8/. For String(ByteBuffer, Charset) then it's still inconsistent to read from the buffer position but not advance it. I think the constructor needs to work like a regular decode in that respect. Related to this is the underflow case where there are insufficient remaining bytes to complete. If you don't advance the position then there is no way to detect this. I've modified the Javadoc and tests. The implementation was actually behaving this way already. The statement about the length of the String .. may not be equal to the length of the subarray might be there from a previous iteration. There isn't any array in the method signature so I think this statement needs to be make a bit clearer. I've rephrased this to refer to the ByteBuffer. For getBytes(byte[], int, int, Charset) then I think the javadoc could say a bit more. It will encode to a maximum of destBuffer.length - destOffset for example. The @return should probably say that it's the number of bytes written to the array rather than copied. Minor nits is that you probably don't want the starting p. Also the finals in the signature seem noisy/not needed. The getBytes(ByteBuffer, Charset) method needs a bit more javadoc. You should be able to borrow from text from the CharsetEncoder#encode methods to help with that. I've updated the Javadoc with more information about the encoding and made these changes. I'm not sure if there's anything else that's missing in this case. regards, Richard Warburton http://insightfullogic.com @RichardWarburto http://twitter.com/richardwarburto
Re: Loading classes with many methods is very expensive
On 10/26/2014 11:21 PM, Stanimir Simeonoff wrote: Great effort. From first glance: the hashCode and equals of MethodList use m.getParameterTypes() which is cloned. I'd rather pay the collision costs and rely on the default hashCode/equals of Method itself (hashCode ignores the parameters). Possibly hashCode can include the returnType but equals should be direct call to Method.equals. Can't do that as Method.equals also compares declaringClass, which is not part of method signature. We could use SharedSecrets to access the internal array of parameterTypes directly. Regards, Peter Stanimir On Sun, Oct 26, 2014 at 10:25 PM, Peter Levart peter.lev...@gmail.com mailto:peter.lev...@gmail.com wrote: Hi all, I revamped the Class.getMethods() implementation so that it is faster for common cases and O(n) at the same time, which makes Martin's test happy (at least in part that measures getMethods() speed - the class loading / linkage in VM is a separate issue). With the following test that loads all classes from rt.jar and calls getMethods() on each of them: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java And system property 'sun.reflect.noCaches=true' (so that we exercise the logic in every loop - not just 1st), original code prints: 19657 classes loaded in 1.987373401 seconds. 494141 methods obtained in 1.02493941 seconds. 494141 methods obtained in 0.905235658 seconds. 494141 methods obtained in 0.914434303 seconds. 494141 methods obtained in 0.887212805 seconds. 494141 methods obtained in 0.888929483 seconds. 494141 methods obtained in 0.883309141 seconds. 494141 methods obtained in 0.88341098 seconds. 494141 methods obtained in 0.897397146 seconds. 494141 methods obtained in 0.885677466 seconds. 494141 methods obtained in 0.895834176 seconds. Patched code does the same about 10% faster: 19657 classes loaded in 2.084409717 tel:2.084409717 seconds. 494124 methods obtained in 0.915928578 seconds. 494124 methods obtained in 0.785342465 seconds. 494124 methods obtained in 0.784852619 seconds. 494124 methods obtained in 0.793450205 seconds. 494124 methods obtained in 0.849915078 seconds. 494124 methods obtained in 0.77835511 seconds. 494124 methods obtained in 0.764144701 seconds. 494124 methods obtained in 0.754122383 seconds. 494124 methods obtained in 0.747961897 seconds. 494124 methods obtained in 0.7489937 seconds. Martin's test prints on my computer with original code the following: Base class load time: 177.80 ms getDeclaredMethods: Methods: 65521, Total time: 35.79 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 50.15 ms, Time per method: 0.0008 ms Derived class load time: 34015.70 ms getDeclaredMethods: Methods: 65521, Total time: 33.82 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 8122.00 ms, Time per method: 0.1239 ms And with patched code this: Base class load time: 157.16 ms getDeclaredMethods: Methods: 65521, Total time: 65.77 ms, Time per method: 0.0010 ms getMethods: Methods: 65530, Total time: 44.64 ms, Time per method: 0.0007 ms Derived class load time: 33996.69 ms getDeclaredMethods: Methods: 65521, Total time: 32.63 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 92.12 ms, Time per method: 0.0014 ms Here's a webrev of the patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.01/ http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/Class.getMethods/webrev.01/ Patched code is simpler (65 lines gone) and I hope, easier to understand and change (I think a change in spec is coming in JDK9 which will handle abstract interface methods the same way as default, right Joel?) I also took the liberty to eliminate some redundant array and Field/Method/Constructor copies. get[Method0,Field0,Counstuctor0] now return 'root' objects. Copying is performed in methods that call them and expose the objects to any code outside java.lang.Class. Also, findFields() and findMethods() don't do copying of Field/Method objects themselves, but rather live that to methods that call them. getInterfaces() method now delegates to getInterfaces(boolean copyArray) so that internally, not array copying is performed. All 55 java/lang/reflect jtreg tests pass with this patch. Regards, Peter
Re: Loading classes with many methods is very expensive
On Mon, Oct 27, 2014 at 12:36 AM, Peter Levart peter.lev...@gmail.com wrote: On 10/26/2014 11:21 PM, Stanimir Simeonoff wrote: Great effort. From first glance: the hashCode and equals of MethodList use m.getParameterTypes() which is cloned. I'd rather pay the collision costs and rely on the default hashCode/equals of Method itself (hashCode ignores the parameters). Possibly hashCode can include the returnType but equals should be direct call to Method.equals. Can't do that as Method.equals also compares declaringClass, which is not part of method signature. We could use SharedSecrets to access the internal array of parameterTypes directly. Opps, very true. If SharedSecrets is not available, get the parameters in the constructor, so clone happens once only. Method.clone() should be comparable to the cost of a LHM node, so the overhead is not so high. I doubt the JIT can EA clone() which would be the best case scenario. I was wondering if the computation of size is better than just array copy (as LHM.iterator is effectively linked list), however for small number of methods it would keep all the references in L1 so that would be better in the common case. Stanimir Regards, Peter Stanimir On Sun, Oct 26, 2014 at 10:25 PM, Peter Levart peter.lev...@gmail.com wrote: Hi all, I revamped the Class.getMethods() implementation so that it is faster for common cases and O(n) at the same time, which makes Martin's test happy (at least in part that measures getMethods() speed - the class loading / linkage in VM is a separate issue). With the following test that loads all classes from rt.jar and calls getMethods() on each of them: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/GetAllRtMethods.java And system property 'sun.reflect.noCaches=true' (so that we exercise the logic in every loop - not just 1st), original code prints: 19657 classes loaded in 1.987373401 seconds. 494141 methods obtained in 1.02493941 seconds. 494141 methods obtained in 0.905235658 seconds. 494141 methods obtained in 0.914434303 seconds. 494141 methods obtained in 0.887212805 seconds. 494141 methods obtained in 0.888929483 seconds. 494141 methods obtained in 0.883309141 seconds. 494141 methods obtained in 0.88341098 seconds. 494141 methods obtained in 0.897397146 seconds. 494141 methods obtained in 0.885677466 seconds. 494141 methods obtained in 0.895834176 seconds. Patched code does the same about 10% faster: 19657 classes loaded in 2.084409717 seconds. 494124 methods obtained in 0.915928578 seconds. 494124 methods obtained in 0.785342465 seconds. 494124 methods obtained in 0.784852619 seconds. 494124 methods obtained in 0.793450205 seconds. 494124 methods obtained in 0.849915078 seconds. 494124 methods obtained in 0.77835511 seconds. 494124 methods obtained in 0.764144701 seconds. 494124 methods obtained in 0.754122383 seconds. 494124 methods obtained in 0.747961897 seconds. 494124 methods obtained in 0.7489937 seconds. Martin's test prints on my computer with original code the following: Base class load time: 177.80 ms getDeclaredMethods: Methods: 65521, Total time: 35.79 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 50.15 ms, Time per method: 0.0008 ms Derived class load time: 34015.70 ms getDeclaredMethods: Methods: 65521, Total time: 33.82 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 8122.00 ms, Time per method: 0.1239 ms And with patched code this: Base class load time: 157.16 ms getDeclaredMethods: Methods: 65521, Total time: 65.77 ms, Time per method: 0.0010 ms getMethods: Methods: 65530, Total time: 44.64 ms, Time per method: 0.0007 ms Derived class load time: 33996.69 ms getDeclaredMethods: Methods: 65521, Total time: 32.63 ms, Time per method: 0.0005 ms getMethods: Methods: 65530, Total time: 92.12 ms, Time per method: 0.0014 ms Here's a webrev of the patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.01/ Patched code is simpler (65 lines gone) and I hope, easier to understand and change (I think a change in spec is coming in JDK9 which will handle abstract interface methods the same way as default, right Joel?) I also took the liberty to eliminate some redundant array and Field/Method/Constructor copies. get[Method0,Field0,Counstuctor0] now return 'root' objects. Copying is performed in methods that call them and expose the objects to any code outside java.lang.Class. Also, findFields() and findMethods() don't do copying of Field/Method objects themselves, but rather live that to methods that call them. getInterfaces() method now delegates to getInterfaces(boolean copyArray) so that internally, not array copying is performed. All 55 java/lang/reflect jtreg tests pass with this patch. Regards, Peter
Re: Loading classes with many methods is very expensive
On 10/26/2014 09:25 PM, Peter Levart wrote: 19657 classes loaded in 1.987373401 seconds. 494141 methods obtained in 1.02493941 seconds. vs. 19657 classes loaded in 2.084409717 seconds. 494124 methods obtained in 0.915928578 seconds. Hi, As you might have noticed, the number of methods obtained from patched code differed from original code. I have investigated this and found that original code treats abstract class methods the same as abstract interface methods as far as multiple inheritance is concerned (it keeps them together in the returned array). So I fixed this and here's new webrev which behaves the same as original code: http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/ Comparing original vs. patched code still shows speed-up: Original: 19657 classes loaded in 1.980493029 seconds. 494141 methods obtained in 0.976318927 seconds. 494141 methods obtained in 0.886504437 seconds. 494141 methods obtained in 0.911153722 seconds. 494141 methods obtained in 0.880550509 seconds. 494141 methods obtained in 0.875526704 seconds. 494141 methods obtained in 0.877258894 seconds. 494141 methods obtained in 0.871794344 seconds. 494141 methods obtained in 0.884159644 seconds. 494141 methods obtained in 0.892648522 seconds. 494141 methods obtained in 0.884581841 seconds. Patched: 19657 classes loaded in 2.055697675 seconds. 494141 methods obtained in 0.853922188 seconds. 494141 methods obtained in 0.776203794 seconds. 494141 methods obtained in 0.858774803 seconds. 494141 methods obtained in 0.778178867 seconds. 494141 methods obtained in 0.760043997 seconds. 494141 methods obtained in 0.756352444 seconds. 494141 methods obtained in 0.740826372 seconds. 494141 methods obtained in 0.744264782 seconds. 494141 methods obtained in 0.73805894 seconds. 494141 methods obtained in 0.746852752 seconds. 55 java/lang/reflect jtreg tests still pass. As they did before, which means that we don't have a coverage for such cases. I'll see where I can add such a case (EnumSet for example, which inherits from Set interface and AbstractColection class via two different paths, so Set.size()/iterator() and AbstractCollection.size()/iterator() are both returned from getMethods())... Regards, Peter
Re: Loading classes with many methods is very expensive
55 java/lang/reflect jtreg tests still pass. As they did before, which means that we don't have a coverage for such cases. I'll see where I can add such a case (EnumSet for example, which inherits from Set interface and AbstractColection class via two different paths, so Set.size()/iterator() and AbstractCollection.size()/iterator() are both returned from getMethods())... Reminds me the case when a package private class implementing a public interface cannot have its methods invoked via object.getClass().getMethod(name, classes).invoke(object, args) outside the package. Also I can't find any spec. on how multiple equivalent methods should be returned. Regards, Peter
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 2)
Style nit: all the
int cache[]
should be
int[] cache
Also not clear if 8061651-lookup-index-open-v2 is latest webrev ??
Thanks,
David
On 25/10/2014 9:38 AM, Ioi Lam wrote:
Hi Mandy,
Thanks for the review. please see comments in-line
On 10/24/14, 2:33 PM, Mandy Chung wrote:
On 10/23/2014 9:34 PM, Ioi Lam wrote:
Hi Mandy,
Thanks for the review. I have updated the patch:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v2/
Thanks for removing the LoaderType enum. Two questions -
do you need the new hasLookupCacheLoader variable? Should
lookupCAcheURLs be always be non-null if it has the lookup
cache?
I removed hasLookupCacheLoader and changed the condition check to
(lookupCacheURLs != null)
Most methods referencinglookupCacheURLs and lookupCacheLoader
are synchronized except initLookupCache and knowToNotExist.
Should they? Or volatile?
I changed both to synchronized.
On 10/21/14, 12:56 PM, Mandy Chung wrote:
line 398: what happens if loaders.size() maxindex? Shouldn't
it return null?
In this case, all the loaders that's needed by the cache[] have been
opened. So we should return cache[].
I forget about that, sorry.I'm thinking how to make it more
obvious to those who doesn't know much of the details. Every time I
read this and I need to reload what I know about and miss something.
I changed the code to this. What do you think?
It seems to me that it would help if you refactor and add a method
ensureLoadersInited that will make it clear what it attempts to do.
The side effect invalidating the cache can be checked after it's
called and no need to worry about lookupCacheEnabled must be checked
in any order. Something like this
if (cache != null cache.length 0) {
int maxindex = cache[cache.length - 1];
ensureLoaderOpened(maxindex);
if (loaders.get(maxindex) == null || !lookupCacheEnabled) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println(Expanded loaders FAILED +
loaders.size() + for maxindex= +
maxindex);
}
return null;
}
...
}
return cache;
I changed the code to
private synchronized int[] getLookupCache(String name) {
if (lookupCacheURLs == null || !lookupCacheEnabled) {
return null;
}
int cache[] = getLookupCacheForClassLoader(lookupCacheLoader,
name);
if (cache != null cache.length 0) {
int maxindex = cache[cache.length - 1]; // cache[] is
strictly ascending.
if (!ensureLoaderOpened(maxindex)) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println(Expanded loaders FAILED +
loaders.size() + for
maxindex= + maxindex);
}
return null;
}
}
return cache;
}
private boolean ensureLoaderOpened(int index) {
if (loaders.size() = index) {
// Open all Loaders up to, and including, index
if (getLoader(index) == null) {
return false;
}
if (!lookupCacheEnabled) {
// cache was invalidated as the result of the above call.
return false;
}
if (DEBUG_LOOKUP_CACHE) {
System.out.println(Expanded loaders + loaders.size() +
to index= + index);
}
}
return true;
}
The code is getting further complicated with this lookup cache and
bear with me for being picky.
if (loaders.size() = maxindex) {
boolean failed = false;
// Call getNextLoader() with a null cache to open all
// Loaders up to, and including, maxindex.
if (getNextLoader(null, maxindex) == null) {
Can this call getLoader(maxindex)? They are essentially the same.
I think getLoader(maxindex) makes it explicit that it forces to
open the loader.
Changed.
Mandy
