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