Improved collection batch fetching #1558
Conversation
| map = new LinkedHashMap<CollectionEntry, IPersistentCollection>(); | ||
| batchLoadableCollections.Add(persister.Role, map); | ||
| } | ||
| map[ce]=collection; |
There was a problem hiding this comment.
Some formatting is lacking here.
| private readonly IDictionary<EntityKey, object> batchLoadableEntityKeys = new LinkedHashMap<EntityKey, object>(8); | ||
|
|
||
| private readonly IDictionary<string, LinkedHashSet<EntityKey>> batchLoadableEntityKeys = new Dictionary<string, LinkedHashSet<EntityKey>>(8); | ||
There was a problem hiding this comment.
Undue whitespace addition.
| CollectionEntry ce = (CollectionEntry) me.Value; | ||
| IPersistentCollection collection = (IPersistentCollection) me.Key; | ||
| if (!collection.WasInitialized && ce.LoadedPersister == collectionPersister) | ||
| foreach (KeyValuePair<CollectionEntry,IPersistentCollection> me in map) |
There was a problem hiding this comment.
Missing space after comma.
| if (log.IsWarnEnabled()) | ||
| { | ||
| log.Warn("Encountered initialized collection in BatchFetchQueue, this should not happen."); | ||
| } |
There was a problem hiding this comment.
I personally tend to minimize such code by eliminating the "isEnabled" guard for simple logs, since the logger itself is supposed to test this. I keep the guard only if the log involve more than calling the logger, like having some string computing before calling it.
The "should never happen" comment is also a bit redundant with the log message. So I would tend to only have one line here instead of five, the one doing the log.
| @@ -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... | |||
There was a problem hiding this comment.
Missing space after comma. And I would rather have the comment on the line above instead of at end of line. I view most end of line comments as bad practice, as they can lack visibility.
| { | ||
| if (key.EntityName.Equals(persister.EntityName)) | ||
| foreach (var key in set) | ||
| { | ||
| //TODO: this needn't exclude subclasses... |
There was a problem hiding this comment.
This comments now looks redundant.
| @@ -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) | |||
There was a problem hiding this comment.
Why some code test > 0 and other > 1? Shouldn't it be > 1 everywhere?
hazzik
force-pushed
the
CollectionBatchFetching
branch
from
4f195c4 to
518797c
Compare
|
I've removed breaking changes. Please review. |
| @@ -302,6 +302,10 @@ public void PostInitialize(IPersistentCollection collection) | |||
| { | |||
| snapshot = LoadedPersister.IsMutable ? collection.GetSnapshot(LoadedPersister) : null; | |||
| collection.SetSnapshot(loadedKey, role, snapshot); | |||
| if (LoadedPersister.GetBatchSize() > 1) | |||
| { | |||
| collection.GetCurrentSession().PersistenceContext.BatchFetchQueue.RemoveBatchLoadableCollection(this); | |||
There was a problem hiding this comment.
It is no more a binary breaking change but it is still a breaking one, isn't it? Because if anyone supplies its own IPersistentCollection not deriving from AbstractPersistentCollection, it will now throw here.
There was a problem hiding this comment.
What if returning 0/1 on get batch size if this is not supported one?
| @@ -835,6 +835,10 @@ public void AddUninitializedCollection(ICollectionPersister persister, IPersiste | |||
| { | |||
| CollectionEntry ce = new CollectionEntry(collection, persister, id, flushing); | |||
| AddCollection(collection, ce, id); | |||
| if (persister.GetBatchSize() > 1) | |||
There was a problem hiding this comment.
Same here but for ICollectionPersister.
| return acp.GetBatchSize(); | ||
| } | ||
|
|
||
| throw new InvalidOperationException("Only persisters of AbstractCollectionPersister are supported."); |
There was a problem hiding this comment.
@fredericDelaporte I think returning 1 here would fix this possible breaking change, wouldn't it?
…size if ICollectionPersister is not supported.
|
Ok, now it does not have any breaking changes. |
|
@hazzik Nice! |
| @@ -302,9 +302,20 @@ public void PostInitialize(IPersistentCollection collection) | |||
| { | |||
There was a problem hiding this comment.
The single argument PostInitialize should be obsoleted. Or is there any reason to keep it, beside avoiding a breaking change? It seems now only called from the two arguments PostInitialize.
|
As for #1560, I consider this one, if squashed, should be squashed by Alexander for slightly better attribution, since he has significantly contributed to it. |
Implemented as it was done in hibernate/hibernate-orm#337. HHH-1775