From aa4e0ae6293c9df8b74b9d3ead027029847987a1 Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Fri, 22 Feb 2019 13:31:28 +0200 Subject: [PATCH 1/3] Avoid recursive calls in BatchFetchQueue --- .../Async/Engine/BatchFetchQueue.cs | 138 +++++++++--------- src/NHibernate/Engine/BatchFetchQueue.cs | 15 +- 2 files changed, 78 insertions(+), 75 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 4a20cad9677..36f757948c2 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -74,7 +74,7 @@ internal async Task GetCollectionBatchAsync(ICollectionPersister colle foreach (KeyValuePair me in map) { cancellationToken.ThrowIfCancellationRequested(); - if (await (ProcessKeyAsync(me)).ConfigureAwait(false)) + if (await (ProcessKeyAndCheckCacheAsync(me)).ConfigureAwait(false)) { return keys; } @@ -107,7 +107,7 @@ async Task CheckCacheAndProcessResultAsync() { for (var j = 0; j < collectionKeys.Count; j++) { - if (await (ProcessKeyAsync(collectionKeys[indexes[j]].Key)).ConfigureAwait(false)) + if (ProcessKey(collectionKeys[indexes[j]].Key) == true) { return true; } @@ -118,7 +118,7 @@ async Task CheckCacheAndProcessResultAsync() var results = await (AreCachedAsync(collectionKeys, indexes, collectionPersister, batchableCache, checkCache, cancellationToken)).ConfigureAwait(false); for (var j = 0; j < results.Length; j++) { - if (!results[j] && await (ProcessKeyAsync(collectionKeys[indexes[j]].Key, true)).ConfigureAwait(false)) + if (!results[j] && ProcessKey(collectionKeys[indexes[j]].Key, true) == true) { return true; } @@ -132,91 +132,89 @@ async Task CheckCacheAndProcessResultAsync() return false; } - Task ProcessKeyAsync(KeyValuePair me, bool ignoreCache = false) + async Task ProcessKeyAndCheckCacheAsync(KeyValuePair me) { - try + return ProcessKey(me) ?? await (CheckCacheAndProcessResultAsync()).ConfigureAwait(false); + } + + bool? ProcessKey(KeyValuePair me, bool ignoreCache = false) + { + var ce = me.Key; + var collection = me.Value; + if (ce.LoadedKey == null) { - var ce = me.Key; - var collection = me.Value; - if (ce.LoadedKey == null) - { - // the LoadedKey of the CollectionEntry might be null as it might have been reset to null - // (see for example Collections.ProcessDereferencedCollection() - // and CollectionEntry.AfterAction()) - // though we clear the queue on flush, it seems like a good idea to guard - // against potentially null LoadedKey:s - return Task.FromResult(false); - } + // the LoadedKey of the CollectionEntry might be null as it might have been reset to null + // (see for example Collections.ProcessDereferencedCollection() + // and CollectionEntry.AfterAction()) + // though we clear the queue on flush, it seems like a good idea to guard + // against potentially null LoadedKey:s + return false; + } - if (collection.WasInitialized) - { - log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); - return Task.FromResult(false); - } + if (collection.WasInitialized) + { + log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); + return false; + } - if (checkForEnd && (index == map.Count || index >= keyIndex.Value + batchSize)) + if (checkForEnd && (index == map.Count || index >= keyIndex.Value + batchSize)) + { + return true; + } + if (collectionPersister.KeyType.IsEqual(key, ce.LoadedKey, collectionPersister.Factory)) + { + if (collectionEntries != null) { - return Task.FromResult(true); + collectionEntries[0] = ce; } - if (collectionPersister.KeyType.IsEqual(key, ce.LoadedKey, collectionPersister.Factory)) + keyIndex = index; + } + else if (!checkCache || batchableCache == null) + { + if (index < map.Count && (!keyIndex.HasValue || index < keyIndex.Value)) { - if (collectionEntries != null) - { - collectionEntries[0] = ce; - } - keyIndex = index; + collectionKeys.Add(new KeyValuePair, int>(me, index)); + return false; } - else if (!checkCache || batchableCache == null) - { - if (index < map.Count && (!keyIndex.HasValue || index < keyIndex.Value)) - { - collectionKeys.Add(new KeyValuePair, int>(me, index)); - return Task.FromResult(false); - } - // No need to check "!checkCache || !IsCached(ce.LoadedKey, collectionPersister)": - // "batchableCache == null" already means there is no cache, so IsCached can only yield false. - // (This method is now removed.) - if (collectionEntries != null) - { - collectionEntries[i] = ce; - } - keys[i++] = ce.LoadedKey; - } - else if (ignoreCache) + // No need to check "!checkCache || !IsCached(ce.LoadedKey, collectionPersister)": + // "batchableCache == null" already means there is no cache, so IsCached can only yield false. + // (This method is now removed.) + if (collectionEntries != null) { - if (collectionEntries != null) - { - collectionEntries[i] = ce; - } - keys[i++] = ce.LoadedKey; + collectionEntries[i] = ce; } - else + keys[i++] = ce.LoadedKey; + } + else if (ignoreCache) + { + if (collectionEntries != null) { - collectionKeys.Add(new KeyValuePair, int>(me, index)); - // Check the cache only when we have collected as many keys as are needed to fill the batch, - // that are after the demanded key. - if (!keyIndex.HasValue || index < keyIndex.Value + batchSize) - { - return Task.FromResult(false); - } - return CheckCacheAndProcessResultAsync(); + collectionEntries[i] = ce; } - if (i == batchSize) + keys[i++] = ce.LoadedKey; + } + else + { + collectionKeys.Add(new KeyValuePair, int>(me, index)); + // Check the cache only when we have collected as many keys as are needed to fill the batch, + // that are after the demanded key. + if (!keyIndex.HasValue || index < keyIndex.Value + batchSize) { - i = 1; // End of array, start filling again from start - if (index == map.Count || keyIndex.HasValue) - { - checkForEnd = true; - return Task.FromResult(index == map.Count || index >= keyIndex.Value + batchSize); - } + return false; } - return Task.FromResult(false); + return null; } - catch (Exception ex) + if (i == batchSize) { - return Task.FromException(ex); + i = 1; // End of array, start filling again from start + if (index == map.Count || keyIndex.HasValue) + { + checkForEnd = true; + return index == map.Count || index >= keyIndex.Value + batchSize; + } } + return false; } } diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index 2a4c61160f4..85336414162 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -244,7 +244,7 @@ internal object[] GetCollectionBatch(ICollectionPersister collectionPersister, o foreach (KeyValuePair me in map) { - if (ProcessKey(me)) + if (ProcessKeyAndCheckCache(me)) { return keys; } @@ -276,7 +276,7 @@ bool CheckCacheAndProcessResult() { for (var j = 0; j < collectionKeys.Count; j++) { - if (ProcessKey(collectionKeys[indexes[j]].Key)) + if (ProcessKey(collectionKeys[indexes[j]].Key) == true) { return true; } @@ -287,7 +287,7 @@ bool CheckCacheAndProcessResult() var results = AreCached(collectionKeys, indexes, collectionPersister, batchableCache, checkCache); for (var j = 0; j < results.Length; j++) { - if (!results[j] && ProcessKey(collectionKeys[indexes[j]].Key, true)) + if (!results[j] && ProcessKey(collectionKeys[indexes[j]].Key, true) == true) { return true; } @@ -301,7 +301,12 @@ bool CheckCacheAndProcessResult() return false; } - bool ProcessKey(KeyValuePair me, bool ignoreCache = false) + bool ProcessKeyAndCheckCache(KeyValuePair me) + { + return ProcessKey(me) ?? CheckCacheAndProcessResult(); + } + + bool? ProcessKey(KeyValuePair me, bool ignoreCache = false) { var ce = me.Key; var collection = me.Value; @@ -367,7 +372,7 @@ bool ProcessKey(KeyValuePair me, bool ig { return false; } - return CheckCacheAndProcessResult(); + return null; } if (i == batchSize) { From 77c7ea453490d934bd6ba84192d7e785ba7b327d Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Fri, 22 Feb 2019 13:50:38 +0200 Subject: [PATCH 2/3] Better async key processing --- src/NHibernate/Async/Engine/BatchFetchQueue.cs | 7 +------ src/NHibernate/Engine/BatchFetchQueue.cs | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 36f757948c2..5d1a2340b24 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -74,7 +74,7 @@ internal async Task GetCollectionBatchAsync(ICollectionPersister colle foreach (KeyValuePair me in map) { cancellationToken.ThrowIfCancellationRequested(); - if (await (ProcessKeyAndCheckCacheAsync(me)).ConfigureAwait(false)) + if (ProcessKey(me) ?? await (CheckCacheAndProcessResultAsync()).ConfigureAwait(false)) { return keys; } @@ -132,11 +132,6 @@ async Task CheckCacheAndProcessResultAsync() return false; } - async Task ProcessKeyAndCheckCacheAsync(KeyValuePair me) - { - return ProcessKey(me) ?? await (CheckCacheAndProcessResultAsync()).ConfigureAwait(false); - } - bool? ProcessKey(KeyValuePair me, bool ignoreCache = false) { var ce = me.Key; diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index 85336414162..d428ce3c64f 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -244,7 +244,7 @@ internal object[] GetCollectionBatch(ICollectionPersister collectionPersister, o foreach (KeyValuePair me in map) { - if (ProcessKeyAndCheckCache(me)) + if (ProcessKey(me) ?? CheckCacheAndProcessResult()) { return keys; } @@ -301,11 +301,6 @@ bool CheckCacheAndProcessResult() return false; } - bool ProcessKeyAndCheckCache(KeyValuePair me) - { - return ProcessKey(me) ?? CheckCacheAndProcessResult(); - } - bool? ProcessKey(KeyValuePair me, bool ignoreCache = false) { var ce = me.Key; From 17680eaa9463166837e14c8bb9e5867cc27252dc Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Sat, 23 Feb 2019 19:42:19 +1300 Subject: [PATCH 3/3] Avoid recursive calls in BatchFetchQueue --- .../Async/Engine/BatchFetchQueue.cs | 91 +++++++++---------- src/NHibernate/Engine/BatchFetchQueue.cs | 10 +- 2 files changed, 47 insertions(+), 54 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 5d1a2340b24..b91583c7e2f 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -266,7 +266,7 @@ internal async Task GetEntityBatchAsync(IEntityPersister persister, ob foreach (var key in set) { cancellationToken.ThrowIfCancellationRequested(); - if (await (ProcessKeyAsync(key)).ConfigureAwait(false)) + if (ProcessKey(key) ?? await (CheckCacheAndProcessResultAsync()).ConfigureAwait(false)) { return ids; } @@ -299,7 +299,7 @@ async Task CheckCacheAndProcessResultAsync() { for (var j = 0; j < entityKeys.Count; j++) { - if (await (ProcessKeyAsync(entityKeys[indexes[j]].Key)).ConfigureAwait(false)) + if (ProcessKey(entityKeys[indexes[j]].Key) == true) { return true; } @@ -310,7 +310,7 @@ async Task CheckCacheAndProcessResultAsync() var results = await (AreCachedAsync(entityKeys, indexes, persister, batchableCache, checkCache, cancellationToken)).ConfigureAwait(false); for (var j = 0; j < results.Length; j++) { - if (!results[j] && await (ProcessKeyAsync(entityKeys[indexes[j]].Key, true)).ConfigureAwait(false)) + if (!results[j] && ProcessKey(entityKeys[indexes[j]].Key, true) == true) { return true; } @@ -324,62 +324,55 @@ async Task CheckCacheAndProcessResultAsync() return false; } - Task ProcessKeyAsync(EntityKey key, bool ignoreCache = false) + bool? ProcessKey(EntityKey key, bool ignoreCache = false) { - try + //TODO: this needn't exclude subclasses... + if (checkForEnd && (index == set.Count || index >= idIndex.Value + batchSize)) { - //TODO: this needn't exclude subclasses... - if (checkForEnd && (index == set.Count || index >= idIndex.Value + batchSize)) - { - return Task.FromResult(true); - } - if (persister.IdentifierType.IsEqual(id, key.Identifier)) - { - idIndex = index; - } - else if (!checkCache || batchableCache == null) - { - if (index < set.Count && (!idIndex.HasValue || index < idIndex.Value)) - { - entityKeys.Add(new KeyValuePair(key, index)); - return Task.FromResult(false); - } - - // No need to check "!checkCache || !IsCached(key, persister)": "batchableCache == null" - // already means there is no cache, so IsCached can only yield false. (This method is now - // removed.) - ids[i++] = key.Identifier; - } - else if (ignoreCache) - { - ids[i++] = key.Identifier; - } - else + return true; + } + if (persister.IdentifierType.IsEqual(id, key.Identifier)) + { + idIndex = index; + } + else if (!checkCache || batchableCache == null) + { + if (index < set.Count && (!idIndex.HasValue || index < idIndex.Value)) { entityKeys.Add(new KeyValuePair(key, index)); - // Check the cache only when we have collected as many keys as are needed to fill the batch, - // that are after the demanded key. - if (!idIndex.HasValue || index < idIndex.Value + batchSize) - { - return Task.FromResult(false); - } - return CheckCacheAndProcessResultAsync(); + return false; } - if (i == batchSize) + + // No need to check "!checkCache || !IsCached(key, persister)": "batchableCache == null" + // already means there is no cache, so IsCached can only yield false. (This method is now + // removed.) + ids[i++] = key.Identifier; + } + else if (ignoreCache) + { + ids[i++] = key.Identifier; + } + else + { + entityKeys.Add(new KeyValuePair(key, index)); + // Check the cache only when we have collected as many keys as are needed to fill the batch, + // that are after the demanded key. + if (!idIndex.HasValue || index < idIndex.Value + batchSize) { - i = 1; // End of array, start filling again from start - if (index == set.Count || idIndex.HasValue) - { - checkForEnd = true; - return Task.FromResult(index == set.Count || index >= idIndex.Value + batchSize); - } + return false; } - return Task.FromResult(false); + return null; } - catch (Exception ex) + if (i == batchSize) { - return Task.FromException(ex); + i = 1; // End of array, start filling again from start + if (index == set.Count || idIndex.HasValue) + { + checkForEnd = true; + return index == set.Count || index >= idIndex.Value + batchSize; + } } + return false; } } diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index d428ce3c64f..abef4f568bc 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -427,7 +427,7 @@ internal object[] GetEntityBatch(IEntityPersister persister, object id, int batc foreach (var key in set) { - if (ProcessKey(key)) + if (ProcessKey(key) ?? CheckCacheAndProcessResult()) { return ids; } @@ -459,7 +459,7 @@ bool CheckCacheAndProcessResult() { for (var j = 0; j < entityKeys.Count; j++) { - if (ProcessKey(entityKeys[indexes[j]].Key)) + if (ProcessKey(entityKeys[indexes[j]].Key) == true) { return true; } @@ -470,7 +470,7 @@ bool CheckCacheAndProcessResult() var results = AreCached(entityKeys, indexes, persister, batchableCache, checkCache); for (var j = 0; j < results.Length; j++) { - if (!results[j] && ProcessKey(entityKeys[indexes[j]].Key, true)) + if (!results[j] && ProcessKey(entityKeys[indexes[j]].Key, true) == true) { return true; } @@ -484,7 +484,7 @@ bool CheckCacheAndProcessResult() return false; } - bool ProcessKey(EntityKey key, bool ignoreCache = false) + bool? ProcessKey(EntityKey key, bool ignoreCache = false) { //TODO: this needn't exclude subclasses... if (checkForEnd && (index == set.Count || index >= idIndex.Value + batchSize)) @@ -521,7 +521,7 @@ bool ProcessKey(EntityKey key, bool ignoreCache = false) { return false; } - return CheckCacheAndProcessResult(); + return null; } if (i == batchSize) {