Current pr is mergeable for me and fix is good. Will let Mark have a last review since he commented first the pr but looks good to me now, thanks a lot Vincente.
Le lun. 27 juil. 2020 à 22:35, Thomas Andraschko < [email protected]> a écrit : > Then +1 to remove the && > > Vicente Rossello <[email protected]> schrieb am Mo., 27. Juli 2020, > 22:32: > >> Hi, >> >> Just changing outside the loop >> >> Boolean result = packageVetoCache.get(previousPackage); >> if (result != null && result) >> { >> return result; >> } >> >> >> to >> >> Boolean result = packageVetoCache.get(previousPackage); >> if (result != null) >> { >> return result; >> } >> >> >> solves the performance problem, the difference is unnoticeably. The real >> problem is that, for every class, is checking for a package-info in all >> "upper packages" every single time. >> >> >> On Mon, Jul 27, 2020 at 10:07 PM Romain Manni-Bucau < >> [email protected]> wrote: >> >>> Issue is that cache cant really be better until you use xbean finder >>> (discovery) to handle it when finder is used and fallback on reflection in >>> other cases. >>> Requires some work but it is the fastest possible impl for us. >>> For a not xbean related impl current cache is already optimal afaik but >>> load class is slow. >>> >>> That said, if you use cds for your stack and append to your classpath >>> your own modules (easy in ide/maven) then you benefit from this perf boost >>> since the classpath prefix is what is used. Requires some dev setup and >>> unrelated to owb but can help. >>> >>> +1 for the prop (simple boolean i think) anyway. I can also do it on >>> wednesday if it helps. >>> >>> Le lun. 27 juil. 2020 à 20:46, Thomas Andraschko < >>> [email protected]> a écrit : >>> >>>> I would improve the cache If possible, otherwise add a property and >>>> disable the Feature per default, to have a good startup perf >>>> >>>> vicente Rossello <[email protected]> schrieb am Mo., 27. Juli >>>> 2020, 20:33: >>>> >>>>> Hi, I'll leave the check also outside. >>>>> >>>>> The problem with CDs is that they are not good for development, where >>>>> I really want a fast startup time >>>>> >>>>> If you want after this PR I can make a property to disable this. >>>>> >>>>> El lun., 27 jul. 2020 20:20, Romain Manni-Bucau <[email protected]> >>>>> escribió: >>>>> >>>>>> The check happens before the loop cause it is faster if you have more >>>>>> than one class per package (which is statistically true). >>>>>> >>>>>> You can also enable CDS to bypass this load time. >>>>>> >>>>>> I would also be happy to have a property to skip this whole feature >>>>>> too as proposed some times ago. >>>>>> >>>>>> Le lun. 27 juil. 2020 à 12:57, Vicente Rossello < >>>>>> [email protected]> a écrit : >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> I made a test with veto at package level to try to see if the patch >>>>>>> is correct, if it's of any use. >>>>>>> https://github.com/apache/openwebbeans/pull/30 >>>>>>> >>>>>>> Feel free to comment or just discard it. >>>>>>> >>>>>>> On Mon, Jul 27, 2020 at 11:30 AM Mark Struberg <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> Should work as expected. I need to think through if we really need >>>>>>>> to walk into all superpackages in the while(true). >>>>>>>> Or if we should try to get those results from the cache as well. >>>>>>>> >>>>>>>> a.b.c.d.e.f.x1 >>>>>>>> a.b.c.d.e.f.x2 >>>>>>>> >>>>>>>> In that case the whole list up to f should already be cached for x2. >>>>>>>> >>>>>>>> LieGrue, >>>>>>>> strub >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Am 27.07.2020 um 11:19 schrieb Vicente Rossello < >>>>>>>> [email protected]>: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> That's what already did and times become as usual, but I'm not sure >>>>>>>> if it breaks something (I'm not using any veto). Tests in >>>>>>>> openwebbeans-impl >>>>>>>> do work with this change >>>>>>>> >>>>>>>> On Mon, Jul 27, 2020 at 11:14 AM Mark Struberg <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi Vincente! >>>>>>>>> >>>>>>>>> There is a bit code before that block which already checks the >>>>>>>>> cache: >>>>>>>>> >>>>>>>>> Boolean result = packageVetoCache.get(previousPackage); >>>>>>>>> if (result != null && result) >>>>>>>>> { >>>>>>>>> return result; >>>>>>>>> } >>>>>>>>> >>>>>>>>> >>>>>>>>> Imo it should also return if a False is cached. >>>>>>>>> can you please remove the && result and do a bench again? >>>>>>>>> >>>>>>>>> txs and LieGrue, >>>>>>>>> strub >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Am 27.07.2020 um 10:00 schrieb Vicente Rossello < >>>>>>>>> [email protected]>: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> I've seen a startup performance regression since OWB 2.0.17 and >>>>>>>>> latest snapshot. Our boot times have increased from 10 to about 14 >>>>>>>>> seconds >>>>>>>>> (only OWB side). I can see that it always try to load the same >>>>>>>>> package-info's in: >>>>>>>>> >>>>>>>>> while (true) >>>>>>>>> { >>>>>>>>> try // not always existing but enables to go further when >>>>>>>>> getPackage is not available (graal) >>>>>>>>> { >>>>>>>>> pckge = classLoader.loadClass(previousPackage + >>>>>>>>> (previousPackage.isEmpty() ? "" :".") + >>>>>>>>> "package-info").getPackage(); >>>>>>>>> break; >>>>>>>>> } >>>>>>>>> catch (Exception e) >>>>>>>>> { >>>>>>>>> if (previousPackage.isEmpty()) >>>>>>>>> { >>>>>>>>> pckge = null; >>>>>>>>> break; >>>>>>>>> } >>>>>>>>> packageVetoCache.put(previousPackage, false); >>>>>>>>> idx = previousPackage.lastIndexOf('.'); >>>>>>>>> if (idx > 0) >>>>>>>>> { >>>>>>>>> previousPackage = previousPackage.substring(0, idx); >>>>>>>>> } >>>>>>>>> else >>>>>>>>> { >>>>>>>>> previousPackage = ""; >>>>>>>>> } >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>>> >>>>>>>>> I think that, in this loop, it should take into account the >>>>>>>>> packageVetoCache (whether it's true or false). Is it correct? Do you >>>>>>>>> want a PR with this correction? >>>>>>>>> >>>>>>>>> >>>>>>>>> Best regards, >>>>>>>>> >>>>>>>>> Vicente. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>
