HHH-17980 Excessive contention during getter identification in the ByteBuddy enhancer #8204
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...main/java/org/hibernate/bytecode/enhance/internal/bytebuddy/ByteBuddyEnhancementContext.java
Dismissed
Show dismissed
Hide dismissed
| //otherwise large models might exhibit significant contention on the map. | ||
| Map<String, MethodDescription> getters = getterByTypeMap.get( erasure ); | ||
|
|
||
| if ( getters == null ) { |
There was a problem hiding this comment.
Just doing that is not enough to remove the contention?
I had good results using this pattern when you recommended me to always check with a get first. And if that good enough it could be more easily backported?
Now I suppose you didn’t come up with something complicated just for fun so interested in your insights :)
There was a problem hiding this comment.
Hi, so glad you’re checking this :)
Yes, indeed, the solution of just doing the 'get' first was the first thing I tried, but it made almost no difference in this particular case.
Turns out that when there is a very high chance of the current operation being the first (only?) to ever check for that particular key, we still end up doing a lot of 'computeIfAbstent' - which takes the very coarse lock. Things are actually made worse, potentially, as now you have an additional pointless get first and then you still fall into the slow path - now in this case the 'get' wasn't always pointless so things balanced out a little, and there was a little improvement, but still a lot of contention: 25+ seconds in my case.
With the pattern below I got a nice balance: extremely low, negligible contention (order of some dozen ms), while still avoiding to perform duplicated work so making effective use of the cache.
I guess it's a good reminder of how such guidelines are highly dependent on context - in this case the expectation of which outcome is most likely.
I must say I'm a little puzzled that there isn't something OOB in the JDK for this scenario.. hopefully CHM could improve on this, but for now this does the trick for us.
There was a problem hiding this comment.
I ended up doubting myself here and wondering why the CHM causes such levels of contention.. looking at the source code it's unclear, it seems to make a similar attempt to what I've done here, except it doesn't seem to be effective in practice !?
It might be interesting to investigage further with the Java team - but I'd rather put all these pieces together and see the live-reload issue fixed first.
This is the first patch on a series, motivated by the analysis of bootstrap times using https://github.com/topicusonderwijs/tribe-krd-quarkus.
https://hibernate.atlassian.net/browse/HHH-17980
This patch in isolation will not dramatically improve things, as contention is currently quite low - however this lock becomes a problem as other bottlenecks are resolved. I'm resolving the other bottlenecks via a combination of patches on the Quarkus side and other improvements coming on the ORM side.
Not all improvements on the ORM side will be suitable for pre-7 versions, still I'd apply this one on 6.5+ as well as the combination of improvements on 6.5 is still meaningful enough to make this lock problematic.