Hi Gail, I would go ahead with this improvement for ORM 6 and avoid spending your precious time on a v5 improvement - especially if it's going to require coordination with both the Infinispan and WildFly teams. Thanks On Fri, 10 Apr 2020 at 00:56, Gail Badner <gbadner(a)redhat.com&gt; wrote: > > I've been looking into how to fix this issue: > > https://hibernate.atlassian.net/browse/HHH-13916 > https://issues.redhat.com/browse/WFLY-13259 > > The crux of the matter is when Hibernate calls CacheHelper.fromSharedCache( > session, cacheKey, cachAccess ), and the entity is not found in the cache, > Infinispan stores a PendingPut containing a > SharedSessionContractImplementor instance. > > IIUC, as an optimization, Infinispan assumes that the entity not found in > the cache will ultimately be added to the cache after it is loaded from the > database. If that doesn't happen, the PendingPut will expire and will > eventually be removed. Until it expires, > the SharedSessionContractImplementor instance cannot be garbage-collected. > > This is particularly a problem if the cache is not disabled while a large > amount of cacheable data is being imported. This is the particular use case > described by WFLY-13259. There is a reproducer attached that > throws OutOfMemoryError. > > The obvious workaround is to set org.hibernate.CacheMode.IGNORE (or > possibly CacheMode.PUT?) while importing data. > > I discussed this briefly with Sanne, and we agree that an improvement would > be to not store a SharedSessionContractImplementor in a PendingPut at all. > > There is already a way to get a UUID for the session by calling > SharedSessionContractImplementor#getSessionIdentifier(). Unfortunately, the > implementation in AbstractSharedSessionContract indicates that frequent > "UUID generations will cause a significant amount of contention". > > Sanne has suggested returning a "token" that is just a new Object. I've > created a branch > <https://github.com/gbadner/hibernate-core/tree/HHH-13916_token> [1] that > does this. > > Infinispan would need to be updated so that PendingPut#owner is set > to SharedSessionContractImplementor#getSessionToken() (instead of > the SharedSessionContractImplementor object). > > Looking at the Infinispan code, I see that code that would be affected is > in > org.infinispan.hibernate.cache.commons.access.PutFromLoadValidator, which > is used by infinispan-hibernate-cache-v51. > > IIUC, in order to fix this any time soon for WildFly or EAP 7.x, [1] would > have to be backported to both Hibernate ORM 5.1 and 5.3 branches, and the > Hibernate versions would have to be updated in Infinispan before Infinispan > could be updated to use SharedSessionContractImplementor#getSessionToken(). > > Galder/Radim, are there any plans for > dropping infinispan-hibernate-cache-v51? > > Are there other places where the SharedSessionContractImplementor is stored > in the cache? > > Aside from infinispan-hibernate-cache-v51, do you see anything about [1] > that would cause problems? > > If not, when do you think we could coordinate this change? Do we need to > wait for Hibernate ORM 6.0? > > This is considered an improvement, so it's not urgent. It would be nice to > fix this though. > > Galder/Radim, please provide your input so we figure out when it can be > fixed. > > Thanks, > Gail > > [1] https://github.com/gbadner/hibernate-core/tree/HHH-13916_token > [2] > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >

FWIW, there's no point in fixing HHH-13916 unless we hear that Infinispan is going to use the fix.

Radim/Galder, WDYT? Thanks, Gail On Tue, Apr 14, 2020 at 3:25 PM Gail Badner <gbadner(a)redhat.com&gt; wrote: > > Hi Sanne, > > I've reworked HHH-13916 to use this alternative, and set the fix version to 6-wishlist. > > Regards, > Gail > > On Tue, Apr 14, 2020 at 1:23 AM Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: >> >> Hi Gail, >> >> I would go ahead with this improvement for ORM 6 and avoid spending >> your precious time on a v5 improvement - especially if it's going to >> require coordination with both the Infinispan and WildFly teams. >> >> Thanks >> >> On Fri, 10 Apr 2020 at 00:56, Gail Badner <gbadner(a)redhat.com&gt; wrote: >> > >> > I've been looking into how to fix this issue: >> > >> > https://hibernate.atlassian.net/browse/HHH-13916 >> > https://issues.redhat.com/browse/WFLY-13259 >> > >> > The crux of the matter is when Hibernate calls CacheHelper.fromSharedCache( >> > session, cacheKey, cachAccess ), and the entity is not found in the cache, >> > Infinispan stores a PendingPut containing a >> > SharedSessionContractImplementor instance. >> > >> > IIUC, as an optimization, Infinispan assumes that the entity not found in >> > the cache will ultimately be added to the cache after it is loaded from the >> > database. If that doesn't happen, the PendingPut will expire and will >> > eventually be removed. Until it expires, >> > the SharedSessionContractImplementor instance cannot be garbage-collected. >> > >> > This is particularly a problem if the cache is not disabled while a large >> > amount of cacheable data is being imported. This is the particular use case >> > described by WFLY-13259. There is a reproducer attached that >> > throws OutOfMemoryError. >> > >> > The obvious workaround is to set org.hibernate.CacheMode.IGNORE (or >> > possibly CacheMode.PUT?) while importing data. >> > >> > I discussed this briefly with Sanne, and we agree that an improvement would >> > be to not store a SharedSessionContractImplementor in a PendingPut at all. >> > >> > There is already a way to get a UUID for the session by calling >> > SharedSessionContractImplementor#getSessionIdentifier(). Unfortunately, the >> > implementation in AbstractSharedSessionContract indicates that frequent >> > "UUID generations will cause a significant amount of contention". >> > >> > Sanne has suggested returning a "token" that is just a new Object. I've >> > created a branch >> > <https://github.com/gbadner/hibernate-core/tree/HHH-13916_token> [1] that >> > does this. >> > >> > Infinispan would need to be updated so that PendingPut#owner is set >> > to SharedSessionContractImplementor#getSessionToken() (instead of >> > the SharedSessionContractImplementor object). >> > >> > Looking at the Infinispan code, I see that code that would be affected is >> > in >> > org.infinispan.hibernate.cache.commons.access.PutFromLoadValidator, which >> > is used by infinispan-hibernate-cache-v51. >> > >> > IIUC, in order to fix this any time soon for WildFly or EAP 7.x, [1] would >> > have to be backported to both Hibernate ORM 5.1 and 5.3 branches, and the >> > Hibernate versions would have to be updated in Infinispan before Infinispan >> > could be updated to use SharedSessionContractImplementor#getSessionToken(). >> > >> > Galder/Radim, are there any plans for >> > dropping infinispan-hibernate-cache-v51? >> > >> > Are there other places where the SharedSessionContractImplementor is stored >> > in the cache? >> > >> > Aside from infinispan-hibernate-cache-v51, do you see anything about [1] >> > that would cause problems? >> > >> > If not, when do you think we could coordinate this change? Do we need to >> > wait for Hibernate ORM 6.0? >> > >> > This is considered an improvement, so it's not urgent. It would be nice to >> > fix this though. >> > >> > Galder/Radim, please provide your input so we figure out when it can be >> > fixed. >> > >> > Thanks, >> > Gail >> > >> > [1] https://github.com/gbadner/hibernate-core/tree/HHH-13916_token >> > [2] >> > _______________________________________________ >> > hibernate-dev mailing list >> > hibernate-dev(a)lists.jboss.org >> > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >>