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