Speedup Hibernate ORM's enhancement of large models

[Sanne]

This works far better, in my local tests.

I feel it's ready, but it will require a fix on the ORM side to be merged first:

• HHH-18011 Parallel entity enhancement causes a classloader cache stampede hibernate/hibernate-orm#8263

This brings enhancement times of the large model provided as benchmark down to ~1 second, without enabling Quarkus's caching capabilities of caching enhancement of unmodified classes (which we should also enable, but it's a separate aspect)

[quarkus-bot]

/cc @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

[yrodiere]

Thanks!

Seems safe to me, and it's great if it speeds things up so much, but could you please add a few comments? I'm afraid it's not as obvious for us as it is for you :)

[Sanne]

Updated to add the suggested comments; also now registering the Panache super types as "core types" so they'd also benefit from caching, if used.

[Sanne]

also now registering the Panache super types as "core types" so they'd also benefit from caching, if used.

Note to self: this implies that if we have live-reload tests in Panache which have entities using the same package name as Panache itself they might fail now for the silly reason of us skipping the work on such classes.

[Sanne]

We decided to use:
"jakarta.persistence.", "java.", "org.hibernate.annotations.", "io.quarkus.hibernate.reactive.panache.", "io.quarkus.hibernate.orm.panache.", "org.hibernate.search.mapper.pojo.mapping.definition.annotation.", "jakarta.validation.constraints."

TODO: check this doesn't break Panache integration tests as they might be running under the same package name.

[Sanne]

Rebased: simplified implementation taking advantage of eb69d59

[Sanne]

Ok this is ready now, it was waiting for ORM 6.5.1.

[yrodiere]

Thanks. LGTM, just a small doubt that there might be an obsolete comment.

[Sanne]

Please don't merge - I've pushed an experiment and need CI feedback.

[Sanne]

@yrodiere I understand what's going on now, unfortunatley sharing the ByteBuddy caches would require much more work - and is tricky.

So I suggest we merge this version: it shares the caches of "core types" (which we know are not being modified) but isolates the caches used for bytecode enhancement in threadlocals to ensure independent interactions. It also shared the Enhancer implementation across runs, and makes its initialization lazy.

It's taking the safer approach - there's room for improvement as this forces each thread to repeat quite some work, and also allocate large structures w/o sharing those, but that would require much more work to share them safely, so I'd rather take the 80% improvement now and keep it safe until we have time to add more stricter integration tests.

Ok to merge? I'd hit the button as it's already approved, but since several changes were made...

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 138115a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[mapuci]

offtopic:

@Sanne could you please take a look at this issue? It is kindof related to what you are doing here :)
#40719
There is something wrong with enhanced classes or even their classloaders...(in quarkus 2 and 3, but not in wildfly).
Reproducer repo contains enhanced .class files, so you can take a look at them.

I tried to reproduce problem with this branch, there is still a bug.

[yrodiere]

I'm really not sure using threadlocals without ever clearing them is safe in dev mode, but I'll let you check that, and merge if you are confident.

[yrodiere]

You could clear the thread-local? Maybe that's not the best place to do this, but it would be nice to at least try... somewhere...

We've been bitten by classloader leaks caused by threadlocals in the past... And it can get quite bad.

[yrodiere]

This threadlocal isn't static -- is this on purpose?

From what I can see ThreadsafeLocator is, in practice, a singleton, so using static here would make threadlocal cleanup easier. You know, where we restart in dev mode, drop half the classloaders but keep the same threads, resulting in threadlocals piling up...