FYI, created https://issues.apache.org/jira/browse/OWB-1343 for the flag to skip all the loadClass(package-info).
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. 28 juil. 2020 à 10:10, Mark Struberg <[email protected]> a écrit : > Go for it please. Can also do smallish tweaks later on. It's for sure much > better than before! > And thanks Vincente for the work! > > LieGrue, > strub > > > Am 28.07.2020 um 08:52 schrieb Romain Manni-Bucau <[email protected]>: > > 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. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >
