Re:[configuration] Performance issue in FileReloadingStrategy

[Oliver Heger <he...@med.uni-marburg.de>]

Hi Jorge,

very good catch! I will update my patch to reflect your suggestions.

I think I will give Emmanuel, who is the original author, some time to 
react. But if he doesn't object, I will commit the changes in a few days.

Thanks.
Oliver

Jorge Ferrer wrote:
> Hi Oliver,
> 
> I've been doing some performance test and when using
> FileReloadingStrategy it takes about 60 times longer to retrieve a
> property value even when the file does not change. I've been digging
> in the code and I think that I've found the reason. The method
> reloadingRequired() of FileChangedReloadingStrategy does:
>     public boolean reloadingRequired(){
>         boolean reloading = false;
>         long now = System.currentTimeMillis();
>         if ((now > lastChecked + refreshDelay) && hasChanged()) {
>             lastChecked = now;
>             reloading = true;
>         }
>         return reloading;
>     }
> Which means that lastChecked is only updated when the file actually
> changes. This means that the hasChanged() method is called (and the
> file system is queried for the last modified date) every time a
> property value is retrieved!. The solution is very simple. Just modify
> this method to:
>     public boolean reloadingRequired(){
>         boolean reloading = false;
>         long now = System.currentTimeMillis();
>         if (now > lastChecked + refreshDelay) {
>             lastChecked = now;
>             if (hasChanged()) {
>                 reloading = true;
>             }
>         }
>         return reloading;
>     } 
> 
> Another possibility is updating lastChecked on the method hasChanged()
> 
> what you think about this?
> 
> Regards,
> Jorge
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [configuration] Performance issue in FileReloadingStrategy

[Emmanuel Bourg <eb...@apache.org>]

Sorry for the late response but I'm just starting to catch up with my 
remaining unanswered mails.

The refresh delay wasn't meant to define a delay between the file 
checks, but a minimum delay between the reloads. I understand the need 
to limit the file checks if it has a non negligeable performance impact 
though. My only concern is that the variable name and the javadoc no 
longer describe its new purpose. We may have to rename it into 
"checkInterval" or "checkPeriod" and deprecated the old accessors.

Emmanuel Bourg


Oliver Heger wrote:
> Hi Jorge,
> 
> very good catch! I will update my patch to reflect your suggestions.
> 
> I think I will give Emmanuel, who is the original author, some time to 
> react. But if he doesn't object, I will commit the changes in a few days.
> 
> Thanks.
> Oliver
> 
> Jorge Ferrer wrote:
> 
>> Hi Oliver,
>>
>> I've been doing some performance test and when using
>> FileReloadingStrategy it takes about 60 times longer to retrieve a
>> property value even when the file does not change. I've been digging
>> in the code and I think that I've found the reason. The method
>> reloadingRequired() of FileChangedReloadingStrategy does:
>>     public boolean reloadingRequired(){
>>         boolean reloading = false;
>>         long now = System.currentTimeMillis();
>>         if ((now > lastChecked + refreshDelay) && hasChanged()) {
>>             lastChecked = now;
>>             reloading = true;
>>         }
>>         return reloading;
>>     }
>> Which means that lastChecked is only updated when the file actually
>> changes. This means that the hasChanged() method is called (and the
>> file system is queried for the last modified date) every time a
>> property value is retrieved!. The solution is very simple. Just modify
>> this method to:
>>     public boolean reloadingRequired(){
>>         boolean reloading = false;
>>         long now = System.currentTimeMillis();
>>         if (now > lastChecked + refreshDelay) {
>>             lastChecked = now;
>>             if (hasChanged()) {
>>                 reloading = true;
>>             }
>>         }
>>         return reloading;
>>     }
>> Another possibility is updating lastChecked on the method hasChanged()
>>
>> what you think about this?
>>
>> Regards,
>> Jorge
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>>
>>
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> 
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org