UpdateTimestampsCache non cached entities concurrency issue

[gokhanabatay]

Hi @hazzik @maca88,
As I mentioned earlier current UpdateTimestampsCache has still concurrency issue. #2129

In real life scenario some of entities uses second level cache but most of entities does not uses second level cache but still try to update UpdateTimestampsCache. At least it should be configurable in entity level or session factory level, default may be true for backward compability issue.

Non cached entities could be related(foreign key) with cached entity may be better solution StandardQueryCache property spaces and cached entity property spaces only triggers UpdateTimestampsCache?

Current UpdateTimestampsCache uses AsyncReaderWriterLock if one writer grants lock other writers of different entities or readers wait for it. If this is true you could realize the concurrency issue that we are facing under high concurrency (400-500tps).

If still not acceptable please consider a way to implement Custom UpdateTimestampsCache via UpdateTimestampsFactory like cache.provider_class attribute in configuration. Could be named cache.update_timestamps_class.

Currently we are trying to replace current UpdateTimestampsCache via reflection but its not cleanest solution.

var cacheablePersisters = ISessionFactory.GetAllClassMetadata()
                .Where(x => x.Value.GetType() == typeof(SingleTableEntityPersister))
                .Select(x => x.Value)
                .Cast<SingleTableEntityPersister>()
                .Where(x => x.HasCache)
                .ToList();

var sessionFactoryImpl = (SessionFactoryImpl)ISessionFactory;
var standardQueryCache = (StandardQueryCache)sessionFactoryImpl.QueryCache;


var updateTimestamps = (CacheBase)typeof(UpdateTimestampsCache).GetField("_updateTimestamps", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(sessionFactoryImpl.UpdateTimestampsCache);

var updateTimestampsCache = new DistributedUpdateTimestampsCache(updateTimestamps, cacheablePersisters.SelectMany(x => x.PropertySpaces).ToHashSet());

typeof(SessionFactoryImpl).GetField("updateTimestampsCache", BindingFlags.Instance | BindingFlags.NonPublic).SetValue(sessionFactoryImpl, updateTimestampsCache);

if (standardQueryCache != null)
{
    typeof(StandardQueryCache).GetField("_updateTimestampsCache", BindingFlags.Instance | BindingFlags.NonPublic).SetValue(standardQueryCache, updateTimestampsCache);
}

public class DistributedUpdateTimestampsCache : UpdateTimestampsCache
{
    protected readonly ISet<string> CacheableSpaces;

    public DistributedUpdateTimestampsCache(CacheBase updateTimestamps, ISet<string> cacheableSpaces) : base(updateTimestamps)
    {
        CacheableSpaces = cacheableSpaces;
    }

    public override void Invalidate(IReadOnlyCollection<string> spaces)
    {
        spaces = spaces.Where(x => CacheableSpaces.Contains(x)).ToList();

        if (spaces.Any())
        {
            base.Invalidate(spaces);
        }
    }

    public override void PreInvalidate(IReadOnlyCollection<string> spaces)
    {
        spaces = spaces.Where(x => CacheableSpaces.Contains(x)).ToList();

        if (spaces.Any())
        {
            base.PreInvalidate(spaces);
        }
    }

    public override Task InvalidateAsync(IReadOnlyCollection<string> spaces, CancellationToken cancellationToken)
    {
        if (cancellationToken.IsCancellationRequested)
        {
            return Task.FromCanceled<object>(cancellationToken);
        }

        spaces = spaces.Where(x => CacheableSpaces.Contains(x)).ToList();

        if (!spaces.Any())
        {
            return Task.CompletedTask;
        }

        return base.InvalidateAsync(spaces, cancellationToken);
    }

    public override Task PreInvalidateAsync(IReadOnlyCollection<string> spaces, CancellationToken cancellationToken)
    {
        if (cancellationToken.IsCancellationRequested)
        {
            return Task.FromCanceled<object>(cancellationToken);
        }

        spaces = spaces.Where(x => CacheableSpaces.Contains(x)).ToList();

        if (!spaces.Any())
        {
            return Task.CompletedTask;
        }

        return base.PreInvalidateAsync(spaces, cancellationToken);
    }
}

[maca88]

The simplest solution would be to remove the locking entierly from UpdateTimestampsCache, which is what Hibernate has done a long time ago to improve performance. In theory the read/write locking prevents that IsUpToDate would return stale data, but that holds true only when one instance of the application is running. In case multiple application instances are running then IsUpToDate may return stale data due to another application instance writing data at the same time. So we could remove the locking entirely as Hiberanate did or add a configuration setting for disabling it.

[gokhanabatay]

I agree @maca88 best solution remove locking entierly from UpdateTimestampsCache also configuration setting needed for non cached entities. If we are not interested in caching for an entity, in classs mapping Cache.Never() mapping confuguration could be implemented. Also configuration level ignore would be great.

Why an entity dml blocks another entity read, it's not occurred when we are using Cache.NonStrictReadWrite();

And you are right our application is running on openshift, 25 application (minimum 2 pods every app) rely on same local distributed cache. UpdateTimestampsCache is important for clients to detect stale data but not important as locking whole application.

[gokhanabatay]

@maca88 I am willing to implement this feature if you can give a little guidance.

I agree @maca88 best solution remove locking entierly from UpdateTimestampsCache also configuration setting needed for non cached entities. If we are not interested in caching for an entity, in classs mapping Cache.Never() mapping confuguration could be implemented. Also configuration level ignore would be great.

Why an entity dml blocks another entity read, it's not occurred when we are using Cache.NonStrictReadWrite();

And you are right our application is running on openshift, 25 application (minimum 2 pods every app) rely on same local distributed cache. UpdateTimestampsCache is important for clients to detect stale data but not important as locking whole application.

What do you think about the offer and how should we proceed, which branch should I use as a base?

[hazzik]

which branch should I use as a base?

master.

[maca88]

Non cached entities could be related(foreign key) with cached entity may be better solution StandardQueryCache property spaces and cached entity property spaces only triggers UpdateTimestampsCache?

Updating timestamps for only cached entities is not a valid approach. Consider this query:

session.Query<User>()
.WithOptions(o => o.SetCacheable(true))
.Where(o => o.Organization.Name == "Name1")
.Select(o => new
{
  o.Id,
  o.Name,
  OrganizationName = o.Organization.Name
})
.ToList();

where User is cached but Organization is not. In case the Organization name is changed, your DistributedUpdateTimestampsCache will not update the Organization timestamp and the query will still return the same result even after the Organization name was changed. I made a working example to show that. By commenting these lines the test then passes.

In short, with your DistributedUpdateTimestampsCache, every change to a non cached entity may produce an outdated result for a cached query that references them. In case the cached entities are rarely changed the outdated cached result may be used for days, which can cause several bugs. I think that by removing the read/write locking (#2742), the performance should be much better and you won't need to use a custom UpdateTimestampsCache anymore.

[gokhanabatay]

@maca88 When we use redis pub/sub distributed cache in your implementation non cached entities causes too much synchronization events. Think about a configuration in entity mapping Cache.Never() that tells this entity never cached with query cache or entity cache ever.

If this approach is ok I could implement this in NHibernate and FluentNHibernate libraries.
For performance needs this is absolutely necessary.

There is also locks in NHibernate.Caches.StackExchangeRedis.DistributedLocalCacheRegionStrategy

[maca88]

non cached entities causes too much syncronization events

The DistributedLocalCacheRegionStrategy was never meant to be used for caches that have a lot for writing, you would probably have a better performance by using either TwoLayerCacheRegionStrategy or DefaultRegionStrategy for them.

Think about a configuration in entity mapping Cache.Never() that tells this entity never cached with query cache or entity cache ever.

How would Cache.Never() work in case the non cached entity is used in a cached query like in the example above. Should Cache.Never() override the o.SetCacheable(true) set on the query?

For performance needs this is absolutly necessary.

In order to further improve the UpdateTimestampsCache performance, you could create a special region strategy for it, which would combine approaches used in DistributedLocalCacheRegionStrategy and TwoLayerCacheRegionStrategy. The strategy could have the following functionalities:

• A local cache that would be used for retrieving/storing the values, similar to DistributedLocalCacheRegionStrategy
• Every X seconds the local cache would synchronize its data with the Redis cache. The Redis cache would be used to synchronize the data among local caches similar to TwoLayerCacheRegionStrategy
• As UpdateTimestampsCache stores only timestamps and does not use the Remove method, the synchronization logic can be simplified by only checking the timestamps and storing always the latest one

By synchronizing the local cache data every X seconds, you gain performance at a cost of potentially having stale data that is up to X seconds old.

[gokhanabatay]

How would Cache.Never() work in case the non cached entity is used in a cached query like in the example above. Should Cache.Never() override the o.SetCacheable(true) set on the query?

Current Entity Mapping Strategies

namespace FluentNHibernate.Mapping
{
    public class CachePart : ICacheMappingProvider
    {
        public CachePart(Type entityType);
        public CachePart CustomInclude(string custom);
        public CachePart CustomUsage(string custom);
        public CachePart IncludeAll();
        public CachePart IncludeNonLazy();
        public CachePart NonStrictReadWrite();
        public CachePart ReadOnly();
        public CachePart ReadWrite();
        public CachePart Region(string name);
        public CachePart Transactional();
    }
}

I thought that we could add new strategy Cache.Never() as a new feature.
If an entity mapped as Cache.Never() we could throw an exception that tells entity is not cacheable, so you cannot use in query cache with o.SetCacheable(true). StandardQueryCache holds QuerySpaces so we could easily check it before execution.
Then don't update for Cache.Never() entities UpdateTimestampsCache ever.

As its a new feature so it would not be a breaking change.

Don't get me wrong removing lock at UpdateTimestampsCache(#2742) is a good start point, as you mentioned I could implement in custom region strategy. Just trying to improve NHibernate if it's make sense.

[maca88]

If an entity mapped as Cache.Never() we could throw an exception that tells entity is not cacheable, so you cannot use in query cache with o.SetCacheable(true)

I am fine with that, feel free to create a PR for it.

[gliljas]

But UpdateTimestampsCache and entity cachability are still orthogonal things and shouldn't be connected. Being able to easily circumvent the behavior with a custom UpdateTimestampsCache implementation seems like a feasible solution.

[gliljas]

If an entity mapped as Cache.Never() we could throw an exception that tells entity is not cacheable, so you cannot use in query cache with o.SetCacheable(true)

I am fine with that, feel free to create a PR for it.

I disagree. SetCacheable shouldn't be the cause an exception, IMO. It's a hint, not a request.

Having Cache.Never as a setting to prevent certain entities (or columns/properties) from ever entering a cache could be useful.