Optimize PersistentGenericBag.EqualsSnapshot #2399
Conversation
| { | ||
| if (CountOccurrences(elt, _gbag, elementType) != CountOccurrences(elt, sn, elementType)) | ||
| { | ||
| if(elementType.IsSame(_gbag[i], sn[i])) |
There was a problem hiding this comment.
No need to count elements placed on the same positions.
There was a problem hiding this comment.
New white space glitch after if.
| continue; | ||
|
|
||
| var elt = _gbag[i]; | ||
| var countInSnapshot = CountOccurrences(elt, sn, elementType); |
There was a problem hiding this comment.
Added shortcut if element is no longer present in snapshot
bahusoid
force-pushed
the
bagSpanshotEquals
branch
from
1dca9d9 to
b743c9e
Compare
There was a problem hiding this comment.
I also had took a look to other EqualsSnapshot implementation, and I was wondering about optimizing this one.
But I was thinking about something much heavier : building dictionaries of bag elements. Not sure it is worth it.
Excepted for the space glitch, your proposal is ok for me.
| { | ||
| if (CountOccurrences(elt, _gbag, elementType) != CountOccurrences(elt, sn, elementType)) | ||
| { | ||
| if(elementType.IsSame(_gbag[i], sn[i])) |
There was a problem hiding this comment.
New white space glitch after if.
I don't think it's worth for bags. As I tried to explain in PR description this check is rarely executed and when executed in most cases it's unmodified collection. So added |
Added tests directly calling
EqualsSnapshotto verify it works correctly.Notes: This check is called when NHibernate detects cases when underlying collection can be modified directly in user code and not via
IPersistenCollectionwrapper, for instance array mappings andPersistentArryaHolder.For bags it's executed if new instance is created with bag then flushed (so
List<Entity>becomePersistentGenericBag). Check is executed for all additional flushes in this session as theoretically user can still have instance of original bagList<Entity>not wrapped inPersistentGenericBagthat can be modified directly