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. > 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 <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 > > > > > > > > >
