> ... > The performance penalty of wildfly not caching the implementation of that method in regards to > hibernate is that HibernatePersistenceProvider uses a cache for classes/methods/fields > and that cache is blown away on every call to getPersistenceProviders and that > happens on every validation of every member of every class. I agree with your interpretation.

Have you looked on the JPA spec mailing lists for clarification about whether the getPersistenceProviders result can be cached or not? I'll take a look to see if I can find anything. On 09/12/2014 11:59 AM, Andrew Schmidt wrote: > I've been investigating performance issues with wildfly and hibernate validator. > The changes made for this https://issues.jboss.org/browse/AS7-1306 imply the > follow from the jpa spec: > > The results of calling the PersistenceProviderResolverHolder.getPersistenceProviderResolver > and the PersistenceProviderResolver.getPersistenceProviders methods must not be cached. > > however, the spec says later on: > > Note that the PersistenceProviderResolver.getPersistenceProviders() method > can potentially be called many times. It is therefore recommended that the > implementation of this method make use of caching. > > My interpretation is that wildfly should be caching the providers and it's the > responsibility of the callers to not cache the results. So the issue AS7-1306 shouldn't > have been implemented. Good point that the JPA.next specification should pick one (allow caching or not).

> > The performance penalty of wildfly not caching the implementation of that method in regards to > hibernate is that HibernatePersistenceProvider uses a cache for classes/methods/fields > and that cache is blown away on every call to getPersistenceProviders and that > happens on every validation of every member of every class. The Hibernate implementation of these classes also do the same thing: https://github.com/hibernate/hibernate-jpa-api/blob/master/src/main/java/... The Hibernate PersistenceProviderResolverHolder does have a comment that caching should be introduced, whatever that means. > > _______________________________________________ > wildfly-dev mailing list > wildfly-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/wildfly-dev > _______________________________________________ wildfly-dev mailing list wildfly-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/wildfly-dev

My concern about caching the PersistenceProvider classes is that leaks in persistence providers versions that are already released (or not released but will not be fixed), will be more harmful. If the WildFly PersistenceProviderResolver implementation was the only implementor doing this, it would be safer to start caching by default (Hibernate + EclipseLink PersistenceProviderResolver impls do the same). We should default to not cache the PersistenceProvider instances for stability but allow for caching to be enabled, so that we *could* share the same PersistenceProvider instance across deployments as Andrew is asking for. Question is how to determine which PersistenceProvider class instances can be shared by multiple deployments without leaking memory. Do we need a system property setting or something in our configuration? If we use a boolean, it doesn't matter but if we have a list of PersistenceProvider class names, that can't be stored in the WildFly standalone.xml. One reason to make this a system property controlled setting is that we could later remove it if we determine that its hopeless (just don't ever cache) or there are no leaks (we could always cache). We also need more than to have no leaks in future persistence provider releases, we also need there to be no leaks in earlier releases that can also be used with WildFly. If anyone wants to contribute a classloader leak detector test to the WildFly testsuite (perhaps using the Eclipse MAT API), that would be helpful in ensuring that we don't cause a leak and can detect them in the future. Thoughts? Scott [1] https://eclipse.googlesource.com/eclipselink/javax.persistence/+/2.0.5.v2... On 09/12/2014 02:19 PM, Jason Greene wrote: > > On Sep 12, 2014, at 1:15 PM, Scott Marlow <smarlow(a)redhat.com&gt; wrote: > >> Have you looked on the JPA spec mailing lists for clarification about >> whether the getPersistenceProviders result can be cached or not? I'll >> take a look to see if I can find anything. >> >> On 09/12/2014 11:59 AM, Andrew Schmidt wrote: >>> I've been investigating performance issues with wildfly and hibernate validator. >>> The changes made for this https://issues.jboss.org/browse/AS7-1306 imply the >>> follow from the jpa spec: >>> >>> The results of calling the PersistenceProviderResolverHolder.getPersistenceProviderResolver >>> and the PersistenceProviderResolver.getPersistenceProviders methods must not be cached. >>> >>> however, the spec says later on: >>> >>> Note that the PersistenceProviderResolver.getPersistenceProviders() method >>> can potentially be called many times. It is therefore recommended that the >>> implementation of this method make use of caching. >>> >>> My interpretation is that wildfly should be caching the providers and it's the >>> responsibility of the callers to not cache the results. So the issue AS7-1306 shouldn't >>> have been implemented. >> >> Good point that the JPA.next specification should pick one (allow >> caching or not). > > I think the spec is consistent, but could be improved to avoid confusion. It says that callers of PersistenceProviderResolver should not cache the result, but PersistenceProviderResolver can internally cache itself. This makes sense because the container should be able to decide when to change the provider, and if the caller caches this wouldn’t take effect. > > >> >>> >>> The performance penalty of wildfly not caching the implementation of that method in regards to >>> hibernate is that HibernatePersistenceProvider uses a cache for classes/methods/fields >>> and that cache is blown away on every call to getPersistenceProviders and that >>> happens on every validation of every member of every class. >> >> The Hibernate implementation of these classes also do the same thing: >> >> https://github.com/hibernate/hibernate-jpa-api/blob/master/src/main/java/... >> >> The Hibernate PersistenceProviderResolverHolder does have a comment that >> caching should be introduced, whatever that means. >> >>> >>> _______________________________________________ >>> wildfly-dev mailing list >>> wildfly-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >>> >> >> _______________________________________________ >> wildfly-dev mailing list >> wildfly-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/wildfly-dev > > -- > Jason T. Greene > WildFly Lead / JBoss EAP Platform Architect > JBoss, a division of Red Hat > _______________________________________________ wildfly-dev mailing list wildfly-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/wildfly-dev

If you look at Andrew’s complaint though, he’s really talking about multiple calls in the *same* deployment.

On Sep 12, 2014, at 2:43 PM, Scott Marlow <smarlow(a)redhat.com&gt; wrote: > My concern about caching the PersistenceProvider classes is that leaks in persistence providers versions that are already released (or not released but will not be fixed), will be more harmful. > > If the WildFly PersistenceProviderResolver implementation was the only implementor doing this, it would be safer to start caching by default (Hibernate + EclipseLink PersistenceProviderResolver impls do the same). > > We should default to not cache the PersistenceProvider instances for stability but allow for caching to be enabled, so that we *could* share the same PersistenceProvider instance across deployments as Andrew is asking for. > > Question is how to determine which PersistenceProvider class instances can be shared by multiple deployments without leaking memory. Do we need a system property setting or something in our configuration? If we use a boolean, it doesn't matter but if we have a list of PersistenceProvider class names, that can't be stored in the WildFly standalone.xml. One reason to make this a system property controlled setting is that we could later remove it if we determine that its hopeless (just don't ever cache) or there are no leaks (we could always cache). > > We also need more than to have no leaks in future persistence provider releases, we also need there to be no leaks in earlier releases that can also be used with WildFly. > > If anyone wants to contribute a classloader leak detector test to the WildFly testsuite (perhaps using the Eclipse MAT API), that would be helpful in ensuring that we don't cause a leak and can detect them in the future. > > Thoughts? > > Scott > > [1] https://eclipse.googlesource.com/eclipselink/javax.persistence/+/2.0.5.v2... > > > On 09/12/2014 02:19 PM, Jason Greene wrote: >> >> On Sep 12, 2014, at 1:15 PM, Scott Marlow <smarlow(a)redhat.com&gt; wrote: >> >>> Have you looked on the JPA spec mailing lists for clarification about >>> whether the getPersistenceProviders result can be cached or not? I'll >>> take a look to see if I can find anything. >>> >>> On 09/12/2014 11:59 AM, Andrew Schmidt wrote: >>>> I've been investigating performance issues with wildfly and hibernate validator. >>>> The changes made for this https://issues.jboss.org/browse/AS7-1306 imply the >>>> follow from the jpa spec: >>>> >>>> The results of calling the PersistenceProviderResolverHolder.getPersistenceProviderResolver >>>> and the PersistenceProviderResolver.getPersistenceProviders methods must not be cached. >>>> >>>> however, the spec says later on: >>>> >>>> Note that the PersistenceProviderResolver.getPersistenceProviders() method >>>> can potentially be called many times. It is therefore recommended that the >>>> implementation of this method make use of caching. >>>> >>>> My interpretation is that wildfly should be caching the providers and it's the >>>> responsibility of the callers to not cache the results. So the issue AS7-1306 shouldn't >>>> have been implemented. >>> >>> Good point that the JPA.next specification should pick one (allow >>> caching or not). >> >> I think the spec is consistent, but could be improved to avoid confusion. It says that callers of PersistenceProviderResolver should not cache the result, but PersistenceProviderResolver can internally cache itself. This makes sense because the container should be able to decide when to change the provider, and if the caller caches this wouldn’t take effect. >> >> >>> >>>> >>>> The performance penalty of wildfly not caching the implementation of that method in regards to >>>> hibernate is that HibernatePersistenceProvider uses a cache for classes/methods/fields >>>> and that cache is blown away on every call to getPersistenceProviders and that >>>> happens on every validation of every member of every class. >>> >>> The Hibernate implementation of these classes also do the same thing: >>> >>> https://github.com/hibernate/hibernate-jpa-api/blob/master/src/main/java/... >>> >>> The Hibernate PersistenceProviderResolverHolder does have a comment that >>> caching should be introduced, whatever that means. >>> >>>> >>>> _______________________________________________ >>>> wildfly-dev mailing list >>>> wildfly-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >>>> >>> >>> _______________________________________________ >>> wildfly-dev mailing list >>> wildfly-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >> >> -- >> Jason T. Greene >> WildFly Lead / JBoss EAP Platform Architect >> JBoss, a division of Red Hat >> > -- Jason T. Greene WildFly Lead / JBoss EAP Platform Architect JBoss, a division of Red Hat

On 12 sept. 2014, at 22:37, Scott Marlow <smarlow(a)redhat.com&gt; wrote: > On 09/12/2014 04:19 PM, Jason Greene wrote: > If you look at Andrew’s complaint though, he’s really talking about multiple calls in the *same* deployment. Ahh, caching the list at the *same* deployment level, is safe. We can definitely do that. > >> On Sep 12, 2014, at 2:43 PM, Scott Marlow <smarlow(a)redhat.com&gt; wrote: >> >> My concern about caching the PersistenceProvider classes is that leaks in persistence providers versions that are already released (or not released but will not be fixed), will be more harmful. >> >> If the WildFly PersistenceProviderResolver implementation was the only implementor doing this, it would be safer to start caching by default (Hibernate + EclipseLink PersistenceProviderResolver impls do the same). >> >> We should default to not cache the PersistenceProvider instances for stability but allow for caching to be enabled, so that we *could* share the same PersistenceProvider instance across deployments as Andrew is asking for. >> >> Question is how to determine which PersistenceProvider class instances can be shared by multiple deployments without leaking memory. Do we need a system property setting or something in our configuration? If we use a boolean, it doesn't matter but if we have a list of PersistenceProvider class names, that can't be stored in the WildFly standalone.xml. One reason to make this a system property controlled setting is that we could later remove it if we determine that its hopeless (just don't ever cache) or there are no leaks (we could always cache). >> >> We also need more than to have no leaks in future persistence provider releases, we also need there to be no leaks in earlier releases that can also be used with WildFly. >> >> If anyone wants to contribute a classloader leak detector test to the WildFly testsuite (perhaps using the Eclipse MAT API), that would be helpful in ensuring that we don't cause a leak and can detect them in the future. >> >> Thoughts? >> >> Scott >> >> [1] https://eclipse.googlesource.com/eclipselink/javax.persistence/+/2.0.5.v2... >> >> >>> On 09/12/2014 02:19 PM, Jason Greene wrote: >>> >>>> On Sep 12, 2014, at 1:15 PM, Scott Marlow <smarlow(a)redhat.com&gt; wrote: >>>> >>>> Have you looked on the JPA spec mailing lists for clarification about >>>> whether the getPersistenceProviders result can be cached or not? I'll >>>> take a look to see if I can find anything. >>>> >>>>> On 09/12/2014 11:59 AM, Andrew Schmidt wrote: >>>>> I've been investigating performance issues with wildfly and hibernate validator. >>>>> The changes made for this https://issues.jboss.org/browse/AS7-1306 imply the >>>>> follow from the jpa spec: >>>>> >>>>> The results of calling the PersistenceProviderResolverHolder.getPersistenceProviderResolver >>>>> and the PersistenceProviderResolver.getPersistenceProviders methods must not be cached. >>>>> >>>>> however, the spec says later on: >>>>> >>>>> Note that the PersistenceProviderResolver.getPersistenceProviders() method >>>>> can potentially be called many times. It is therefore recommended that the >>>>> implementation of this method make use of caching. >>>>> >>>>> My interpretation is that wildfly should be caching the providers and it's the >>>>> responsibility of the callers to not cache the results. So the issue AS7-1306 shouldn't >>>>> have been implemented. >>>> >>>> Good point that the JPA.next specification should pick one (allow >>>> caching or not). >>> >>> I think the spec is consistent, but could be improved to avoid confusion. It says that callers of PersistenceProviderResolver should not cache the result, but PersistenceProviderResolver can internally cache itself. This makes sense because the container should be able to decide when to change the provider, and if the caller caches this wouldn’t take effect. >>> >>> >>>> >>>>> >>>>> The performance penalty of wildfly not caching the implementation of that method in regards to >>>>> hibernate is that HibernatePersistenceProvider uses a cache for classes/methods/fields >>>>> and that cache is blown away on every call to getPersistenceProviders and that >>>>> happens on every validation of every member of every class. >>>> >>>> The Hibernate implementation of these classes also do the same thing: >>>> >>>> https://github.com/hibernate/hibernate-jpa-api/blob/master/src/main/java/... >>>> >>>> The Hibernate PersistenceProviderResolverHolder does have a comment that >>>> caching should be introduced, whatever that means. >>>> >>>>> >>>>> _______________________________________________ >>>>> wildfly-dev mailing list >>>>> wildfly-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >>>> >>>> _______________________________________________ >>>> wildfly-dev mailing list >>>> wildfly-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/wildfly-dev >>> >>> -- >>> Jason T. Greene >>> WildFly Lead / JBoss EAP Platform Architect >>> JBoss, a division of Red Hat > > -- > Jason T. Greene > WildFly Lead / JBoss EAP Platform Architect > JBoss, a division of Red Hat _______________________________________________ wildfly-dev mailing list wildfly-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/wildfly-dev