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] > <mailto:[email protected]>> a écrit : > Then +1 to remove the && > > Vicente Rossello <[email protected] <mailto:[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] > <mailto:[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] > <mailto:[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] > <mailto:[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] > <mailto:[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] > <mailto:[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 > <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] > <mailto:[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] >> <mailto:[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] >> <mailto:[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] >>> <mailto:[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. >> >
