Le mar. 8 nov. 2022 à 22:18, Jonathan Gallimore <
jonathan.gallim...@gmail.com> a écrit :
> On Tue, Nov 8, 2022 at 6:56 PM Romain Manni-Bucau <rmannibu...@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 <david.blev...@gmail.com> a
> > écrit :
> >
> > > > On Nov 8, 2022, at 7:50 AM, 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.
> > >
> > > 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
> > >
> > >
> > >
> > >
> >
>
