Implement multiple get and put for query cache and query batch #1788

fredericDelaporte · 2018-07-09T09:05:20Z

This is a follow up to @maca88 #1633 (batching 2nd level cache operations) and @bahusoid #1718 (query batch). It aims at combining both: enabling batching of 2nd level cache operations for the query batch.

fredericDelaporte · 2018-07-09T09:10:22Z

src/NHibernate/Cache/CacheBatcher.cs

 	/// from an <see cref="IEntityPersister"/> or <see cref="ICollectionPersister"/>.
 	/// When a different persister or a different operation is added to the batch, the current batch will be executed.
 	/// </summary>
-	internal partial class CacheBatcher
+	public sealed partial class CacheBatcher


For enabling batching of puts across batched queries, I had to transmit a CacheBatcher with a public interface method, so I had to switch this type to public.

fredericDelaporte · 2018-07-09T09:12:01Z

src/NHibernate/Cache/CacheBatcher.cs

@@ -34,7 +30,7 @@ public CacheBatcher(ISessionImplementor session)
 		/// </summary>
 		/// <param name="persister">The entity persister.</param>
 		/// <param name="data">The data to put in the cache.</param>
-		public void AddToBatch(IEntityPersister persister, CachePutData data)
+		internal void AddToBatch(IEntityPersister persister, CachePutData data)


CachePutDatabeing internal, the method has to be internal too since the declaring class is no more internal but public. For consistency and has users should not need to use directly the class, but just have to pass it around, I have switched all the methods to internal.

fredericDelaporte · 2018-07-09T09:16:16Z

src/NHibernate/Cache/StandardQueryCache.cs

 	{
 		private static readonly INHibernateLogger Log = NHibernateLogger.For(typeof (StandardQueryCache));
 		private readonly ICache _queryCache;
+		private readonly IBatchableReadOnlyCache _batchableReadOnlyCache;
+		private readonly IBatchableCache _batchableCache;


#1777 (CacheBase) would allow a simpler implementation here.

fredericDelaporte · 2018-07-09T09:23:15Z

src/NHibernate/Cache/StandardQueryCache.cs

+			if (queryParameters.IsReadOnlyInitialized)
+				persistenceContext.DefaultReadOnly = queryParameters.ReadOnly;
+			else
+				queryParameters.ReadOnly = persistenceContext.DefaultReadOnly;


Here is the reason for the need of QueryParameters: handling the readonly flag for loading entities. With single gets, it can be done on the caller side.
But with multiple gets, it has to be done on the callee side. Unless gets are sorted out according to their readonly status matching or not the one of the persistence context (which is not trivial to handle) and then called as separated multiple gets. I considered it would complicate the code a bit to much and so choose to move this handling in the query cache.
(For the record, multi-query/criteria were not handling the readonly flag of each query they were containing.)

fredericDelaporte · 2018-07-09T09:37:49Z

src/NHibernate/Cache/IQueryCache.cs

 		bool Put(QueryKey key, ICacheAssembler[] returnTypes, IList result, bool isNaturalKeyLookup, ISessionImplementor session);
+		// Since 5.2
+		[Obsolete("Have the query cache implement IBatchableQueryCache, and use IBatchableQueryCache.Get")]
 		IList Get(QueryKey key, ICacheAssembler[] returnTypes, bool isNaturalKeyLookup, ISet<string> spaces, ISessionImplementor session);


In order to properly handle the multi-get cases, I had to transmit the QueryParameters. So for consistency, I have made the same change to the single Get and to Put methods.

src/NHibernate/Multi/QueryBatch.cs

@@ -44,18 +47,17 @@ public void Execute()
 			{
 				Init();

-				if (!Session.Factory.ConnectionProvider.Driver.SupportsMultipleQueries)
+				using (Session.BeginProcess())


src/NHibernate/Multi/QueryBatch.cs

-				using (Session.BeginProcess())
-				{
-					DoExecute();
+					ExecuteBatched();


src/NHibernate/Multi/QueryBatch.cs

 			{
+				// The auto-flush must be handled before obtaining the commands, because an auto-flush
+				// may have to invalidate cached data which otherwise would cause a command to be skipped.
 				Session.AutoFlushIfRequired(querySpaces);


src/NHibernate/Multi/QueryBatch.cs

-				stopWatch.Start();
-			}
+			var resultSetsCommand = Session.Factory.ConnectionProvider.Driver.GetResultSetsCommand(Session);
+			CombineQueries(resultSetsCommand, GetCachedResults());


src/NHibernate/Multi/QueryBatchItemBase.cs

-					if (index == _queryInfos.Count - 1)
-					{
-						InitializeEntitiesAndCollections(reader, hydratedObjects);
-					}


maca88 · 2018-07-09T18:36:31Z

src/NHibernate/Cache/StandardQueryCache.cs

+					//      the UnresolvableObjectException could occur while resolving
+					//      associations, leaving the PC in an inconsistent state
+					Log.Debug(ex, "could not reassemble cached result set");
+					// TODO: add and handle a RemoveMany?


I forgot about RemoveMany when doing #1633 as it was not needed there, but it is already implemented in nhibernate/NHibernate-Caches#45, so why not add it if it can be used here.

I have put a question mark because this case is almost an error case, and it does not look likely to me it would benefit from batching. It should not happen frequently, and even less frequently would it benefit from a Many handling, since the first encountered one short-circuit the result-set processing: it could be batched only in multi-queries case, if many queries in it encounter a "no more resolvable entity reference" case.

So in fact I should change that comment, I do not think it would be worth it to handle a many case here.

src/NHibernate/Multi/IQueryBatchItem.cs

-		IEnumerable<ISqlCommand> GetCommands();
+		/// <param name="cachedResults">The cached results.</param>
+		/// <returns>The commands for obtaining the results not already cached.</returns>
+		IEnumerable<ISqlCommand> GetCommands(IDictionary<ICachingInformation, IList> cachedResults);


Additional cleanup/doc originating from nhibernate#1788 but which should be here

fredericDelaporte · 2018-07-12T00:06:11Z

Squashed and rebased for resolving conflicts with #1789.

hazzik

Some code stlyles

src/NHibernate/Cache/IQueryCache.cs

 	/// </summary>
 	public partial interface IQueryCache
 	{
+		/// <summary>
+		/// The underlying <see cref="ICache"/>.
+		/// </summary>
 		ICache Cache { get; }


hazzik · 2018-07-09T11:39:54Z

src/NHibernate/Cache/IQueryCache.cs

 		void Clear();
+		// Since 5.2


Need a new line before

hazzik · 2018-07-09T11:40:04Z

src/NHibernate/Cache/IQueryCache.cs

 		bool Put(QueryKey key, ICacheAssembler[] returnTypes, IList result, bool isNaturalKeyLookup, ISessionImplementor session);
+		// Since 5.2


Need a new line before

hazzik · 2018-07-09T11:40:36Z

src/NHibernate/Cache/IQueryCache.cs

 		IList Get(QueryKey key, ICacheAssembler[] returnTypes, bool isNaturalKeyLookup, ISet<string> spaces, ISessionImplementor session);
+		/// <summary>


A new line before please

fredericDelaporte · 2018-07-12T00:07:33Z

src/NHibernate/Multi/QueryBatchItemBase.cs

+		{
+			get
+			{
+				ThrowIfNotInitialized();


This one would have been better moved in #1789, but I forgot it.

fredericDelaporte · 2018-07-12T00:11:10Z

src/NHibernate/Multi/QueryBatchItemBase.cs

@@ -243,8 +294,15 @@ private void InitializeEntitiesAndCollections(DbDataReader reader, List<object>[
 				if (queryInfo.IsResultFromCache)
 					continue;
 				queryInfo.Loader.InitializeEntitiesAndCollections(
-					hydratedObjects[i], reader, Session, Session.PersistenceContext.DefaultReadOnly);
+					hydratedObjects[i], reader, Session, queryInfo.Parameters.IsReadOnly(Session),


This change would also have been better moved in #1789. The old code was from multi-query/criteria, which was not knowing for which queries they were initializing and so could not handle their readonly flag.

Adjust formatting

bahusoid · 2018-07-12T06:33:30Z

src/NHibernate/Multi/QueryBatch.cs

+
+		private void CombineQueries(IResultSetsCommand resultSetsCommand)
+		{
+			foreach (var multiSource in _queries)


This multiSource name is leftover of initial interface namings. Maybe it's better to rename it simply to query (if you agree - please note that this name is used mulitple times in this file)

src/NHibernate/Multi/QueryBatchItemBase.cs

@@ -208,16 +258,17 @@ public void ExecuteNonBatched()

 		protected List<T> GetTypedResults<T>()
 		{
-			if (_loaderResults == null)
+			ThrowIfNotInitialized();
+			if (_queryInfos.Any(qi => qi.Result == null))


bahusoid · 2018-07-12T08:12:15Z

src/NHibernate/Loader/Loader.cs

-		internal IList GetResultFromQueryCache(ISessionImplementor session, QueryParameters queryParameters,
-											  ISet<string> querySpaces, IQueryCache queryCache,
-											  QueryKey key)
+		internal bool CanGetFromCache(ISessionImplementor session, QueryParameters queryParameters)


This method doesn't handle per query SetCacheMode calls. Which actually now is used in tests (as no op but might lead to confusion that it actually can be used in Future):

nhibernate-core/src/NHibernate.Test/Futures/QueryBatchFixture.cs

Lines 354 to 355 in 7ce79ed

.SetCacheMode(CacheMode.Normal)

.Future<EntitySimpleChild>()

Maybe this query cache mode needs to be moved to QueryParameters and handled similarly as QueryParameters.ReadOnly flag?

So we should move this code to QueryParameters.CanGetFromCache(session) method. And for parity introduce QueryParameters.CanPutToCache(session)

Indeed, the queries individual cache mode has never been handled for future and multi-query/criteria, but we could. I do not think it should be done here, but better as another PR.

Yes I agree - per query cacheMode support should be done as another PR.

But to minimize "change noise" I think it's better to introduce here QueryParameters.CanGetFromCache(session) and use it instead of this method (and also introduce QueryParameters.CanPutToCache(session) with simple session check). Again I see no difference why this usage should be different from QueryParameters.IsReadOnly(session) - so those checks should be implemented and used the same way.

And dedicated "per query cacheMode" PR will simiply fill those 2 methods with proper implementation.

I think it will not work, so better stick to YAGNI, not doing this in advance.

CacheMode needs to be temporarily switched on the session instead, because many cache interactions triggered deeply in the loading implementation have no access to query parameters.

For this PR I'm just saying that it's better to have consistent way for readonly check for query and cache mode check for query. So from this point - what will not work?
We use queryParameters.IsReadOnly(session) for readonly check. So it looks logical to me to use queryParameters.CanGetFromCache(session) for cache mode check. So why introduce new logically similair method in Loader instead? It can be still marked internal here if you think this "cache mode per query" won't work.

These cases are not comparable in my opinion:

IsReadonly is a bit messy, originating from times where nullable were not avaliable. It is also there to hide the complexity of replacing nullability by an additional boolean.

CacheMode is not a boolean but a flag enumeration value. It conveys more state than a boolean. CanGetFromCache is just a small part of it, because by example it tells nothing about what should be done with entities assembled from their id cached in query result, whether they should themselves be put into the cache or not if they were missing in it when re-assembled.
So it would be just a partial part of the information we actually need to handle properly cache mode, and we will have to put more somehow somewhere. It then looks likely to me that CanGetFromCache will just be redundant and not needed.

I'm not talking about how data needs to be stored. But rather how it should be used in code.
Calling queryParameters.IsReadOnly(session) for checking query read-only flag looks pretty clean to me. How messy its implementation is irrelevant.
And as we need now method CanGetFromCache I think place and synatx should be the same. But let's not spend more time on it. If you are not convinced - let's keep it as is for time being.

You were right with the need for CanGetFromCache and CanPutToCache, but I still think it was better to do it in a dedicated PR.

src/NHibernate/Multi/QueryBatch.cs

bahusoid · 2018-07-12T08:28:53Z

src/NHibernate/Multi/ICachingInformation.cs

+		/// <summary>
+		/// The query result.
+		/// </summary>
+		IList Result { get; }


May be it should be exposed as GetResultsToCache or some other name that suggests that implementor should return values that's going to be put to cache.

If doing so, it should then hold neither results retrieved from cache nor results after cacheable transformation which should not be put (the put must be done on the untransformed result).

Adjust code according to review

Adjust some naming

Adjust some logs & messages

bahusoid

No more complaints from my part. And thanks for fixing the mess I made :)

fredericDelaporte · 2018-07-13T14:04:48Z

I validated it, so it was also my responsibility.

fredericDelaporte added t: Improvement c: Core p: Minor labels Jul 9, 2018

fredericDelaporte added this to the 5.2 milestone Jul 9, 2018

fredericDelaporte force-pushed the multiCacheQueryBatch branch 4 times, most recently from a721e4f to 38c3c53 Compare July 9, 2018 10:49

fredericDelaporte commented Jul 9, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

maca88 reviewed Jul 9, 2018

View reviewed changes

bahusoid reviewed Jul 10, 2018

View reviewed changes

fredericDelaporte added a commit to fredericDelaporte/nhibernate-core that referenced this pull request Jul 10, 2018

fixup! Fix shortcomings of QueryBatcher initial implementation

8883f6c

Additional cleanup/doc originating from nhibernate#1788 but which should be here

fredericDelaporte mentioned this pull request Jul 10, 2018

Fix shortcomings of QueryBatcher initial implementation #1789

Merged

Implement multiple get and put for query cache and query batch

5d4f46f

fredericDelaporte force-pushed the multiCacheQueryBatch branch from 1be92d8 to 5d4f46f Compare July 12, 2018 00:02

hazzik reviewed Jul 12, 2018

View reviewed changes

fredericDelaporte commented Jul 12, 2018

View reviewed changes

fixup! Implement multiple get and put for query cache and query batch

afe7130

Adjust formatting

bahusoid reviewed Jul 12, 2018

View reviewed changes

fredericDelaporte force-pushed the multiCacheQueryBatch branch from 61cce23 to 8eabc3c Compare July 12, 2018 11:17

fixup! Implement multiple get and put for query cache and query batch

455f664

Adjust code according to review

fredericDelaporte force-pushed the multiCacheQueryBatch branch from 8eabc3c to 455f664 Compare July 12, 2018 11:21

fredericDelaporte added 2 commits July 12, 2018 14:11

fixup! Implement multiple get and put for query cache and query batch

39ea112

Adjust some naming

fixup! Implement multiple get and put for query cache and query batch

f87b030

Adjust some logs & messages

bahusoid approved these changes Jul 12, 2018

View reviewed changes

fredericDelaporte merged commit eda5fc9 into nhibernate:master Jul 13, 2018

fredericDelaporte deleted the multiCacheQueryBatch branch July 13, 2018 14:05

fredericDelaporte added the r: Fixed label Jul 13, 2018

This was referenced Jul 13, 2018

Replace ICache interface by a CacheBase class #1777

Merged

Fix cache build for honoring mapped concurrency #1480

Merged

Obsolete MultiQuery and MultiCriteria #1783

Merged

		bool Put(QueryKey key, ICacheAssembler[] returnTypes, IList result, bool isNaturalKeyLookup, ISessionImplementor session);
		// Since 5.2

		IList Get(QueryKey key, ICacheAssembler[] returnTypes, bool isNaturalKeyLookup, ISet<string> spaces, ISessionImplementor session);
		/// <summary>

Implement multiple get and put for query cache and query batch #1788

Implement multiple get and put for query cache and query batch #1788

Conversation

fredericDelaporte commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as outdated.

This comment was marked as off-topic.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

fredericDelaporte commented Jul 12, 2018

hazzik left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

bahusoid Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahusoid Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahusoid left a comment

Choose a reason for hiding this comment

fredericDelaporte commented Jul 13, 2018

bahusoid Jul 12, 2018 •

edited

Loading

bahusoid Jul 12, 2018 •

edited

Loading