From 0cd9c10dabde1b8dd112ecf694ebce6ee058606f Mon Sep 17 00:00:00 2001 From: Gunnar Liljas Date: Fri, 26 Jan 2018 01:04:50 +0100 Subject: [PATCH 1/7] Improved collection batch fetching (HHH-1775) --- .../NHSpecificTest/SetFixture.cs | 2 + .../Async/Engine/BatchFetchQueue.cs | 45 ++++++-- .../AbstractPersistentCollection.cs | 2 +- src/NHibernate/Engine/BatchFetchQueue.cs | 108 ++++++++++++++---- src/NHibernate/Engine/CollectionEntry.cs | 4 + .../Engine/StatefulPersistenceContext.cs | 4 + src/NHibernate/Event/Default/EvictVisitor.cs | 4 + .../Collection/AbstractCollectionPersister.cs | 5 + .../Collection/BasicCollectionPersister.cs | 2 +- .../Collection/ICollectionPersister.cs | 2 + .../Collection/OneToManyPersister.cs | 2 +- src/NHibernate/Type/ManyToOneType.cs | 2 +- 12 files changed, 144 insertions(+), 38 deletions(-) diff --git a/src/NHibernate.Test/NHSpecificTest/SetFixture.cs b/src/NHibernate.Test/NHSpecificTest/SetFixture.cs index afad4dd973e..47c71a06814 100644 --- a/src/NHibernate.Test/NHSpecificTest/SetFixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/SetFixture.cs @@ -348,6 +348,8 @@ public object NotFoundObject get { throw new NotImplementedException(); } } + public int BatchSize { get; } = 0; + public ISessionFactoryImplementor Factory { get { return null; } diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 1587efd648e..710a309b175 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -15,6 +15,7 @@ using NHibernate.Persister.Entity; using NHibernate.Util; using System.Collections.Generic; +using Iesi.Collections.Generic; namespace NHibernate.Engine { @@ -40,23 +41,42 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect int end = -1; bool checkForEnd = false; - // this only works because collection entries are kept in a sequenced - // map by persistence context (maybe we should do like entities and - // keep a separate sequences set...) - foreach (DictionaryEntry me in context.CollectionEntries) + if (batchLoadableCollections.TryGetValue(collectionPersister.Role, out var map)) { - CollectionEntry ce = (CollectionEntry) me.Value; - IPersistentCollection collection = (IPersistentCollection) me.Key; - if (!collection.WasInitialized && ce.LoadedPersister == collectionPersister) + foreach (KeyValuePair me in map) { + 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 + continue; + } + + if (collection.WasInitialized) + { + // should never happen + if (log.IsWarnEnabled()) + { + log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); + } + continue; + } + if (checkForEnd && i == end) { return keys; //the first key found after the given key } - //if ( end == -1 && count > batchSize*10 ) return keys; //try out ten batches, max - - bool isEqual = collectionPersister.KeyType.IsEqual(id, ce.LoadedKey, collectionPersister.Factory); + var isEqual = collectionPersister.KeyType.IsEqual( + id, + ce.LoadedKey, + collectionPersister.Factory + ); if (isEqual) { @@ -79,6 +99,7 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect } } } + return keys; //we ran out of keys to try } @@ -101,9 +122,9 @@ public async Task GetEntityBatchAsync(IEntityPersister persister,objec int end = -1; bool checkForEnd = false; - foreach (EntityKey key in batchLoadableEntityKeys.Keys) + if (batchLoadableEntityKeys.TryGetValue(persister.EntityName,out var set))//TODO: this needn't exclude subclasses... { - if (key.EntityName.Equals(persister.EntityName)) + foreach (var key in set) { //TODO: this needn't exclude subclasses... if (checkForEnd && i == end) diff --git a/src/NHibernate/Collection/AbstractPersistentCollection.cs b/src/NHibernate/Collection/AbstractPersistentCollection.cs index a187d665464..76a5a960495 100644 --- a/src/NHibernate/Collection/AbstractPersistentCollection.cs +++ b/src/NHibernate/Collection/AbstractPersistentCollection.cs @@ -233,7 +233,7 @@ public virtual bool RowUpdatePossible } /// - protected virtual ISessionImplementor Session + public virtual ISessionImplementor Session { get { return session; } } diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index bca90051da5..a3cd45809a3 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -5,24 +5,24 @@ using NHibernate.Persister.Entity; using NHibernate.Util; using System.Collections.Generic; +using Iesi.Collections.Generic; namespace NHibernate.Engine { public partial class BatchFetchQueue { - private static readonly object Marker = new object(); + private static readonly INHibernateLogger log = NHibernateLogger.For(typeof(BatchFetchQueue)); /// - /// Defines a sequence of elements that are currently - /// eligible for batch fetching. + /// Used to hold information about the entities that are currently eligible for batch-fetching. Ultimately + /// used by to build entity load batches. /// /// - /// Even though this is a map, we only use the keys. A map was chosen in - /// order to utilize a to maintain sequencing - /// as well as uniqueness. + /// A Map structure is used to segment the keys by entity type since loading can only be done for a particular entity + /// type at a time. /// - private readonly IDictionary batchLoadableEntityKeys = new LinkedHashMap(8); - + private readonly IDictionary> batchLoadableEntityKeys = new Dictionary>(8); + /// /// A map of subselect-fetch descriptors /// keyed by the against which the descriptor is @@ -30,6 +30,7 @@ public partial class BatchFetchQueue /// private readonly IDictionary subselectsByEntityKey = new Dictionary(8); + private readonly IDictionary> batchLoadableCollections = new Dictionary>(8); /// /// The owning persistence context. /// @@ -50,6 +51,7 @@ public BatchFetchQueue(IPersistenceContext context) public void Clear() { batchLoadableEntityKeys.Clear(); + batchLoadableCollections.Clear(); subselectsByEntityKey.Clear(); } @@ -113,7 +115,12 @@ public void AddBatchLoadableEntityKey(EntityKey key) { if (key.IsBatchLoadable) { - batchLoadableEntityKeys[key] = Marker; + if (!batchLoadableEntityKeys.TryGetValue(key.EntityName, out var set)) + { + set = new LinkedHashSet(); + batchLoadableEntityKeys.Add(key.EntityName, set); + } + set.Add(key); } } @@ -125,7 +132,44 @@ public void AddBatchLoadableEntityKey(EntityKey key) public void RemoveBatchLoadableEntityKey(EntityKey key) { if (key.IsBatchLoadable) - batchLoadableEntityKeys.Remove(key); + { + if (batchLoadableEntityKeys.TryGetValue(key.EntityName, out var set)) + { + set.Remove(key); + } + } + } + + /// + /// If a CollectionEntry represents a batch loadable collection, add + /// it to the queue. + /// + /// + /// + public void AddBatchLoadableCollection(IPersistentCollection collection, CollectionEntry ce) + { + var persister = ce.LoadedPersister; + + if (!batchLoadableCollections.TryGetValue(persister.Role, out var map)) + { + map = new LinkedHashMap(); + batchLoadableCollections.Add(persister.Role, map); + } + map[ce]=collection; + } + + /// + /// After a collection was initialized or evicted, we don't + /// need to batch fetch it anymore, remove it from the queue + /// if necessary + /// + /// + public void RemoveBatchLoadableCollection(CollectionEntry ce) + { + if (batchLoadableCollections.TryGetValue(ce.LoadedPersister.Role, out var map)) + { + map.Remove(ce); + } } /// @@ -143,23 +187,42 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj int end = -1; bool checkForEnd = false; - // this only works because collection entries are kept in a sequenced - // map by persistence context (maybe we should do like entities and - // keep a separate sequences set...) - foreach (DictionaryEntry me in context.CollectionEntries) + if (batchLoadableCollections.TryGetValue(collectionPersister.Role, out var map)) { - CollectionEntry ce = (CollectionEntry) me.Value; - IPersistentCollection collection = (IPersistentCollection) me.Key; - if (!collection.WasInitialized && ce.LoadedPersister == collectionPersister) + foreach (KeyValuePair me in map) { + 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 + continue; + } + + if (collection.WasInitialized) + { + // should never happen + if (log.IsWarnEnabled()) + { + log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); + } + continue; + } + if (checkForEnd && i == end) { return keys; //the first key found after the given key } - //if ( end == -1 && count > batchSize*10 ) return keys; //try out ten batches, max - - bool isEqual = collectionPersister.KeyType.IsEqual(id, ce.LoadedKey, collectionPersister.Factory); + var isEqual = collectionPersister.KeyType.IsEqual( + id, + ce.LoadedKey, + collectionPersister.Factory + ); if (isEqual) { @@ -182,6 +245,7 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj } } } + return keys; //we ran out of keys to try } @@ -202,9 +266,9 @@ public object[] GetEntityBatch(IEntityPersister persister,object id,int batchSiz int end = -1; bool checkForEnd = false; - foreach (EntityKey key in batchLoadableEntityKeys.Keys) + if (batchLoadableEntityKeys.TryGetValue(persister.EntityName,out var set))//TODO: this needn't exclude subclasses... { - if (key.EntityName.Equals(persister.EntityName)) + foreach (var key in set) { //TODO: this needn't exclude subclasses... if (checkForEnd && i == end) diff --git a/src/NHibernate/Engine/CollectionEntry.cs b/src/NHibernate/Engine/CollectionEntry.cs index 7d8ee06aa45..ea231f46770 100644 --- a/src/NHibernate/Engine/CollectionEntry.cs +++ b/src/NHibernate/Engine/CollectionEntry.cs @@ -302,6 +302,10 @@ public void PostInitialize(IPersistentCollection collection) { snapshot = LoadedPersister.IsMutable ? collection.GetSnapshot(LoadedPersister) : null; collection.SetSnapshot(loadedKey, role, snapshot); + if (LoadedPersister.BatchSize > 1) + { + ((AbstractPersistentCollection) collection).Session.PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); + } } /// diff --git a/src/NHibernate/Engine/StatefulPersistenceContext.cs b/src/NHibernate/Engine/StatefulPersistenceContext.cs index cdb75ca6a7d..8a4678a492c 100644 --- a/src/NHibernate/Engine/StatefulPersistenceContext.cs +++ b/src/NHibernate/Engine/StatefulPersistenceContext.cs @@ -835,6 +835,10 @@ public void AddUninitializedCollection(ICollectionPersister persister, IPersiste { CollectionEntry ce = new CollectionEntry(collection, persister, id, flushing); AddCollection(collection, ce, id); + if (persister.BatchSize > 0) + { + batchFetchQueue.AddBatchLoadableCollection(collection, ce); + } } /// add a detached uninitialized collection diff --git a/src/NHibernate/Event/Default/EvictVisitor.cs b/src/NHibernate/Event/Default/EvictVisitor.cs index 82501c24d07..7764b2f2c45 100644 --- a/src/NHibernate/Event/Default/EvictVisitor.cs +++ b/src/NHibernate/Event/Default/EvictVisitor.cs @@ -53,6 +53,10 @@ private void EvictCollection(IPersistentCollection collection) Session.PersistenceContext.CollectionEntries.Remove(collection); if (log.IsDebugEnabled()) log.Debug("evicting collection: {0}", MessageHelper.CollectionInfoString(ce.LoadedPersister, collection, ce.LoadedKey, Session)); + if (ce.LoadedPersister?.BatchSize > 1) + { + Session.PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(ce); + } if (ce.LoadedPersister != null && ce.LoadedKey != null) { //TODO: is this 100% correct? diff --git a/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs b/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs index 27da5e4bc11..6bfe50dfd88 100644 --- a/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs @@ -2043,6 +2043,11 @@ public string[] RootTableKeyColumnNames get { return new string[] {IdentifierColumnName}; } } + public int BatchSize + { + get { return batchSize; } + } + public SqlString GetSelectByUniqueKeyString(string propertyName) { return diff --git a/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs b/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs index 315dc779f10..18ecf89b23f 100644 --- a/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs @@ -281,7 +281,7 @@ private string ManyToManySelectFragment(IJoinable rhs, string rhsAlias, string l /// protected override ICollectionInitializer CreateCollectionInitializer(IDictionary enabledFilters) { - return BatchingCollectionInitializer.CreateBatchingCollectionInitializer(this, batchSize, Factory, enabledFilters); + return BatchingCollectionInitializer.CreateBatchingCollectionInitializer(this, BatchSize, Factory, enabledFilters); } public override SqlString FromJoinFragment(string alias, bool innerJoin, bool includeSubclasses) diff --git a/src/NHibernate/Persister/Collection/ICollectionPersister.cs b/src/NHibernate/Persister/Collection/ICollectionPersister.cs index dc75daf4626..d44b376ecb0 100644 --- a/src/NHibernate/Persister/Collection/ICollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/ICollectionPersister.cs @@ -282,5 +282,7 @@ public partial interface ICollectionPersister /// A place-holder to inform that the data-reader was empty. /// object NotFoundObject { get; } + + int BatchSize { get; } } } diff --git a/src/NHibernate/Persister/Collection/OneToManyPersister.cs b/src/NHibernate/Persister/Collection/OneToManyPersister.cs index a2e1080164c..639ba74ab53 100644 --- a/src/NHibernate/Persister/Collection/OneToManyPersister.cs +++ b/src/NHibernate/Persister/Collection/OneToManyPersister.cs @@ -343,7 +343,7 @@ protected override SelectFragment GenerateSelectFragment(string alias, string co /// protected override ICollectionInitializer CreateCollectionInitializer(IDictionary enabledFilters) { - return BatchingCollectionInitializer.CreateBatchingOneToManyInitializer(this, batchSize, Factory, enabledFilters); + return BatchingCollectionInitializer.CreateBatchingOneToManyInitializer(this, BatchSize, Factory, enabledFilters); } public override SqlString FromJoinFragment(string alias, bool innerJoin, bool includeSubclasses) diff --git a/src/NHibernate/Type/ManyToOneType.cs b/src/NHibernate/Type/ManyToOneType.cs index 5f130dc8c35..4ac52cd54f2 100644 --- a/src/NHibernate/Type/ManyToOneType.cs +++ b/src/NHibernate/Type/ManyToOneType.cs @@ -101,7 +101,7 @@ private void ScheduleBatchLoadIfNeeded(object id, ISessionImplementor session) { IEntityPersister persister = session.Factory.GetEntityPersister(GetAssociatedEntityName()); EntityKey entityKey = session.GenerateEntityKey(id, persister); - if (!session.PersistenceContext.ContainsEntity(entityKey)) + if (entityKey.IsBatchLoadable && !session.PersistenceContext.ContainsEntity(entityKey)) { session.PersistenceContext.BatchFetchQueue.AddBatchLoadableEntityKey(entityKey); } From 4b404051244db68d0b006a9d4d09a3531bfbaf1f Mon Sep 17 00:00:00 2001 From: Gunnar Liljas Date: Sun, 28 Jan 2018 01:48:07 +0100 Subject: [PATCH 2/7] Fixes after code review --- .../Async/Engine/BatchFetchQueue.cs | 14 +++++-------- src/NHibernate/Engine/BatchFetchQueue.cs | 20 ++++++++----------- .../Engine/StatefulPersistenceContext.cs | 2 +- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 710a309b175..ec843e382c6 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -43,7 +43,7 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect if (batchLoadableCollections.TryGetValue(collectionPersister.Role, out var map)) { - foreach (KeyValuePair me in map) + foreach (KeyValuePair me in map) { var ce = me.Key; var collection = me.Value; @@ -59,11 +59,7 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect if (collection.WasInitialized) { - // should never happen - if (log.IsWarnEnabled()) - { - log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); - } + log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); continue; } @@ -99,7 +95,7 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect } } } - + return keys; //we ran out of keys to try } @@ -113,7 +109,7 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect /// The maximum number of keys to return /// A cancellation token that can be used to cancel the work /// an array of identifiers, of length batchSize (possibly padded with nulls) - public async Task GetEntityBatchAsync(IEntityPersister persister,object id,int batchSize, CancellationToken cancellationToken) + public async Task GetEntityBatchAsync(IEntityPersister persister, object id, int batchSize, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); object[] ids = new object[batchSize]; @@ -122,7 +118,7 @@ public async Task GetEntityBatchAsync(IEntityPersister persister,objec int end = -1; bool checkForEnd = false; - if (batchLoadableEntityKeys.TryGetValue(persister.EntityName,out var set))//TODO: this needn't exclude subclasses... + if (batchLoadableEntityKeys.TryGetValue(persister.EntityName, out var set)) { foreach (var key in set) { diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index a3cd45809a3..aa7ec616b39 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -22,7 +22,7 @@ public partial class BatchFetchQueue /// type at a time. /// private readonly IDictionary> batchLoadableEntityKeys = new Dictionary>(8); - + /// /// A map of subselect-fetch descriptors /// keyed by the against which the descriptor is @@ -30,7 +30,7 @@ public partial class BatchFetchQueue /// private readonly IDictionary subselectsByEntityKey = new Dictionary(8); - private readonly IDictionary> batchLoadableCollections = new Dictionary>(8); + private readonly IDictionary> batchLoadableCollections = new Dictionary>(8); /// /// The owning persistence context. /// @@ -155,7 +155,7 @@ public void AddBatchLoadableCollection(IPersistentCollection collection, Collect map = new LinkedHashMap(); batchLoadableCollections.Add(persister.Role, map); } - map[ce]=collection; + map[ce] = collection; } /// @@ -189,7 +189,7 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj if (batchLoadableCollections.TryGetValue(collectionPersister.Role, out var map)) { - foreach (KeyValuePair me in map) + foreach (KeyValuePair me in map) { var ce = me.Key; var collection = me.Value; @@ -205,11 +205,7 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj if (collection.WasInitialized) { - // should never happen - if (log.IsWarnEnabled()) - { - log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); - } + log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); continue; } @@ -245,7 +241,7 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj } } } - + return keys; //we ran out of keys to try } @@ -258,7 +254,7 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj /// The identifier of the entity currently demanding load. /// The maximum number of keys to return /// an array of identifiers, of length batchSize (possibly padded with nulls) - public object[] GetEntityBatch(IEntityPersister persister,object id,int batchSize) + public object[] GetEntityBatch(IEntityPersister persister, object id, int batchSize) { object[] ids = new object[batchSize]; ids[0] = id; //first element of array is reserved for the actual instance we are loading! @@ -266,7 +262,7 @@ public object[] GetEntityBatch(IEntityPersister persister,object id,int batchSiz int end = -1; bool checkForEnd = false; - if (batchLoadableEntityKeys.TryGetValue(persister.EntityName,out var set))//TODO: this needn't exclude subclasses... + if (batchLoadableEntityKeys.TryGetValue(persister.EntityName, out var set)) { foreach (var key in set) { diff --git a/src/NHibernate/Engine/StatefulPersistenceContext.cs b/src/NHibernate/Engine/StatefulPersistenceContext.cs index 8a4678a492c..3e414dc280c 100644 --- a/src/NHibernate/Engine/StatefulPersistenceContext.cs +++ b/src/NHibernate/Engine/StatefulPersistenceContext.cs @@ -835,7 +835,7 @@ public void AddUninitializedCollection(ICollectionPersister persister, IPersiste { CollectionEntry ce = new CollectionEntry(collection, persister, id, flushing); AddCollection(collection, ce, id); - if (persister.BatchSize > 0) + if (persister.BatchSize > 1) { batchFetchQueue.AddBatchLoadableCollection(collection, ce); } From 518797c00624f5106d61a8489a462ca423c1dd3a Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Sun, 28 Jan 2018 23:10:27 +1300 Subject: [PATCH 3/7] Remove breaking changes --- .../NHSpecificTest/SetFixture.cs | 2 -- .../Collection/AbstractPersistentCollection.cs | 10 +++++++++- .../Collection/IPersistentCollection.cs | 18 ++++++++++++++++++ src/NHibernate/Engine/CollectionEntry.cs | 4 ++-- .../Engine/StatefulPersistenceContext.cs | 2 +- src/NHibernate/Event/Default/EvictVisitor.cs | 3 ++- .../Collection/AbstractCollectionPersister.cs | 7 +++++-- .../Collection/BasicCollectionPersister.cs | 2 +- .../Collection/ICollectionPersister.cs | 18 +++++++++++++++++- .../Persister/Collection/OneToManyPersister.cs | 2 +- 10 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/NHibernate.Test/NHSpecificTest/SetFixture.cs b/src/NHibernate.Test/NHSpecificTest/SetFixture.cs index 47c71a06814..afad4dd973e 100644 --- a/src/NHibernate.Test/NHSpecificTest/SetFixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/SetFixture.cs @@ -348,8 +348,6 @@ public object NotFoundObject get { throw new NotImplementedException(); } } - public int BatchSize { get; } = 0; - public ISessionFactoryImplementor Factory { get { return null; } diff --git a/src/NHibernate/Collection/AbstractPersistentCollection.cs b/src/NHibernate/Collection/AbstractPersistentCollection.cs index 76a5a960495..a8c4a6f4e7d 100644 --- a/src/NHibernate/Collection/AbstractPersistentCollection.cs +++ b/src/NHibernate/Collection/AbstractPersistentCollection.cs @@ -233,7 +233,7 @@ public virtual bool RowUpdatePossible } /// - public virtual ISessionImplementor Session + protected virtual ISessionImplementor Session { get { return session; } } @@ -868,5 +868,13 @@ public abstract object ReadFrom(DbDataReader reader, ICollectionPersister role, */ #endregion + + /// + /// Get the session associated with the collection. + /// + public ISessionImplementor GetCurrentSession() + { + return session; + } } } diff --git a/src/NHibernate/Collection/IPersistentCollection.cs b/src/NHibernate/Collection/IPersistentCollection.cs index c16f4bef857..6448830d573 100644 --- a/src/NHibernate/Collection/IPersistentCollection.cs +++ b/src/NHibernate/Collection/IPersistentCollection.cs @@ -1,3 +1,4 @@ +using System; using System.Collections; using System.Data.Common; using NHibernate.Collection.Generic; @@ -337,4 +338,21 @@ public partial interface IPersistentCollection /// ICollection GetOrphans(object snapshot, string entityName); } + + public static class PersistentCollectionExtensions + { + /// + /// Get the session associated with the collection. + /// + //6.0 TODO: Merge into IPersistentCollection interface. Consider converting to a property. + public static ISessionImplementor GetCurrentSession(this IPersistentCollection collection) + { + if (collection is AbstractPersistentCollection apc) + { + return apc.GetCurrentSession(); + } + + throw new InvalidOperationException("Only collections of AbstractPersistentCollection are supported."); + } + } } diff --git a/src/NHibernate/Engine/CollectionEntry.cs b/src/NHibernate/Engine/CollectionEntry.cs index ea231f46770..6f515826907 100644 --- a/src/NHibernate/Engine/CollectionEntry.cs +++ b/src/NHibernate/Engine/CollectionEntry.cs @@ -302,9 +302,9 @@ public void PostInitialize(IPersistentCollection collection) { snapshot = LoadedPersister.IsMutable ? collection.GetSnapshot(LoadedPersister) : null; collection.SetSnapshot(loadedKey, role, snapshot); - if (LoadedPersister.BatchSize > 1) + if (LoadedPersister.GetBatchSize() > 1) { - ((AbstractPersistentCollection) collection).Session.PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); + collection.GetCurrentSession().PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); } } diff --git a/src/NHibernate/Engine/StatefulPersistenceContext.cs b/src/NHibernate/Engine/StatefulPersistenceContext.cs index 3e414dc280c..baf2bf842f3 100644 --- a/src/NHibernate/Engine/StatefulPersistenceContext.cs +++ b/src/NHibernate/Engine/StatefulPersistenceContext.cs @@ -835,7 +835,7 @@ public void AddUninitializedCollection(ICollectionPersister persister, IPersiste { CollectionEntry ce = new CollectionEntry(collection, persister, id, flushing); AddCollection(collection, ce, id); - if (persister.BatchSize > 1) + if (persister.GetBatchSize() > 1) { batchFetchQueue.AddBatchLoadableCollection(collection, ce); } diff --git a/src/NHibernate/Event/Default/EvictVisitor.cs b/src/NHibernate/Event/Default/EvictVisitor.cs index 7764b2f2c45..ce4a3c89a95 100644 --- a/src/NHibernate/Event/Default/EvictVisitor.cs +++ b/src/NHibernate/Event/Default/EvictVisitor.cs @@ -2,6 +2,7 @@ using NHibernate.Collection; using NHibernate.Engine; using NHibernate.Impl; +using NHibernate.Persister.Collection; using NHibernate.Type; namespace NHibernate.Event.Default @@ -53,7 +54,7 @@ private void EvictCollection(IPersistentCollection collection) Session.PersistenceContext.CollectionEntries.Remove(collection); if (log.IsDebugEnabled()) log.Debug("evicting collection: {0}", MessageHelper.CollectionInfoString(ce.LoadedPersister, collection, ce.LoadedKey, Session)); - if (ce.LoadedPersister?.BatchSize > 1) + if (ce.LoadedPersister?.GetBatchSize() > 1) { Session.PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(ce); } diff --git a/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs b/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs index 6bfe50dfd88..2611e99474d 100644 --- a/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs @@ -2043,9 +2043,12 @@ public string[] RootTableKeyColumnNames get { return new string[] {IdentifierColumnName}; } } - public int BatchSize + /// + /// Get the batch size of a collection persister. + /// + public int GetBatchSize() { - get { return batchSize; } + return batchSize; } public SqlString GetSelectByUniqueKeyString(string propertyName) diff --git a/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs b/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs index 18ecf89b23f..315dc779f10 100644 --- a/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/BasicCollectionPersister.cs @@ -281,7 +281,7 @@ private string ManyToManySelectFragment(IJoinable rhs, string rhsAlias, string l /// protected override ICollectionInitializer CreateCollectionInitializer(IDictionary enabledFilters) { - return BatchingCollectionInitializer.CreateBatchingCollectionInitializer(this, BatchSize, Factory, enabledFilters); + return BatchingCollectionInitializer.CreateBatchingCollectionInitializer(this, batchSize, Factory, enabledFilters); } public override SqlString FromJoinFragment(string alias, bool innerJoin, bool includeSubclasses) diff --git a/src/NHibernate/Persister/Collection/ICollectionPersister.cs b/src/NHibernate/Persister/Collection/ICollectionPersister.cs index d44b376ecb0..26449bc840f 100644 --- a/src/NHibernate/Persister/Collection/ICollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/ICollectionPersister.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Data.Common; using NHibernate.Cache; @@ -282,7 +283,22 @@ public partial interface ICollectionPersister /// A place-holder to inform that the data-reader was empty. /// object NotFoundObject { get; } + } + + public static class CollectionPersisterExtensions + { + /// + /// Get the batch size of a collection persister. + /// + //6.0 TODO: Merge into ICollectionPersister and convert to a property. + public static int GetBatchSize(this ICollectionPersister persister) + { + if (persister is AbstractCollectionPersister acp) + { + return acp.GetBatchSize(); + } - int BatchSize { get; } + throw new InvalidOperationException("Only persisters of AbstractCollectionPersister are supported."); + } } } diff --git a/src/NHibernate/Persister/Collection/OneToManyPersister.cs b/src/NHibernate/Persister/Collection/OneToManyPersister.cs index 639ba74ab53..a2e1080164c 100644 --- a/src/NHibernate/Persister/Collection/OneToManyPersister.cs +++ b/src/NHibernate/Persister/Collection/OneToManyPersister.cs @@ -343,7 +343,7 @@ protected override SelectFragment GenerateSelectFragment(string alias, string co /// protected override ICollectionInitializer CreateCollectionInitializer(IDictionary enabledFilters) { - return BatchingCollectionInitializer.CreateBatchingOneToManyInitializer(this, BatchSize, Factory, enabledFilters); + return BatchingCollectionInitializer.CreateBatchingOneToManyInitializer(this, batchSize, Factory, enabledFilters); } public override SqlString FromJoinFragment(string alias, bool innerJoin, bool includeSubclasses) From 066cd0ae5dff72922bc9627ecd6bd3a0ee82d7b5 Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Sun, 28 Jan 2018 23:39:26 +1300 Subject: [PATCH 4/7] CollectionPersisterExtensions.GetBatchSize returns 1 as a safe batch size if ICollectionPersister is not supported. --- src/NHibernate/Persister/Collection/ICollectionPersister.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/NHibernate/Persister/Collection/ICollectionPersister.cs b/src/NHibernate/Persister/Collection/ICollectionPersister.cs index 26449bc840f..4d87ea158b3 100644 --- a/src/NHibernate/Persister/Collection/ICollectionPersister.cs +++ b/src/NHibernate/Persister/Collection/ICollectionPersister.cs @@ -298,7 +298,11 @@ public static int GetBatchSize(this ICollectionPersister persister) return acp.GetBatchSize(); } - throw new InvalidOperationException("Only persisters of AbstractCollectionPersister are supported."); + NHibernateLogger + .For(typeof(CollectionPersisterExtensions)) + .Warn("Collection persister of {0} type is not supported, returning 1 as a batch size.", persister?.GetType()); + + return 1; } } } From 8e337eaa050766ed9415293d88792a82b8ff51ad Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Sun, 28 Jan 2018 23:57:11 +1300 Subject: [PATCH 5/7] Pass IPersistenceContext into PostInitialize --- .../Engine/Loading/CollectionLoadContext.cs | 12 +++++++----- ...DefaultInitializeCollectionEventListener.cs | 2 +- .../Collection/AbstractPersistentCollection.cs | 8 -------- .../Collection/IPersistentCollection.cs | 18 ------------------ src/NHibernate/Engine/CollectionEntry.cs | 13 ++++++++++++- .../Engine/Loading/CollectionLoadContext.cs | 12 +++++++----- .../Engine/StatefulPersistenceContext.cs | 2 +- ...DefaultInitializeCollectionEventListener.cs | 2 +- 8 files changed, 29 insertions(+), 40 deletions(-) diff --git a/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs b/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs index b4f81ed2c74..0ab01e61d22 100644 --- a/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs +++ b/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs @@ -128,7 +128,9 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec { log.Debug("ending loading collection [{0}]", lce); } - ISessionImplementor session = LoadContext.PersistenceContext.Session; + + var persistenceContext = LoadContext.PersistenceContext; + var session = persistenceContext.Session; bool statsEnabled = session.Factory.Statistics.IsStatisticsEnabled; var stopWath = new Stopwatch(); @@ -141,17 +143,17 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec if (persister.CollectionType.HasHolder()) { - LoadContext.PersistenceContext.AddCollectionHolder(lce.Collection); + persistenceContext.AddCollectionHolder(lce.Collection); } - CollectionEntry ce = LoadContext.PersistenceContext.GetCollectionEntry(lce.Collection); + CollectionEntry ce = persistenceContext.GetCollectionEntry(lce.Collection); if (ce == null) { - ce = LoadContext.PersistenceContext.AddInitializedCollection(persister, lce.Collection, lce.Key); + ce = persistenceContext.AddInitializedCollection(persister, lce.Collection, lce.Key); } else { - ce.PostInitialize(lce.Collection); + ce.PostInitialize(lce.Collection, persistenceContext); } bool addToCache = hasNoQueuedAdds && persister.HasCache && diff --git a/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs b/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs index d62cac434c8..1f0d706bed1 100644 --- a/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs +++ b/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs @@ -127,7 +127,7 @@ private async Task InitializeCollectionFromCacheAsync(object id, ICollecti CollectionCacheEntry cacheEntry = (CollectionCacheEntry)persister.CacheEntryStructure.Destructure(ce, factory); await (cacheEntry.AssembleAsync(collection, persister, persistenceContext.GetCollectionOwner(id, persister), cancellationToken)).ConfigureAwait(false); - persistenceContext.GetCollectionEntry(collection).PostInitialize(collection); + persistenceContext.GetCollectionEntry(collection).PostInitialize(collection, persistenceContext); return true; } } diff --git a/src/NHibernate/Collection/AbstractPersistentCollection.cs b/src/NHibernate/Collection/AbstractPersistentCollection.cs index a8c4a6f4e7d..a187d665464 100644 --- a/src/NHibernate/Collection/AbstractPersistentCollection.cs +++ b/src/NHibernate/Collection/AbstractPersistentCollection.cs @@ -868,13 +868,5 @@ public abstract object ReadFrom(DbDataReader reader, ICollectionPersister role, */ #endregion - - /// - /// Get the session associated with the collection. - /// - public ISessionImplementor GetCurrentSession() - { - return session; - } } } diff --git a/src/NHibernate/Collection/IPersistentCollection.cs b/src/NHibernate/Collection/IPersistentCollection.cs index 6448830d573..c16f4bef857 100644 --- a/src/NHibernate/Collection/IPersistentCollection.cs +++ b/src/NHibernate/Collection/IPersistentCollection.cs @@ -1,4 +1,3 @@ -using System; using System.Collections; using System.Data.Common; using NHibernate.Collection.Generic; @@ -338,21 +337,4 @@ public partial interface IPersistentCollection /// ICollection GetOrphans(object snapshot, string entityName); } - - public static class PersistentCollectionExtensions - { - /// - /// Get the session associated with the collection. - /// - //6.0 TODO: Merge into IPersistentCollection interface. Consider converting to a property. - public static ISessionImplementor GetCurrentSession(this IPersistentCollection collection) - { - if (collection is AbstractPersistentCollection apc) - { - return apc.GetCurrentSession(); - } - - throw new InvalidOperationException("Only collections of AbstractPersistentCollection are supported."); - } - } } diff --git a/src/NHibernate/Engine/CollectionEntry.cs b/src/NHibernate/Engine/CollectionEntry.cs index 6f515826907..6db8b8c9811 100644 --- a/src/NHibernate/Engine/CollectionEntry.cs +++ b/src/NHibernate/Engine/CollectionEntry.cs @@ -302,9 +302,20 @@ public void PostInitialize(IPersistentCollection collection) { snapshot = LoadedPersister.IsMutable ? collection.GetSnapshot(LoadedPersister) : null; collection.SetSnapshot(loadedKey, role, snapshot); + } + + /// + /// Updates the CollectionEntry to reflect that the + /// has been initialized. + /// + /// The initialized that this Entry is for. + /// + public void PostInitialize(IPersistentCollection collection, IPersistenceContext persistenceContext) + { + PostInitialize(collection); if (LoadedPersister.GetBatchSize() > 1) { - collection.GetCurrentSession().PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); + persistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); } } diff --git a/src/NHibernate/Engine/Loading/CollectionLoadContext.cs b/src/NHibernate/Engine/Loading/CollectionLoadContext.cs index 9d87fc4d044..9dd97e4351e 100644 --- a/src/NHibernate/Engine/Loading/CollectionLoadContext.cs +++ b/src/NHibernate/Engine/Loading/CollectionLoadContext.cs @@ -234,7 +234,9 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist { log.Debug("ending loading collection [{0}]", lce); } - ISessionImplementor session = LoadContext.PersistenceContext.Session; + + var persistenceContext = LoadContext.PersistenceContext; + var session = persistenceContext.Session; bool statsEnabled = session.Factory.Statistics.IsStatisticsEnabled; var stopWath = new Stopwatch(); @@ -247,17 +249,17 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist if (persister.CollectionType.HasHolder()) { - LoadContext.PersistenceContext.AddCollectionHolder(lce.Collection); + persistenceContext.AddCollectionHolder(lce.Collection); } - CollectionEntry ce = LoadContext.PersistenceContext.GetCollectionEntry(lce.Collection); + CollectionEntry ce = persistenceContext.GetCollectionEntry(lce.Collection); if (ce == null) { - ce = LoadContext.PersistenceContext.AddInitializedCollection(persister, lce.Collection, lce.Key); + ce = persistenceContext.AddInitializedCollection(persister, lce.Collection, lce.Key); } else { - ce.PostInitialize(lce.Collection); + ce.PostInitialize(lce.Collection, persistenceContext); } bool addToCache = hasNoQueuedAdds && persister.HasCache && diff --git a/src/NHibernate/Engine/StatefulPersistenceContext.cs b/src/NHibernate/Engine/StatefulPersistenceContext.cs index baf2bf842f3..709e8a0de01 100644 --- a/src/NHibernate/Engine/StatefulPersistenceContext.cs +++ b/src/NHibernate/Engine/StatefulPersistenceContext.cs @@ -917,7 +917,7 @@ public CollectionEntry AddInitializedCollection(ICollectionPersister persister, object id) { CollectionEntry ce = new CollectionEntry(collection, persister, id, flushing); - ce.PostInitialize(collection); + ce.PostInitialize(collection, this); AddCollection(collection, ce, id); return ce; } diff --git a/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs b/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs index c2d65277a60..2f37f28d1ea 100644 --- a/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs +++ b/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs @@ -115,7 +115,7 @@ private bool InitializeCollectionFromCache(object id, ICollectionPersister persi CollectionCacheEntry cacheEntry = (CollectionCacheEntry)persister.CacheEntryStructure.Destructure(ce, factory); cacheEntry.Assemble(collection, persister, persistenceContext.GetCollectionOwner(id, persister)); - persistenceContext.GetCollectionEntry(collection).PostInitialize(collection); + persistenceContext.GetCollectionEntry(collection).PostInitialize(collection, persistenceContext); return true; } } From 488618bcb33de96e5fff4ec82a38f90a46a73bc1 Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Wed, 31 Jan 2018 12:05:32 +1300 Subject: [PATCH 6/7] Obsolete PostInitialize with single argument --- src/NHibernate/Engine/CollectionEntry.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/NHibernate/Engine/CollectionEntry.cs b/src/NHibernate/Engine/CollectionEntry.cs index 6db8b8c9811..131ec2e365d 100644 --- a/src/NHibernate/Engine/CollectionEntry.cs +++ b/src/NHibernate/Engine/CollectionEntry.cs @@ -298,6 +298,8 @@ public void PreFlush(IPersistentCollection collection) /// has been initialized. /// /// The initialized that this Entry is for. + //Since v5.1 + [Obsolete("Please use PostInitialize(collection, persistenceContext) instead.")] public void PostInitialize(IPersistentCollection collection) { snapshot = LoadedPersister.IsMutable ? collection.GetSnapshot(LoadedPersister) : null; @@ -312,7 +314,10 @@ public void PostInitialize(IPersistentCollection collection) /// public void PostInitialize(IPersistentCollection collection, IPersistenceContext persistenceContext) { +#pragma warning disable 618 + //6.0 TODO: Inline PostInitialize here. PostInitialize(collection); +#pragma warning restore 618 if (LoadedPersister.GetBatchSize() > 1) { persistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); From c0e0d559400d80ab9ba80869a30e29fb73f937b7 Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Wed, 31 Jan 2018 13:15:00 +1300 Subject: [PATCH 7/7] Remove redundant code changes --- src/NHibernate/Async/Engine/BatchFetchQueue.cs | 6 +----- src/NHibernate/Engine/BatchFetchQueue.cs | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index ec843e382c6..470619da3d5 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -68,11 +68,7 @@ public async Task GetCollectionBatchAsync(ICollectionPersister collect return keys; //the first key found after the given key } - var isEqual = collectionPersister.KeyType.IsEqual( - id, - ce.LoadedKey, - collectionPersister.Factory - ); + bool isEqual = collectionPersister.KeyType.IsEqual(id, ce.LoadedKey, collectionPersister.Factory); if (isEqual) { diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index aa7ec616b39..dd668788379 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -214,11 +214,7 @@ public object[] GetCollectionBatch(ICollectionPersister collectionPersister, obj return keys; //the first key found after the given key } - var isEqual = collectionPersister.KeyType.IsEqual( - id, - ce.LoadedKey, - collectionPersister.Factory - ); + bool isEqual = collectionPersister.KeyType.IsEqual(id, ce.LoadedKey, collectionPersister.Factory); if (isEqual) {