Thanks for the reply. Its possibly a tangent, but if the intention is to not cache misses, I'm curious as to why misses are cached when resolving by name:
https://github.com/apache/openwebbeans/blob/master/webbeans-impl/src/main/java/org/apache/webbeans/container/InjectionResolver.java#L395 ActionServlet#process is obviously not a great example - I could just @Inject into a field, or cache the lookup, as you say. I can't however do that in, say, a 3rd party library that I don't control. Anyway, thank you for the feedback, and the quick responses. It sounds like there's no appetite for either change I've suggested in OWB, and that we should handle this elsewhere if we need it. Thanks Jon On Tue, Nov 8, 2022 at 3:51 PM Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > Hi Jon, > > It is intended to not cache the missed hits since they shouldn't occur at > runtime and would open the door to OOME issues. > I looked at your sample, there is indeed a bug in ActionServlet#process > which should do that resolution in init(). > > From my XP frameworks using that kind of resolution cache it themselves > (JSF for ex) if relevant or don't cache it cause it is not a big perf issue > (could be for jbatch) else it looks like bugs. > > Hope it helps > > Romain Manni-Bucau > @rmannibucau <https://twitter.com/rmannibucau> | Blog > <https://rmannibucau.metawerx.net/> | Old Blog > <http://rmannibucau.wordpress.com> | Github < > https://github.com/rmannibucau> | > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > < > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > Le mar. 8 nov. 2022 à 15:36, Jonathan Gallimore < > jonathan.gallim...@gmail.com> a écrit : > > > Thanks for the reply, Romain! > > > > There's a couple of points on this one. Firstly, it's not startup > behaviour > > that's an issue in this specific case, it's actually resolution at > runtime. > > In effect, because the child InjectionResolver cannot resolve the bean, > and > > never caches that fact, it tries to work through the beans available each > > and every time an attempt is made to resolve the bean. Only once that has > > happened, do we delegate to the parent InjectionResolver (which does have > > the bean cached). > > > > On the TomEE-side, WebappInjectionResolver is a very thin extension of > > InjectionResolver, and it hasn't changed in 7 years: > > > > > https://github.com/apache/tomee/blob/tomee-8.x/container/openejb-core/src/main/java/org/apache/openejb/cdi/WebAppInjectionResolver.java > > . > > If there's a specific change you think might be useful, I'm happy to give > > it a go. We could, theoretically, do the caching in > > WebappInjectionResolver, although I'm hesitant to double the number of > > HashMaps we're keeping around for caching these resolutions. Making > > InjectionResolver.resolvedBeansByType (and maybe > > InjectionResolver.resolvedBeansByName) protected rather than private > could > > offer an easy way for anyone consuming OpenWebBeans to extend its > behaviour > > in this regard. We could then add any settings, etc on the TomEE side and > > just use the existing caching hash maps - for example we could check the > > cache of both the child and the parent InjectionResolvers before doing > > anything else. > > > > I did put together a sample project to demonstrate what I'm seeing: > > https://github.com/jgallimore/cdi-resolve-type-perf. > > > > It can be built with: > > > > mvn clean install > > > > TomEE run with: > > > > cd hello-perf-test > > mvn tomee:run > > > > And the performance test run with: > > > > cd hello-perf-test > > ./grinder.sh > > > > In essence, the servlet is looking up a CDI bean each time, and always > gets > > the InjectionResolver for the web archive. I've deliberately created a > > large number of classes (10000) for this to work through in a big jar > that > > sits in WEB-INF/lib. The actual bean I want to resolve is in the EJB > module > > in the EAR, and is picked up when trying to resolve it from the war file > > fails. Running a performance test on this runs at around 1500 > transactions > > per second (*). Allowing the InjectionResolver to cache > > Collections.EMPTY_SET when a type isn't resolved - e.g.: > > > > if (!startup) > > { > > if (resolvedComponents == null || resolvedComponents.size() > == > > 0) > > { > > resolvedBeansByType.put(cacheKey, Collections.EMPTY_SET); > > } > > else > > { > > resolvedBeansByType.put(cacheKey, resolvedComponents); > > } > > > > if (logger.isLoggable(Level.FINE)) > > { > > logger.log(Level.FINE, "DEBUG_ADD_BYTYPE_CACHE_BEANS", > > cacheKey); > > } > > } > > > > this goes up to around 65000 transactions per second (*), so the impact > is > > definitely felt. > > > > * WSL2 Ubuntu/Windows 11 on i7-12700H 2.30 GHz, 32GB RAM > > > > Thanks > > > > Jon > > > > On Thu, Nov 3, 2022 at 4:50 PM Romain Manni-Bucau <rmannibu...@gmail.com > > > > wrote: > > > > > Hi Jon, > > > > > > The fix Thomas did is accurate and needed so think we should be fine > like > > > that but I also agree the "pre-fix" implementation of TomEE never > caught > > up > > > this change. > > > I guess the fix is to ensure the wrappers we have in TomEE also have > this > > > kind of behavior but handling the cache for TomEE startup which is not > > > aligned on OWB startup for an EAR. > > > > > > Hope it makes sense. > > > > > > Romain Manni-Bucau > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > > <https://rmannibucau.metawerx.net/> | Old Blog > > > <http://rmannibucau.wordpress.com> | Github < > > > https://github.com/rmannibucau> | > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > > > < > > > > > > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > > > > > > > > > Le jeu. 3 nov. 2022 à 17:07, Jonathan Gallimore < > > > jonathan.gallim...@gmail.com> a écrit : > > > > > > > Hey folks > > > > > > > > I've run into an interesting problem on the TomEE-side, running an > EAR > > > > file. This seems to end up with 2 injection resolvers for the > > > application: > > > > one for the web part of the application, and a parent for the EAR. > > > > > > > > When resolving by type at runtime, with large archives, we've seen a > > > > slowdown from previous versions of OpenWebBeans. This appears to be > > > caused > > > > by the InjectionResolver.implResolveByType returning an empty set. > > TomEE > > > > will then try the parent resolver (successfully), but the result > (empty > > > > set) in the child resolver is not cached, so it searches through the > > > whole > > > > set of beans each time. The result used to be cached, prior to this > > > commit: > > > > > > > > > > > > > > https://github.com/apache/openwebbeans/commit/a6d22d2abf6d6d5a02a14be1367daacca450359d > > > > > > > > Could caching the empty result in the InjectionResolver be > > re-introduced, > > > > albeit with a switch? (I'm happy for the default to be "don't > cache"). > > > > Interestingly, InjectionResolver.implResolveByName does cache the > empty > > > > result. I'm happy to create a PR. > > > > > > > > Many thanks > > > > > > > > Jon > > > > > > > > > >
