IsDirty performance hit since 5.4.0

[trivalik]

I faced a longer IsDirty run after upgrading.

NHibernate 3.3.1:

NHibernate 5.4.2:

The first question is, was there a bug in old implementation that now lead to this cascade?
Since the result of IsDirty is true in both cases, I can imagine that there is optimization potential.

The test case of IsDirty is there that we have a table which stores for every element a parent element (not lazy load). The children are as HasMany relation. We remove just near all elements, and this lead to this bad result.

[hazzik]

@trivalik can you share full screenshot for 3.3.1 and also expand all trees for CascadeToOne?

[trivalik]

For 3.3.1 is through there was none, it was:

For 5.4.2 ( I can imagine that CascadeAfterSafe the problem is like in #1413):

[bahusoid]

You said you have the same performance issue with 5.3. Did you check with 5.2? Can you share how profiling looks for 5.3

[trivalik]

Wow. I wasn't aware that 5.3.16 has also 7ms.

[bahusoid]

I wasn't aware that 5.3.16 has also 7ms.

Then it's likely #2752 to blame

[trivalik]

I can confirm that the over 1 second run for IsDirty starts with the changes in commit 89a79d2.

[hazzik]

@trivalik if in that commit you change Persist to PersistOnFlush - does it change anything?

Also do you have any test case to reproduce this?

[trivalik]

The change to:

protected override CascadingAction CascadingAction => CascadingAction.PersistOnFlush;

requires more time than just CascadingAction.Persist.

Right now I have only my private test case but no general for your test suite.

I ended up the following, but not sure if this is actually the repro:

I changed the GH1413 code to:

using NHibernate.Cfg.MappingSchema;
using NHibernate.Engine;
using NHibernate.Mapping.ByCode;
using NUnit.Framework;

namespace NHibernate.Test.NHSpecificTest.GH1413
{
	[TestFixture]
	public class ByCodeFixture : TestCaseMappingByCode
	{
		private int _parentId;

		protected override HbmMapping GetMappings()
		{
			var mapper = new ModelMapper();
			mapper.Class<EntityParent>(rc =>
			{
				rc.Id(x => x.Id, m => m.Generator(Generators.Identity));
				rc.Property(x => x.Name);
				rc.Bag(x => x.Children, m =>
				{
					m.Cascade(Mapping.ByCode.Cascade.All);
					m.Inverse(true);
				}, a => a.OneToMany()
				);
			});

			mapper.Class<EntityChild>(rc =>
			{
				rc.Id(x => x.Id, m => m.Generator(Generators.Identity));
				rc.Property(x => x.Name);
			});

			return mapper.CompileMappingForAllExplicitlyAddedEntities();
		}

		protected override void OnSetUp()
		{
			using (var session = OpenSession())
			using (var transaction = session.BeginTransaction())
			{
				var parent = new EntityParent
				{
					Name = "InitialParent",
				};
				session.Save(parent);

				session.Flush();
				transaction.Commit();
				_parentId = parent.Id;
			}
		}

		protected override void OnTearDown()
		{
			using (var session = OpenSession())
			using (var transaction = session.BeginTransaction())
			{
				session.Delete("from System.Object");

				session.Flush();
				transaction.Commit();
			}
		}

		[Theory]
		public void SessionIsDirtyShouldNotTriggerCascadeSaving(bool beginTransaction)
		{
			Sfi.Statistics.IsStatisticsEnabled = true;
			using (var session = OpenSession())
			using (beginTransaction ? session.BeginTransaction() : null)
			{
				var parent = GetParent(session);
				var entityChild = new EntityParent
				{
					Name = "NewListElem",
					Children = //add them an additional child
					{
						new EntityParent
						{
							Name = "ChildA",
						}
					}
				};

				//parent.Children is cascaded
				parent.Children.Add(entityChild);

				Sfi.Statistics.Clear();
				var isDirty = session.IsDirty();

				Assert.That(isDirty, "ISession.IsDirty() call should return true.");
			}
		}

		private EntityParent GetParent(ISession session)
		{
			return session.Get<EntityParent>(_parentId);
		}
	}
}

Therefore the EntityChild just changed to EntityParent.

using System;
using System.Collections.Generic;

namespace NHibernate.Test.NHSpecificTest.GH1413
{
	public class EntityChild
	{
		public virtual int Id { get; set; }
		public virtual string Name { get; set; }
	}

	public class EntityParent
	{
		public virtual int Id { get; set; }
		public virtual string Name { get; set; }
		public virtual IList<EntityParent> Children { get; set; } = new List<EntityParent>();  // replace EnitityChild with EntitiyParent
	}
}

Can you confirm?

[hazzik]

I ended up the following, but not sure if this is actually the repro

@trivalik, I'm not sure either.

Can you please test if changing this code:

nhibernate-core/src/NHibernate/Event/Default/AbstractFlushingEventListener.cs

Lines 187 to 202 in 90d50c0

	protected virtual void PrepareEntityFlushes(IEventSource session)
	{
	log.Debug("processing flush-time cascades");
	ICollection list = IdentityMap.ConcurrentEntries(session.PersistenceContext.EntityEntries);
	//safe from concurrent modification because of how entryList() is implemented on IdentityMap
	foreach (DictionaryEntry me in list)
	{
	EntityEntry entry = (EntityEntry) me.Value;
	Status status = entry.Status;
	if (status == Status.Loaded || status == Status.Saving || status == Status.ReadOnly)
	{
	CascadeOnFlush(session, entry.Persister, me.Key, Anything);
	}
	}
	}

And moving Anything out of the foreach loop fixes anything? Like this:

protected virtual void PrepareEntityFlushes(IEventSource session)
{
	log.Debug("processing flush-time cascades");

	ICollection list = IdentityMap.ConcurrentEntries(session.PersistenceContext.EntityEntries);
	var context = Anything; // <<<< HERE
	//safe from concurrent modification because of how entryList() is implemented on IdentityMap
	foreach (DictionaryEntry me in list)
	{
		EntityEntry entry = (EntityEntry) me.Value;
		Status status = entry.Status;
		if (status == Status.Loaded || status == Status.Saving || status == Status.ReadOnly)
		{
			CascadeOnFlush(session, entry.Persister, me.Key, context /* <<<< HERE */);
		}
	}
}

[hazzik]

@bahusoid my understanding is that DefaultDirtyCheckEventListener is shared, so that should not be done. My suggestion is consistent with how it is done in hibernate:

https://github.com/hibernate/hibernate-orm/blob/156fd51e5c81dc3b5f7fec10bba191468530463e/hibernate-core/src/main/java/org/hibernate/event/internal/AbstractFlushingEventListener.java#L134C1-L147

private void prepareEntityFlushes(EventSource session, PersistenceContext persistenceContext) throws HibernateException {

	LOG.debug( "Processing flush-time cascades" );

	final PersistContext context = getContext(); /// <<< the context is a wrapper around a hashset/identity map
	//safe from concurrent modification because of how concurrentEntries() is implemented on IdentityMap
	for ( Map.Entry<Object,EntityEntry> me : persistenceContext.reentrantSafeEntityEntries() ) {
//		for ( Map.Entry me : IdentityMap.concurrentEntries( persistenceContext.getEntityEntries() ) ) {
		final EntityEntry entry = me.getValue();
		if ( flushable( entry ) ) {
			cascadeOnFlush( session, entry.getPersister(), me.getKey(), context );
		}
	}
}