Is there a way to detect whether we are running in Graal? LieGrue, strub
> Am 29.07.2020 um 08:46 schrieb Romain Manni-Bucau <[email protected]>: > > FYI, created https://issues.apache.org/jira/browse/OWB-1343 > <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] > <mailto:[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] >> <mailto:[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. >>> >> >
