Hi Galder, Thanks for the response. I agree this would be a good change for H6. I'm happy to hear that this is on someone's radar for Infinispan.
Sanne, regarding: > I'd say if you clearly mark the old method deprecated (or even remove it?) it will be guaranteed we don't forget about this improvement. Are you suggesting that SharedSessionContractImplementor#getSessionIdentifier be deprecated? If so, it has other uses, so it really can't be deprecated. I've moved the fix version for HHH-13916 to 6.0.0.Beta1 and created a wip/6.0 PR: https://github.com/hibernate/hibernate-orm/pull/3341 . Regards, Gail On Wed, Apr 15, 2020 at 7:58 AM Galder Zamarreno <gal...@redhat.com> wrote: > Given that there's a workaround for the issue, I'd agree with Sanne to > make this an ORM 6+ only improvement. > > Indeed I'm no longer in the team and hence could not be the maintainer for > such a module, but I'd be happy to discuss improvements for ORM6 caching. > I've not been aware of any discussions on that yet. > > On the Infinispan side, just a couple of weeks back Tristan was asking me > about the need for a remote Infinispan cache provider for ORM. Maybe ORM6 > offers a good opportunity to rework the provider and make it easy to > maintain a local/remote provider. > > Galder > > > On Wed, Apr 15, 2020 at 1:42 PM Sanne Grinovero <sa...@hibernate.org> > wrote: > >> On Wed, 15 Apr 2020 at 02:16, Gail Badner <gbad...@redhat.com> wrote: >> > >> > FWIW, there's no point in fixing HHH-13916 unless we hear that >> Infinispan is going to use the fix. >> >> I suppose we can expect that that will eventually happen - I just >> don't know when and were to best make a note of such a need? I'd say >> if you clearly mark the old method deprecated (or even remove it?) it >> will be guaranteed we don't forget about this improvement. >> >> As far as I know, there isn't an Infinispan 2LC codebase for ORM6 yet, >> and there will be more differences likely? >> >> Also keep in mind that Galder no longer works on Infinispan, he's now >> focused on GraalVM and JDK engineering. I'm glad he's still around and >> we might be able to bother him for suggestions and wisdom, but I'm not >> sure yet who's going to be responsible to create such a new module. >> Radim is on the performance team; as always, would be awesome to have >> his help but I don't know of the performance team will be able to own >> the module maintenance. >> >> Thanks, >> Sanne >> >> >> > >> > Radim/Galder, WDYT? >> > >> > Thanks, >> > Gail >> > >> > On Tue, Apr 14, 2020 at 3:25 PM Gail Badner <gbad...@redhat.com> 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 <sa...@hibernate.org> >> 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 <gbad...@redhat.com> 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@lists.jboss.org >> >>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> >>> > >> >>> >> >> _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
