[hibernate-dev] HHH-13916 / WFLY-13259

Wed Apr 15 20:06:23 EDT 2020

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 <galder at 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 <sanne at hibernate.org>
> wrote:
>
>> On Wed, 15 Apr 2020 at 02:16, Gail Badner <gbadner at 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 <gbadner at 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 <sanne at 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 <gbadner at 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 at lists.jboss.org
>> >>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >>> >
>> >>>
>>
>>