[infinispan-dev] Time measurement and expiry

[Dan Berindei]

On Mon, Oct 17, 2011 at 4:13 PM, Sanne Grinovero <sanne at infinispan.org> wrote:
> On 17 October 2011 09:02, Dan Berindei <dan.berindei at gmail.com> wrote:
>> On Sat, Oct 15, 2011 at 7:25 PM, Sanne Grinovero <sanne at infinispan.org> wrote:
>>> Hi all,
>>> I'm looking at some performance details, and noticed the current code
>>> is using System.currentTimeMillis() to estimate performance for short
>>> lived operations.
>>>
>>> For the purpose of CacheMgmtInterceptor we should use System.nanoTime() instead:
>>> http://blogs.oracle.com/dholmes/entry/inside_the_hotspot_vm_clocks
>>>
>>
>> Very interesting, I knew that in Windows currentTimeMillis() basically
>> just reads a volatile because I got bit by the 15 millisecond accuracy
>> issue before, so I thought it would always be very fast. I had no idea
>> on Linux it would have the same performance as nanoTime().
>
> Even in Windows it seems to be a bit more expensive than just reading
> a volatile;
>

The article you linked to said it's about 6 cycles, I don't think it
can do much more than reading a variable and returning it in that
time.

>>> Searching for "currentTimeMillis()" in the code base reveals it's
>>> being used for expiry of entries as well. If someone messes with
>>> system clocks data will expire; is it expected that entries expire at
>>> calendar time and not at a set lifetime?
>>
>> I would say if someone's messing with system clocks they already have
>> a much bigger problem than their cached data expiring...
>
> Absolutely, but this is just an example. I'd expect that if I'm using
> it as a cache, the data will stay in it for a certain amount of
> relative time.
> It doesn't have to be a user changing the clock, for example there
> might be automated changes like daylight adjustments (summer/winter
> time) or relatively small updates via NTP to keep the clocks in sync..
> of course they might not affect the functionality significantly, but
> I'm just pointing out that we might prefer to use the nanoTime source
> as our API suggests a period of entry validity in milliseconds, not an
> expiry in terms of calendar Date.
>

Daylight changes don't modify the UTC time, so
System.currentTimeMillis() should not be affected.

It's true that we only offer a validity period in milliseconds, but
I'm sure if we had a Date-based alternative many people would use that
instead. In the end I don't think the extra complexity is worth it, at
least until somebody actually asks for it.

>> Besides, someone may intentionally change the system time to test the
>> ISPN expiration algorithm :)
>>
>>> I understand we need to use the absolute time to be able to transmit
>>> the value to other nodes, where the nanoTime of different nodes might
>>> not be comparable, but we could store one and use both: transmit the
>>> absolute value only to other nodes and refer to the other for accurate
>>> expiries.
>>> Other nodes receiving the absolute value will check for the remaining
>>> lifespane and store the corresponding nanoTime.
>>>
>>> ExpiryHelper itself will invoke the currentTimeMillis() operation
>>> internally, that means that it's going to be invoked at least once for
>>> each entry being accessed and might result in a lot of invocations
>>> when traversing several entries; I'm wondering if it shouldn't take a
>>> millisecond parameter to consider as current, so that this relatively
>>> expensive method can be invoked only once at the beginning of a batch
>>> of operations.
>>>
>>
>> + 1 for adding a currentTime parameter to the isExpiredX() methods in
>> ExpiryHelper and also to isExpired() CacheEntry and its
>> implementations.
>
> Agreed: ISPN-1459
>
>>> Also reading this time information is a high overhead in some
>>> configurations, I'm wondering if we should make it possible to
>>> configure Infinispan to not track performance on each cache operation?
>>> Someone might prefer to estimate an average from multiple calls; I'm
>>> going to remove CacheMgmtInterceptor for my tests.
>>>
>>
>> CacheMgmtInterceptor doesn't seem to be enabled when JMX statistics
>> are disabled, so I don't think you need to explicitly remove
>> CacheMgmtInterceptor for your performance tests.
>
> Yes that's how I do it ;)
> What concerns me is that there is going to be a relatively strong
> overhead just by enabling statistics.
>

Makes sense, many people will probably have their own tooling to
measure their app's performance. We could remove this from the JMX
statistics (at least by default) but make sure we have the hooks in
place for the users to integrate with a different monitoring system.