-
Notifications
You must be signed in to change notification settings - Fork 3k
Hibernate ORM - Assorted improvements to proxy generation #49579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
...te-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/ProxyBuildingHelper.java
Show resolved
Hide resolved
generatedProxyQueue.add(buildExecutor.submit(() -> { | ||
DynamicType.Unloaded<?> unloaded = proxyHelper.buildUnloadedProxy(managedClassOrPackageName, | ||
proxyInterfaceNames); | ||
return new CachedProxy(managedClassOrPackageName, unloaded, proxyInterfaceNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
I noticed something odd though - the parameter proxyInterfaceNames
passed to buildUnloadedProxy
is a Set whose content is always the same? IFF that's the case then the buildUnloadedProxy
could resolve the types in advance and also reuse as a constant.
But it looks like suspicious, like if we lost something during previous changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I don't want to open a can of worms here.
I tried to "optimize" things a bit here but it makes the code less readable. Given the description of the type will be in the TypePool cache after the first call, I think we are good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quirkiness is a result of my (botched?) upgrade to Hibernate ORM 7: 6f4cfac#diff-583c184a7bb985fc15d72cd11861e77f12dbd3332307065f3122b8d6390c1602L1371-R1367
In short, previously you could add custom interface names with @Proxy
, but that's gone in ORM 7+.
I'll send a PR to do what Sanne suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks 👍
ea7eac5
to
531bc1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most changes look fine, but unless I'm missing something (and I very well might be), it seems the proxy cache is no longer populated?
This raises the question: was this done to fix a particular problem, and is there a benchmark we could use to check how effective it is? It's fine if there isn't, but if there is that would give a lot more confidence there isn't some hard-to-catch issue like this.
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
531bc1b
to
93e17ca
Compare
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Outdated
Show resolved
Hide resolved
93e17ca
to
1a6978a
Compare
1a6978a
to
8bd0f55
Compare
Status for workflow
|
Better reviewed commit per commit.