[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491470 ] Bret Weinraub commented on OPENJPA-219: --- The no-leak version is "cache v3" although that's a misnomer ; there's no cache involved. VM - Version ms Sun - Original 333 Sun - Cache v1 83 Sun - Cache v2 40 Sun - Cache v3 73 JRockit - Original 176 JRockit - Cache v1 72 JRockit - Cache v2 22 JRockit - Cache v3 52 > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Assigned To: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219-NoLeak.patch, OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491458 ] Patrick Linskey commented on OPENJPA-219: - How does this compare to the leaky-patch version? > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219-NoLeak.patch, OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491316 ] Patrick Linskey commented on OPENJPA-219: - > another way to approach this would be not to cache at all, > and just iterate over getDeclaredFields/Methods() looking for a match Good point. That would certainly be a better approach. It looks like Class.getDeclaredFields() will be a bit slower, since more memory copying happens (the array is copied before being returned -- bummer that those APIs don't use collections), but that might not be noticeable in the benchmark. > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491310 ] Abe White commented on OPENJPA-219: --- 1. Yes, clearly we'd use our org.apache.openjpa.lib.util.concurrent. ConcurrentReferenceHashMap with weak keys to hold the Classes. 2. We have no evidence that a positive cache would consume any more memory than a negative cache in this case. A positive cache's entry size for a given class is limited by the number of fields/methods in that class. A negative cache's entry size is limited only by how many nonexistent field/method names we look for. The negative cache will probably be smaller in this case except in deep inheritance hierarchies because I don't think we look for field or method names outside the inheritance hierarchy, but (a) I'm not sure of that and (b) there's no guarantee that will always be the case. 3. You know, another way to approach this would be not to cache at all, and just iterate over getDeclaredFields/Methods() looking for a match rather than using the singular getDeclaredField/Method() that throws an exception. If that gives decent performance, we wouldn't have to worry about the memory consumption and complication of caching. Bret, do you think you could try that out and see how it does? > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491104 ] Patrick Linskey commented on OPENJPA-219: - I agree about the mem leak problem in the patch that I created; hence my comments in the original post about using weak references. There are a number of ways that you could solve this caching problem, such as making all / part of Reflection non-static and setting up per-Configuration caches, or using weak references in the cache, or even (and probably least attractively) creating undeploy APIs. Abe said: > If we're going to cache, I don't see why we wouldn't cache the declared > fields/methods rather than the nonexistent ones. It would be a simple > matter of keeping a Class->Set cache, where the Set is the names of > the fields/methods returned by Class.getDeclaredFields/Methods(). > Then we'd have both a positive (set.contains(x)) and negative > (!set.contains(x)) cache. As I mentioned in the original description, the bug here is that negative lookups are slow because of the exception creation. I see no reason to add extra caching if we don't know if we need it, although Bret's numbers potentially indicate that there is a benefit to positive caching. I have no problem with having a positive cache, but I think that we should only include it if it's going to help, since otherwise it'll just be one more thing contributing to memory consumption. > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491078 ] Bret Weinraub commented on OPENJPA-219: --- I will add the "Cache v2" version here shortly. > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491077 ] Bret Weinraub commented on OPENJPA-219: --- I developed a patch version that cached both "success" and "failure" cases - here are some of the performance numbers I generated: VM Implementation Benchmark Rate (ms) Sun 1 - Original333 Sun 2 - Cache v183 Sun 3 - Cache v240 JRockit 1 - Original176 JRockit 2 - Cache v172 JRockit 3 - Cache v222 Where Original : recent svn version Cache V1: patch above plus a couple of minor tweaks Cache V2: caches both success and failure calls thereby minimizing calls to Reflection API. Adding a boolean that says "use cache" or not would be simple. Anyway let me add that I/we feel that permanently caching Class objects would create memory leak problems in the case where a ClassLoader were to be thrown away. Holding the class Object in a hash will keep the ClassLoader tree from being garbage collected ; we see this anti-pattern frequently, it is pathalogical, and can eventually lead to OOME for operations that recycle class loaders. An example would be repeatedly deploying the same application to an application server, such as done during iterative development of WebApps/EnterpriseApps. So I/we believe to be critical to create a mechanism whereby these Class object were not held in a static context. If this code were associated with an object then when the object was GC-ed the Class references would be eliminated therey by allowing the class loader to be GC-ed. > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491038 ] Abe White commented on OPENJPA-219: --- If we're going to cache, I don't see why we wouldn't cache the declared fields/methods rather than the nonexistent ones. It would be a simple matter of keeping a Class->Set cache, where the Set is the names of the fields/methods returned by Class.getDeclaredFields/Methods(). Then we'd have both a positive (set.contains(x)) and negative (!set.contains(x)) cache. I'd also like to see some explicit control over whether requests to find a field/method cause cache additions. It might be that we always want caching now, but it's such a general class that I can imagine naïvely using the Reflection class's helper methods on one-off random field/method requests in the future without remembering the memory consequences. If all use results in caching, that could be bad. So I'd like us to either add separate caching methods or a boolean parameter to the existing methods. And either way, we need to revisit our current use of Reflection to decide which version to use in the current code. > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > Attachments: OPENJPA-219.patch > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-219) Reflection: negative caching would be beneficial in redeployment scenarios
[ https://issues.apache.org/jira/browse/OPENJPA-219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489536 ] Patrick Linskey commented on OPENJPA-219: - Note that we invoke Class.getDeclaredField() (and Class.getDeclaredMethod()) in a number of places. We should take care to replace all the uses of that method with our optimized version. > Reflection: negative caching would be beneficial in redeployment scenarios > -- > > Key: OPENJPA-219 > URL: https://issues.apache.org/jira/browse/OPENJPA-219 > Project: OpenJPA > Issue Type: Bug > Components: kernel >Affects Versions: 0.9.0, 0.9.6, 0.9.7 >Reporter: Patrick Linskey > Fix For: 0.9.8 > > > In a variety of situations, OpenJPA searches class hierarchies for fields. > These searches invoke Class.getDeclaredField() in order to find non-public > fields. This method throws an exception when it fails to find the specified > field, and the exception creation is, as ever, slow. > It would be useful to create a static (and thus per-classloader) > Map,Collection> of fields known not to be > available in a given class. > It may also be beneficial to maintain a cache of which fields *are* present > in a given class, but this issue is being filed as a result of a demonstrated > performance hit during deployment due to the lack of a negative cache. If we > obtain quantitative data indicating that a positive cache would be useful, we > might want to implement such a thing at that time. > Trace 3 (2115 occurances): [excepti][00090] java/lang/NoSuchFieldException: > domainName > at > java/lang/Class.getDeclaredField(Ljava/lang/String;I)Ljava/lang/reflect/Field;(Unknown > Source) > at > org/apache/openjpa/enhance/Reflection.findField(Ljava/lang/Class;Ljava/lang/String;Z)Ljava/lang/reflect/Field;(Reflection.java:101) > at > org/apache/openjpa/util/ApplicationIds.toPKValues(Ljava/lang/Object;Lorg/apache/openjpa/meta/ClassMetaData;)[Ljava/lang/Object; > (ApplicationIds.java:89) -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
