Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/16/2013 11:44 PM, Alan Bateman wrote: On 13/06/2013 12:47, Aleksey Shipilev wrote: Friendly reminder for the reviewers. On 06/10/2013 07:53 PM, Aleksey Shipilev wrote: This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ The current webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ I'm just catching up on this thread, webrev.02 looks very good to me. I can sponsor it. Thanks! I'll appreciate this. One comment on ClassRepositoryHolder is that it's yet another class (we have a proliferation of holder classes since we started using this idiom). It makes me wonder if it might be better to move the constant to ClassRepository. Ok, makes sense. I moved it to ClassRepository itself. Given we have the ClassRepository field in Class, the static initialization will be performed on the first class load. I have no problems having it public, since it is protected enough from the external modifications. The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.03/ Testing: - Linux/x86_64/release build OK - Linux/x86_64/release passes java/lang/reflect jtreg - microbenchmark scores are still intact -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On Jun 17, 2013, at 10:06 AM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 06/16/2013 11:44 PM, Alan Bateman wrote: On 13/06/2013 12:47, Aleksey Shipilev wrote: Friendly reminder for the reviewers. On 06/10/2013 07:53 PM, Aleksey Shipilev wrote: This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ The current webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ I'm just catching up on this thread, webrev.02 looks very good to me. I can sponsor it. Thanks! I'll appreciate this. One comment on ClassRepositoryHolder is that it's yet another class (we have a proliferation of holder classes since we started using this idiom). It makes me wonder if it might be better to move the constant to ClassRepository. Ok, makes sense. I moved it to ClassRepository itself. Given we have the ClassRepository field in Class, the static initialization will be performed on the first class load. I have no problems having it public, since it is protected enough from the external modifications. The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.03/ Looks good to me. Paul. Testing: - Linux/x86_64/release build OK - Linux/x86_64/release passes java/lang/reflect jtreg - microbenchmark scores are still intact -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/17/2013 10:06 AM, Aleksey Shipilev wrote: On 06/16/2013 11:44 PM, Alan Bateman wrote: On 13/06/2013 12:47, Aleksey Shipilev wrote: Friendly reminder for the reviewers. On 06/10/2013 07:53 PM, Aleksey Shipilev wrote: This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ The current webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ I'm just catching up on this thread, webrev.02 looks very good to me. I can sponsor it. Thanks! I'll appreciate this. One comment on ClassRepositoryHolder is that it's yet another class (we have a proliferation of holder classes since we started using this idiom). It makes me wonder if it might be better to move the constant to ClassRepository. Ok, makes sense. I moved it to ClassRepository itself. Given we have the ClassRepository field in Class, the static initialization will be performed on the first class load. I have no problems having it public, since it is protected enough from the external modifications. Sorry, Aleksey, just a nit: In getInterfaces(), the method could be written using a single volatile read from ReflectionData.interfaces field instead of two (by introducing local variable). Regards, Peter The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.03/ Testing: - Linux/x86_64/release build OK - Linux/x86_64/release passes java/lang/reflect jtreg - microbenchmark scores are still intact -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 17/06/2013 09:06, Aleksey Shipilev wrote: : Ok, makes sense. I moved it to ClassRepository itself. Given we have the ClassRepository field in Class, the static initialization will be performed on the first class load. I have no problems having it public, since it is protected enough from the external modifications. The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.03/ This looks to good. I agree with Peter's point about the 2 reads in getInterfaces() although it's unlikely to make a significant difference. If you can create the change-set then I can push this for you today. -Alan
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/17/2013 02:20 PM, Peter Levart wrote: Sorry, Aleksey, just a nit: In getInterfaces(), the method could be written using a single volatile read from ReflectionData.interfaces field instead of two (by introducing local variable). Yes, thanks! Nothing to be sorry of, I'll cover this. -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 17/06/2013 13:32, Aleksey Shipilev wrote: On 06/17/2013 03:44 PM, Alan Bateman wrote: This looks to good. I agree with Peter's point about the 2 reads in getInterfaces() although it's unlikely to make a significant difference. Oh, it can have the difference on non-x86 hardware and/or in optimized code. This nit is fixed here: http://cr.openjdk.java.net/~shade/8016236/webrev.04/ I did the same testing as before: Linux x86_64/release builds and runs jdk/test/java/lang/reflect jtreg successfully; the performance is still there. If you can create the change-set then I can push this for you today. Here: http://cr.openjdk.java.net/~shade/8016236/8016236.changeset Although only Alan counts as official reviewer; do we need another one? The updated webrev looks good for me. For the jdk repository then only one reviewer with reviewer role is required. I see you have Peter and Paul listed in a Reviewed-by line so I think we are good. Do you want Doug listed too as this started out as a patch from Doug. -Alan
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/17/2013 05:22 PM, Alan Bateman wrote: For the jdk repository then only one reviewer with reviewer role is required. I see you have Peter and Paul listed in a Reviewed-by line so I think we are good. Do you want Doug listed too as this started out as a patch from Doug. Ouch! My bad. Doug had not reviewed the final patch, and the final patch is significantly different from the original one. Do we still want to put Doug to Contributed-by, or is there some other tag useful in cases like these? -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/17/13 09:25, Aleksey Shipilev wrote: Ouch! My bad. Doug had not reviewed the final patch, and the final patch is significantly different from the original one. Do we still want to put Doug to Contributed-by, or is there some other tag useful in cases like these? It looks fine to me. Thanks for the thorough care and testing. I don't think we distinguish idea-cotributed-by vs code-contributed-by :-) -Doug
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/17/2013 05:33 PM, Doug Lea wrote: On 06/17/13 09:25, Aleksey Shipilev wrote: Ouch! My bad. Doug had not reviewed the final patch, and the final patch is significantly different from the original one. Do we still want to put Doug to Contributed-by, or is there some other tag useful in cases like these? It looks fine to me. Thanks for the thorough care and testing. I don't think we distinguish idea-cotributed-by vs code-contributed-by :-) Thanks! I had updated the changeset capturing both: http://cr.openjdk.java.net/~shade/8016236/8016236.changeset -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 13/06/2013 12:47, Aleksey Shipilev wrote: Friendly reminder for the reviewers. On 06/10/2013 07:53 PM, Aleksey Shipilev wrote: This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ The current webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ I'm just catching up on this thread, webrev.02 looks very good to me. I can sponsor it. One comment on ClassRepositoryHolder is that it's yet another class (we have a proliferation of holder classes since we started using this idiom). It makes me wonder if it might be better to move the constant to ClassRepository. -Alan.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 10/06/2013 21:57, Peter Levart wrote: : I should note that ReflectionData is invalidated when the class is redefined. I don't know if generic signature is one of those things that can change with class redefinition, but invalidation is just one purpose of ReflectionData and the other is caching (via SoftReference), so generic signature can be added to it for that purpose. If 'genericSignature' is cached on ReflectionData then it would be consistent to also cache the derived 'genericInfo', what do you think? A redefine or retransform is allowed to change attributes so while signatures/modifier/hierarchy can't change then the Signature attribute might. So I think it's right in the current webrev (but good to think about such things). -Alan.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Friendly reminder for the reviewers. On 06/10/2013 07:53 PM, Aleksey Shipilev wrote: This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ The current webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ Notable differences from Doug's version are: - handle non-generic cases as well - reuse ReflectionData to cache the interfaces - code style (chained ternary operators blown up) - fixes the race along the way http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6398355 Testing: - Linux x86_64/release: build OK - Linux x86_64/release: java/lang/reflect regression tests OK - Microbenchmarks show whooping increase in performance, see below Thanks, Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On Jun 10, 2013, at 7:20 PM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: Thanks for taking a look, Paul! Can I count your review as official? Unfortunately i am not an unofficial reviewer so can only be counted in the unofficial category for now :-( On 06/10/2013 08:44 PM, Paul Sandoz wrote: - reuse ReflectionData to cache the interfaces and generic signatures Any guess on the size impact due to those new fields in ReflectionData? Seems to be additional 16 bytes (boolean+oop+oop+alignment, see below); again, since the ReflectionData is lazily-constructed, this is not the issue for most of the classes. In EE-land a lot more reflection tends to happen on application classes. I see Peter has some ideas to avoid additional fields with a special lazilly initialized ClassRepository instance. Paul. Before: Running 64-bit HotSpot VM. Using compressed references with 3-bit shift. Objects are 8 bytes aligned. java.lang.Class.ReflectionData offset size type description 012 (assumed to be the object header) 12 4 int ReflectionData.redefinedCount 16 4 Field[] ReflectionData.declaredFields 20 4 Field[] ReflectionData.publicFields 24 4 Method[] ReflectionData.declaredMethods 28 4 Method[] ReflectionData.publicMethods 32 4 Constructor[] ReflectionData.declaredConstructors 36 4 Constructor[] ReflectionData.publicConstructors 40 4 Field[] ReflectionData.declaredPublicFields 44 4 Method[] ReflectionData.declaredPublicMethods 48 (object boundary, size estimate) After: Running 64-bit HotSpot VM. Using compressed references with 3-bit shift. Objects are 16 bytes aligned. java.lang.Class.ReflectionData offset size type description 012 (assumed to be the object header) 12 4 int ReflectionData.redefinedCount 16 1 boolean ReflectionData.genericSignatureResolved 17 3 (alignment/padding gap) 20 4 Field[] ReflectionData.declaredFields 24 4 Field[] ReflectionData.publicFields 28 4 Method[] ReflectionData.declaredMethods 32 4 Method[] ReflectionData.publicMethods 36 4 Constructor[] ReflectionData.declaredConstructors 40 4 Constructor[] ReflectionData.publicConstructors 44 4 Field[] ReflectionData.declaredPublicFields 48 4 Method[] ReflectionData.declaredPublicMethods 52 4 Class[] ReflectionData.interfaces 56 4String ReflectionData.genericSignature 60 4 (loss due to the next object alignment) 64 (object boundary, size estimate)
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/11/2013 01:31 AM, Peter Levart wrote: While you're at it adding getInterfaces() cache, why not also getSuperclass()? Is it maybe already intrinsified? Yes, the C2 code and benchmarking shows it is already an intrinsic. -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Hi Peter, On 06/11/2013 12:57 AM, Peter Levart wrote: When 'sun.reflect.noCaches=true' system property is defined, reflectionData() returns null. In that case, the code should cope without using ReflectionData (no caching). See other uses of reflectionData() (for example: in privateGetDeclaredFields())... Good catch. Indeed, I was deluged by newReflectionData returning null. Fixed. If 'genericSignature' is cached on ReflectionData then it would be consistent to also cache the derived 'genericInfo', what do you think? genericSignature is off the ReflectionData now, so it makes a little sense to move genericInfo there. The interference of genericInfo and class redefinition is an interesting in itself, and I think it should be taken care of elsewhere (notably, when JEP-159 lands). I'm somewhat resistant to do this along with the performance fix. If there was a singleton ClassRepository NONE instance, then event the boolean flag wouldn't be needed. Oh, that's a good idea. Implemented. The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ Testing: - Linux x86_64/release builds OK - Linux x86_64/release jdk/test/java/lang/reflect/ jtreg OK - Microbenchmark scores are still showing the major increase - Layout dumps show Reflection data is 8 bytes more (1 oop + alignment) -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Aleksey, On 06/11/2013 11:56 AM, Aleksey Shipilev wrote: Hi Peter, On 06/11/2013 12:57 AM, Peter Levart wrote: When 'sun.reflect.noCaches=true' system property is defined, reflectionData() returns null. In that case, the code should cope without using ReflectionData (no caching). See other uses of reflectionData() (for example: in privateGetDeclaredFields())... Good catch. Indeed, I was deluged by newReflectionData returning null. Fixed. If 'genericSignature' is cached on ReflectionData then it would be consistent to also cache the derived 'genericInfo', what do you think? genericSignature is off the ReflectionData now, so it makes a little sense to move genericInfo there. The interference of genericInfo and class redefinition is an interesting in itself, and I think it should be taken care of elsewhere (notably, when JEP-159 lands). I'm somewhat resistant to do this along with the performance fix. If there was a singleton ClassRepository NONE instance, then event the boolean flag wouldn't be needed. Oh, that's a good idea. Implemented. The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ That's good now. I just wonder if renaming of native getGenericSignature - getGenericSignature0 is still needed now that there's no getGenericSignature non-native method in Class. Every native method ending with '0' has a non-native counterpart with same name (just '0' stripped). Regards, Peter Testing: - Linux x86_64/release builds OK - Linux x86_64/release jdk/test/java/lang/reflect/ jtreg OK - Microbenchmark scores are still showing the major increase - Layout dumps show Reflection data is 8 bytes more (1 oop + alignment) -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/11/2013 03:27 PM, Peter Levart wrote: The new webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.02/ That's good now. Thanks! I just wonder if renaming of native getGenericSignature - getGenericSignature0 is still needed now that there's no getGenericSignature non-native method in Class. Every native method ending with '0' has a non-native counterpart with same name (just '0' stripped). I do think that marking the native methods with the suffixes is a good thing, easing the future change in the internal APIs (e.g. introducing non-native getGenericSignature). I can revert this if anybody else thinks otherwise. -Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 2013-06-10, at 11:53 AM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3000.008 nsec/op Am I reading this correctly? 0.008 nsec/op means 125 times 10-to-the-9th ops per second. That's a high clock rate. Have most been optimized out, are we using parallelism to our advantage, what happened here? David
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/11/2013 07:58 PM, David Chase wrote: On 2013-06-10, at 11:53 AM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3000.008 nsec/op Am I reading this correctly? 0.008 nsec/op means 125 times 10-to-the-9th ops per second. Look up to the table header: that is the error margin. The result is 5.300 +- 0.008 nsec/op. That's a high clock rate. Yeah, that is why any benchmark rating lower than 1 ns/op is raising the red flag all over the performance guys. :) -Aleksey.
RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Hi, This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ Notable differences from Doug's version are: - handle non-generic cases as well - reuse ReflectionData to cache the interfaces and generic signatures - code style (chained ternary operators blown up) - fixes the race along the way http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6398355 Testing: - Linux x86_64/release: build OK - Linux x86_64/release: java/lang/reflect regression tests OK - Microbenchmarks show whooping increase in performance, see below (If there are no stylistic and other comments, I would like to do the JPRT submit and/or additional testing [which?]). The benchmark is here (you need JMH to build and run): http://cr.openjdk.java.net/~shade/8016236/classbench.zip Before: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 326.8351.220 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 308.7932.828 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 312.6271.637 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 216.9838.902 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 59.9330.183 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 65.4690.284 nsec/op Doug's version: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 15.1060.271 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3040.024 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.7390.045 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 213.8251.346 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 61.6510.394 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 64.3400.522 nsec/op After: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 14.9850.104 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3000.008 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.8740.175 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 23.9410.177 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 8.5830.044 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 12.4000.045 nsec/op Thanks, Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On Jun 10, 2013, at 5:53 PM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: Hi, This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ Notable differences from Doug's version are: - handle non-generic cases as well I was wondering about that case too. - reuse ReflectionData to cache the interfaces and generic signatures Any guess on the size impact due to those new fields in ReflectionData? - code style (chained ternary operators blown up) - fixes the race along the way http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6398355 Testing: - Linux x86_64/release: build OK - Linux x86_64/release: java/lang/reflect regression tests OK - Microbenchmarks show whooping increase in performance, see below Nice improvements. Paul. (If there are no stylistic and other comments, I would like to do the JPRT submit and/or additional testing [which?]). The benchmark is here (you need JMH to build and run): http://cr.openjdk.java.net/~shade/8016236/classbench.zip Before: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 326.8351.220 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 308.7932.828 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 312.6271.637 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 216.9838.902 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 59.9330.183 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 65.4690.284 nsec/op Doug's version: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 15.1060.271 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3040.024 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.7390.045 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 213.8251.346 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 61.6510.394 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 64.3400.522 nsec/op After: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 14.9850.104 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3000.008 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.8740.175 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 23.9410.177 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 8.5830.044 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 12.4000.045 nsec/op Thanks, Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Thanks for taking a look, Paul! Can I count your review as official? On 06/10/2013 08:44 PM, Paul Sandoz wrote: - reuse ReflectionData to cache the interfaces and generic signatures Any guess on the size impact due to those new fields in ReflectionData? Seems to be additional 16 bytes (boolean+oop+oop+alignment, see below); again, since the ReflectionData is lazily-constructed, this is not the issue for most of the classes. Before: Running 64-bit HotSpot VM. Using compressed references with 3-bit shift. Objects are 8 bytes aligned. java.lang.Class.ReflectionData offset size type description 012 (assumed to be the object header) 12 4 int ReflectionData.redefinedCount 16 4 Field[] ReflectionData.declaredFields 20 4 Field[] ReflectionData.publicFields 24 4 Method[] ReflectionData.declaredMethods 28 4 Method[] ReflectionData.publicMethods 32 4 Constructor[] ReflectionData.declaredConstructors 36 4 Constructor[] ReflectionData.publicConstructors 40 4 Field[] ReflectionData.declaredPublicFields 44 4 Method[] ReflectionData.declaredPublicMethods 48 (object boundary, size estimate) After: Running 64-bit HotSpot VM. Using compressed references with 3-bit shift. Objects are 16 bytes aligned. java.lang.Class.ReflectionData offset size type description 012 (assumed to be the object header) 12 4 int ReflectionData.redefinedCount 16 1 boolean ReflectionData.genericSignatureResolved 17 3 (alignment/padding gap) 20 4 Field[] ReflectionData.declaredFields 24 4 Field[] ReflectionData.publicFields 28 4 Method[] ReflectionData.declaredMethods 32 4 Method[] ReflectionData.publicMethods 36 4 Constructor[] ReflectionData.declaredConstructors 40 4 Constructor[] ReflectionData.publicConstructors 44 4 Field[] ReflectionData.declaredPublicFields 48 4 Method[] ReflectionData.declaredPublicMethods 52 4 Class[] ReflectionData.interfaces 56 4String ReflectionData.genericSignature 60 4 (loss due to the next object alignment) 64 (object boundary, size estimate)
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Hi Aleksey, When 'sun.reflect.noCaches=true' system property is defined, reflectionData() returns null. In that case, the code should cope without using ReflectionData (no caching). See other uses of reflectionData() (for example: in privateGetDeclaredFields())... I should note that ReflectionData is invalidated when the class is redefined. I don't know if generic signature is one of those things that can change with class redefinition, but invalidation is just one purpose of ReflectionData and the other is caching (via SoftReference), so generic signature can be added to it for that purpose. If 'genericSignature' is cached on ReflectionData then it would be consistent to also cache the derived 'genericInfo', what do you think? I don't think 3 fields are needed (genericSignatureResolved, genericSignature genericInfo). I think one boolean flag (say : genericInfoPresent) and genericInfo is all that is needed. If there was a singleton ClassRepository NONE instance, then event the boolean flag wouldn't be needed. Regards, Peter On 06/10/2013 05:53 PM, Aleksey Shipilev wrote: Hi, This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ Notable differences from Doug's version are: - handle non-generic cases as well - reuse ReflectionData to cache the interfaces and generic signatures - code style (chained ternary operators blown up) - fixes the race along the way http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6398355 Testing: - Linux x86_64/release: build OK - Linux x86_64/release: java/lang/reflect regression tests OK - Microbenchmarks show whooping increase in performance, see below (If there are no stylistic and other comments, I would like to do the JPRT submit and/or additional testing [which?]). The benchmark is here (you need JMH to build and run): http://cr.openjdk.java.net/~shade/8016236/classbench.zip Before: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 326.8351.220 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 308.7932.828 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 312.6271.637 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 216.9838.902 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 59.9330.183 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 65.4690.284 nsec/op Doug's version: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 15.1060.271 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3040.024 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.7390.045 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 213.8251.346 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 61.6510.394 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 64.3400.522 nsec/op After: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 14.9850.104 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3000.008 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.8740.175 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 23.9410.177 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 8.5830.044 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 12.4000.045 nsec/op Thanks, Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
Hi Aleksey, While you're at it adding getInterfaces() cache, why not also getSuperclass()? Is it maybe already intrinsified? Regards, Peter On 06/10/2013 05:53 PM, Aleksey Shipilev wrote: Hi, This is the follow-up on the issue Doug identified: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017798.html I had reworked the patch, webrev is here: http://cr.openjdk.java.net/~shade/8016236/webrev.01/ Notable differences from Doug's version are: - handle non-generic cases as well - reuse ReflectionData to cache the interfaces and generic signatures - code style (chained ternary operators blown up) - fixes the race along the way http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6398355 Testing: - Linux x86_64/release: build OK - Linux x86_64/release: java/lang/reflect regression tests OK - Microbenchmarks show whooping increase in performance, see below (If there are no stylistic and other comments, I would like to do the JPRT submit and/or additional testing [which?]). The benchmark is here (you need JMH to build and run): http://cr.openjdk.java.net/~shade/8016236/classbench.zip Before: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 326.8351.220 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 308.7932.828 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 312.6271.637 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 216.9838.902 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 59.9330.183 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 65.4690.284 nsec/op Doug's version: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 15.1060.271 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3040.024 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.7390.045 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 213.8251.346 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 61.6510.394 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 64.3400.522 nsec/op After: Benchmark Mode ThrCnt Sec Mean Mean errorUnits o.b.ClassBench.generic_getGenericInterfaces avgt 1 51 14.9850.104 nsec/op o.b.ClassBench.generic_getGenericSuperclass avgt 1 51 5.3000.008 nsec/op o.b.ClassBench.generic_getTypeParametersavgt 1 51 16.8740.175 nsec/op o.b.ClassBench.raw_getGenericInterfaces avgt 1 51 23.9410.177 nsec/op o.b.ClassBench.raw_getGenericSuperclass avgt 1 51 8.5830.044 nsec/op o.b.ClassBench.raw_getTypeParametersavgt 1 51 12.4000.045 nsec/op Thanks, Aleksey.
Re: RFR (S) CR 8016236: Class.getGenericInterfaces performance improvement
On 06/10/2013 10:57 PM, Peter Levart wrote:
I don't think 3 fields are needed (genericSignatureResolved,
genericSignature genericInfo). I think one boolean flag (say :
genericInfoPresent) and genericInfo is all that is needed. If there
was a singleton ClassRepository NONE instance, then even the boolean
flag wouldn't be needed.
Hi Aleksey,
Here's what I meant by above (no genericSignature caching is needed and
no additional fields):
private static class LazyHolder {
static final ClassRepository NULL_CLASS_REPOSITORY =
ClassRepository.make(Ljava/lang/Object;, null);
}
// accessor for generic info repository
private ClassRepository getGenericInfo() {
ClassRepository genericInfo = this.genericInfo;
// lazily initialize repository if necessary
if (genericInfo == null) {
String signature = getGenericSignature();
// create and cache generic info repository
this.genericInfo = genericInfo = (signature == null)
? LazyHolder.NULL_CLASS_REPOSITORY
: ClassRepository.make(signature, getFactory());
}
//return cached repository
return genericInfo == LazyHolder.NULL_CLASS_REPOSITORY
? null
: genericInfo;
}
Regards, Peter
