You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openwebbeans.apache.org by Jonathan Gallimore <jo...@gmail.com> on 2022/11/03 16:07:24 UTC
InjectionResolver query
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
Re: InjectionResolver query
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le mar. 8 nov. 2022 à 22:18, Jonathan Gallimore <
jonathan.gallimore@gmail.com> a écrit :
> On Tue, Nov 8, 2022 at 6:56 PM Romain Manni-Bucau <rm...@gmail.com>
> wrote:
>
> > Indeed we can always add a pluggable (by config) cache but how many cases
> > does it cover?
> >
>
> To be fair, OWB is pretty friendly in terms of being pluggable. If the 2
> hash maps in InjectionResolver had protected access rather than private,
> I'd just tweak the extended class in TomEE and be done. If that's something
> that could be considered, that would be very much appreciated.
>
Nothing strong against but two notes:
1. TomEE can already handle that - same tech solution than the webapp bean
manager
2. I'd like OWB and TomEE to keep a consistent behavior at runtime if
possible so not sure it is that good
Did you think about a "debug" - word is bad - mode where this kind of
lookup would be detected and logged as a warning?
Can enable end users to detect it and potentially fix it
>
>
> > From what I saw it always hides another issue - in the ActionServlet the
> > other issue is the usage of CDI.current() for ex which is itself a
> hotspot
> > and not defined in the sample case (ear).
> > So is it worth? Not 100% sure.
> >
>
> Using CDI.current() in a servlet specifically perhaps feels a bit wild, and
> I've just done it as an illustration. You could imagine something like a
> tag library calling out to a common jar that does CDI.current() though, and
> the effect would be the same as the example I've provided. I'm not sure
> that programmatically looking up a bean in an application is contentious.
current will go through a SPI which is an issue by itself and then the
lookup goes through at least 2 levels you never need more than once so it
should really be a computeIfAbsent for the bean resolution in any lib.
>
>
> >
> > Anyway if there is a real will to have a configurable cache there, it
> must
> > ensure to not cache anything in the resolution (startup) phase nor cache
> > anything from an app scoped @Inject constructor or postconstruct callback
> > for ex - ie dont pollute the app with workaround meta when code is well
> > done.
> >
>
> Agreed. I'm specifically *not* proposing changing anything in the startup
> phase. In addition to that, I did suggest keeping the existing
> functionality as the default, and only caching the empty set if a flag is
> set. If I change this on the TomEE side, that's the approach I'd take there
> too.
>
Sounds wise.
>
>
> > It is doable but just want to highlight we should probably avoid a quick
> > and dirty workaround in the main codebase.
> >
> > That said I like the proposal for the JSF case, sounds like being better
> > than what we have currently + we know we are in the ELResolver so we can
> > clearly be more clever there anyway IMHO.
> >
>
> I'm not fully sure what you're suggesting here, or if its different to what
> I'm suggesting.
>
Just saying that you spotted an inconsistency - and likely something missed
- with name based resolution we could enhance to keep the JSF boost but
align the other cases on the type based resolution.
Maybe just an issue to open ;).
>
> We are in a position where EAR applications performed well on older
> versions of TomEE now really struggle performance-wise because the type
> resolution failure isn't cached any more. While I understand your view of
> fixing the app, I'd like to be able to offer the "old" functionality -
> whether that's through a change in OWB or TomEE.
>
Maybe let's catch when it got changed but AFAIK all the changes are startup
related so maybe an actual regression even if current behavior does not
shock me more than that.
Side note: it is quite trivial to fix adding a CDI decorator normally -
using a container extension or app extension, can be easier than patching
the container itself and enable to get the app related config injected
instead of relying on container config - it is sometimes easier to deploy
depending the env.
@Decorator @Priority(...)
public abstract class BeanManagerDecorator implements BeanManager,
Serializable {
@Inject
@Delegate
BeanManager beanManager;
// override getBeans/resolve/whatever you need to get cache ;)
}
>
> Jon
>
>
> >
> >
> > 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 à 19:19, David Blevins <da...@gmail.com> a
> > écrit :
> >
> > > > On Nov 8, 2022, at 7:50 AM, Romain Manni-Bucau <
> rmannibucau@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.
> > >
> > > My experience is a bit different. Systems like DNS will cache both
> hits
> > > and misses. There's an entire RFC dedicated to caching misses
> > > specifically: https://www.rfc-editor.org/rfc/rfc2308
> > >
> > > Yes, it can result in OOME issues, but some users may prefer that issue
> > to
> > > the performance issue.
> > >
> > > A reasonable way to implement this could be to count the misses we've
> > > cached and if the number exceeds a certain threshold we could do any of
> > the
> > > following:
> > >
> > > - stop caching misses
> > > - log warnings
> > > - purge old cache misses
> > >
> > > There could be a flag to control the desired behavior with the default
> to
> > > be the current strategy no caching of misses.
> > >
> > >
> > > -David
> > >
> > >
> > >
> > >
> >
>
Re: InjectionResolver query
Posted by Jonathan Gallimore <jo...@gmail.com>.
On Tue, Nov 8, 2022 at 6:56 PM Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Indeed we can always add a pluggable (by config) cache but how many cases
> does it cover?
>
To be fair, OWB is pretty friendly in terms of being pluggable. If the 2
hash maps in InjectionResolver had protected access rather than private,
I'd just tweak the extended class in TomEE and be done. If that's something
that could be considered, that would be very much appreciated.
> From what I saw it always hides another issue - in the ActionServlet the
> other issue is the usage of CDI.current() for ex which is itself a hotspot
> and not defined in the sample case (ear).
> So is it worth? Not 100% sure.
>
Using CDI.current() in a servlet specifically perhaps feels a bit wild, and
I've just done it as an illustration. You could imagine something like a
tag library calling out to a common jar that does CDI.current() though, and
the effect would be the same as the example I've provided. I'm not sure
that programmatically looking up a bean in an application is contentious.
>
> Anyway if there is a real will to have a configurable cache there, it must
> ensure to not cache anything in the resolution (startup) phase nor cache
> anything from an app scoped @Inject constructor or postconstruct callback
> for ex - ie dont pollute the app with workaround meta when code is well
> done.
>
Agreed. I'm specifically *not* proposing changing anything in the startup
phase. In addition to that, I did suggest keeping the existing
functionality as the default, and only caching the empty set if a flag is
set. If I change this on the TomEE side, that's the approach I'd take there
too.
> It is doable but just want to highlight we should probably avoid a quick
> and dirty workaround in the main codebase.
>
> That said I like the proposal for the JSF case, sounds like being better
> than what we have currently + we know we are in the ELResolver so we can
> clearly be more clever there anyway IMHO.
>
I'm not fully sure what you're suggesting here, or if its different to what
I'm suggesting.
We are in a position where EAR applications performed well on older
versions of TomEE now really struggle performance-wise because the type
resolution failure isn't cached any more. While I understand your view of
fixing the app, I'd like to be able to offer the "old" functionality -
whether that's through a change in OWB or TomEE.
Jon
>
>
> 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 à 19:19, David Blevins <da...@gmail.com> a
> écrit :
>
> > > On Nov 8, 2022, at 7:50 AM, Romain Manni-Bucau <rm...@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.
> >
> > My experience is a bit different. Systems like DNS will cache both hits
> > and misses. There's an entire RFC dedicated to caching misses
> > specifically: https://www.rfc-editor.org/rfc/rfc2308
> >
> > Yes, it can result in OOME issues, but some users may prefer that issue
> to
> > the performance issue.
> >
> > A reasonable way to implement this could be to count the misses we've
> > cached and if the number exceeds a certain threshold we could do any of
> the
> > following:
> >
> > - stop caching misses
> > - log warnings
> > - purge old cache misses
> >
> > There could be a flag to control the desired behavior with the default to
> > be the current strategy no caching of misses.
> >
> >
> > -David
> >
> >
> >
> >
>
Re: InjectionResolver query
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Indeed we can always add a pluggable (by config) cache but how many cases
does it cover?
From what I saw it always hides another issue - in the ActionServlet the
other issue is the usage of CDI.current() for ex which is itself a hotspot
and not defined in the sample case (ear).
So is it worth? Not 100% sure.
Anyway if there is a real will to have a configurable cache there, it must
ensure to not cache anything in the resolution (startup) phase nor cache
anything from an app scoped @Inject constructor or postconstruct callback
for ex - ie dont pollute the app with workaround meta when code is well
done.
It is doable but just want to highlight we should probably avoid a quick
and dirty workaround in the main codebase.
That said I like the proposal for the JSF case, sounds like being better
than what we have currently + we know we are in the ELResolver so we can
clearly be more clever there anyway IMHO.
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 à 19:19, David Blevins <da...@gmail.com> a
écrit :
> > On Nov 8, 2022, at 7:50 AM, Romain Manni-Bucau <rm...@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.
>
> My experience is a bit different. Systems like DNS will cache both hits
> and misses. There's an entire RFC dedicated to caching misses
> specifically: https://www.rfc-editor.org/rfc/rfc2308
>
> Yes, it can result in OOME issues, but some users may prefer that issue to
> the performance issue.
>
> A reasonable way to implement this could be to count the misses we've
> cached and if the number exceeds a certain threshold we could do any of the
> following:
>
> - stop caching misses
> - log warnings
> - purge old cache misses
>
> There could be a flag to control the desired behavior with the default to
> be the current strategy no caching of misses.
>
>
> -David
>
>
>
>
Re: InjectionResolver query
Posted by David Blevins <da...@gmail.com>.
> On Nov 8, 2022, at 7:50 AM, Romain Manni-Bucau <rm...@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.
My experience is a bit different. Systems like DNS will cache both hits and misses. There's an entire RFC dedicated to caching misses specifically: https://www.rfc-editor.org/rfc/rfc2308
Yes, it can result in OOME issues, but some users may prefer that issue to the performance issue.
A reasonable way to implement this could be to count the misses we've cached and if the number exceeds a certain threshold we could do any of the following:
- stop caching misses
- log warnings
- purge old cache misses
There could be a flag to control the desired behavior with the default to be the current strategy no caching of misses.
-David
Re: InjectionResolver query
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le mar. 8 nov. 2022 à 17:26, Jonathan Gallimore <
jonathan.gallimore@gmail.com> a écrit :
> 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
100% JSF case (even if you are right it is not the case on an strict API
standpoint but name is not intended to be much used in all other libs)
due to the ELResolver nature and impls out there it would be too slow
otherwise and keyset is quite closed compared to potential types (no issue
with equals/hashcode there ;)) AFAIK.
>
>
> 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.
>
yes and no, normally from a spec standpoint you can decorate the bean
manager so you can handle the resolution for the lib transparently for the
lib - if it is for TomEE and not a customer/3rd party I would still
encourage to fix the upstream if possible.
>
> 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 <rm...@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.gallimore@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 <
> rmannibucau@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.gallimore@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
> > > > >
> > > >
> > >
> >
>
Re: InjectionResolver query
Posted by Jonathan Gallimore <jo...@gmail.com>.
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 <rm...@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.gallimore@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 <rmannibucau@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.gallimore@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
> > > >
> > >
> >
>
Re: InjectionResolver query
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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.gallimore@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 <rm...@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.gallimore@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
> > >
> >
>
Re: InjectionResolver query
Posted by Jonathan Gallimore <jo...@gmail.com>.
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 <rm...@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.gallimore@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
> >
>
Re: InjectionResolver query
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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.gallimore@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
>