Merge pull request #344 from servicetitan/upstream/OpenSystemLogicOnlyRegion

Optimize IDisposable implementations

Author: alex-kulakov

Orm/Xtensive.Orm.Tests/Storage/Prefetch/PrefetchManagerBasicTest.cs

@@ -7,6 +7,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Runtime.CompilerServices;
 using System.Threading;
 using NUnit.Framework;
 using Xtensive.Core;
@@ -27,16 +28,14 @@ namespace Xtensive.Orm.Tests.Storage.Prefetch
   [TestFixture]
   public class PrefetchManagerBasicTest : PrefetchManagerTestBase
   {
+    private const int Iterations = 10;
     private volatile static int instanceCount;
 
     #region Nested class
 
     public class MemoryLeakTester
     {
-      ~MemoryLeakTester()
-      {
-        instanceCount--;
-      }
+      ~MemoryLeakTester() => Interlocked.Decrement(ref instanceCount);
     }
 
     #endregion
@@ -932,17 +931,18 @@ public void RemoveTest()
     [Test]
     public void ReferenceToSessionIsNotPreservedInCacheTest()
     {
-      // Use separate method for session related processing
+      // Use separate method with [MethodImpl(MethodImplOptions.NoInlining)] attribute for session related processing
       // to make sure we don't hold session reference somewhere on stack
       OpenSessionsAndRunPrefetches();
       TestHelper.CollectGarbage(true);
       Assert.That(instanceCount, Is.EqualTo(0));
     }
 
+    [MethodImpl(MethodImplOptions.NoInlining)]
     private void OpenSessionsAndRunPrefetches()
     {
-      instanceCount = 10;
-      for (int i = 0; i < instanceCount; i++) {
+      instanceCount = Iterations;
+      for (int i = 0; i < Iterations; i++) {
         using (var session = Domain.OpenSession())
         using (var t = session.OpenTransaction()) {
           session.Extensions.Set(new MemoryLeakTester());

Orm/Xtensive.Orm/Collections/BindingCollection.cs

@@ -112,9 +112,7 @@ public virtual BindingScope Add(TKey key, TValue value)
     public virtual void PermanentAdd(TKey key, TValue value)
     {
       bindings[key] = value;
-      if (!permanentBindings.Contains(key)) {
-        permanentBindings.Add(key);
-      }
+      permanentBindings.Add(key);
     }
 
     /// <summary>

Orm/Xtensive.Orm/Collections/Graphs/Graph.cs

@@ -56,9 +56,8 @@ public Graph<Node<TNode>, Edge<TEdge>> CreateMutableCopy()
       foreach (var rNode in copy.Nodes) {
         var node = rNode.Value;
         foreach (var edge in node.Edges) {
-          if (!processedEdges.Contains(edge)) {
+          if (processedEdges.Add(edge)) {
             var rEdge = new Edge<TEdge>(nodeMap[edge.Source], nodeMap[edge.Target], (TEdge) edge);
-            processedEdges.Add(edge);
           }
         }
       }

Orm/Xtensive.Orm/Orm/Entity.cs

@@ -138,8 +138,7 @@ public VersionInfo VersionInfo {
         List<PrefetchFieldDescriptor> columnsToPrefetch = null;
         foreach (var columnInfo in versionColumns) {
           if (!tuple.GetFieldState(columnInfo.Field.MappingInfo.Offset).IsAvailable()) {
-            if (columnsToPrefetch==null)
-              columnsToPrefetch = new List<PrefetchFieldDescriptor>();
+            columnsToPrefetch ??= new List<PrefetchFieldDescriptor>(1);
             columnsToPrefetch.Add(new PrefetchFieldDescriptor(columnInfo.Field));
           }
         }

Orm/Xtensive.Orm/Orm/Internals/Pinner.cs

@@ -14,6 +14,14 @@ namespace Xtensive.Orm.Internals
 {
   internal sealed class Pinner : SessionBound
   {
+    private class DisposableRemover : IDisposable
+    {
+      public Pinner Pinner { get; init; }
+      public EntityState State { get; init; }
+
+      public void Dispose() => Pinner.roots.Remove(State);
+    }
+
     private readonly HashSet<EntityState> roots = new HashSet<EntityState>();
 
     private EntityChangeRegistry activeRegistry;
@@ -24,15 +32,8 @@ internal sealed class Pinner : SessionBound
     public EntityChangeRegistry PinnedItems { get; private set; }
     public EntityChangeRegistry PersistableItems { get; private set; }
 
-    public IDisposable RegisterRoot(EntityState state)
-    {
-      if (roots.Contains(state))
-        return null;
-      roots.Add(state);
-      return new Disposable<Pinner, EntityState>(
-        this, state,
-        (disposing, _this, _state) => _this.roots.Remove(_state));
-    }
+    public IDisposable RegisterRoot(EntityState state) =>
+      roots.Add(state) ? new DisposableRemover { Pinner = this, State = state } : null;
 
     public void ClearRoots()
     {

Orm/Xtensive.Orm/Orm/Model/FullTextIndexInfoCollection.cs

@@ -54,8 +54,7 @@ public bool TryGetValue(TypeInfo typeInfo, out FullTextIndexInfo fullTextIndexIn
     public void Add(TypeInfo typeInfo, FullTextIndexInfo fullTextIndexInfo)
     {
       EnsureNotLocked();
-      if (!container.Contains(fullTextIndexInfo))
-        container.Add(fullTextIndexInfo);
+      container.Add(fullTextIndexInfo);
       indexMap.Add(typeInfo, fullTextIndexInfo);
     }
 

Orm/Xtensive.Orm/Orm/OperationLog.cs

@@ -61,7 +61,7 @@ public KeyMapping Replay(Session session)
       KeyMapping keyMapping;
 
       using (session.Activate()) {
-        using (isSystemOperationLog ? session.OpenSystemLogicOnlyRegion() : null) 
+        using (isSystemOperationLog ? (IDisposable) session.OpenSystemLogicOnlyRegion() : null)
         using (var tx = session.OpenTransaction(TransactionOpenMode.New)) {
 
           foreach (var operation in operations)

Orm/Xtensive.Orm/Orm/Operations/OperationRegistry.cs

@@ -16,10 +16,24 @@ namespace Xtensive.Orm.Operations
   /// </summary>
   public sealed class OperationRegistry
   {
+    public readonly struct SystemOperationRegistrationScope : IDisposable
+    {
+      private readonly OperationRegistry registry;
+      private readonly bool prevIsSystemOperationRegistrationEnabled;
+
+      public SystemOperationRegistrationScope(OperationRegistry registry, bool enable)
+      {
+        this.registry = registry;
+        prevIsSystemOperationRegistrationEnabled = registry.IsSystemOperationRegistrationEnabled;
+        registry.IsSystemOperationRegistrationEnabled = enable;
+      }
+
+      public void Dispose() => registry.IsSystemOperationRegistrationEnabled = prevIsSystemOperationRegistrationEnabled;
+    }
+
     private readonly ICompletableScope blockingScope;
     private bool isOperationRegistrationEnabled = true;
     private bool isUndoOperationRegistrationEnabled = true;
-    private bool isSystemOperationRegistrationEnabled = true;
     private Collections.Deque<ICompletableScope> scopes = new Collections.Deque<ICompletableScope>();
 
     /// <summary>
@@ -41,10 +55,7 @@ public bool IsRegistrationEnabled {
     /// <summary>
     /// Gets a value indicating whether system operation registration is enabled.
     /// </summary>
-    public bool IsSystemOperationRegistrationEnabled {
-      get { return isSystemOperationRegistrationEnabled; }
-      internal set { isSystemOperationRegistrationEnabled = value; }
-    }
+    public bool IsSystemOperationRegistrationEnabled { get; internal set; } = true;
 
     /// <summary>
     /// Gets a value indicating whether this instance can register operation
@@ -219,29 +230,13 @@ public IDisposable DisableUndoOperationRegistration()
     /// Temporarily disables system operation logging.
     /// </summary>
     /// <returns>An <see cref="IDisposable"/> object enabling the logging back on its disposal.</returns>
-    public IDisposable DisableSystemOperationRegistration()
-    {
-      if (!isSystemOperationRegistrationEnabled)
-        return null;
-      var result = new Disposable<OperationRegistry, bool>(this, isSystemOperationRegistrationEnabled,
-        (disposing, _this, previousState) => _this.isSystemOperationRegistrationEnabled = previousState);
-      isSystemOperationRegistrationEnabled = false;
-      return result;
-    }
+    public SystemOperationRegistrationScope DisableSystemOperationRegistration() => new(this, false);
 
     /// <summary>
     /// Temporarily enables system operation logging.
     /// </summary>
     /// <returns>An <see cref="IDisposable"/> object disabling the logging back on its disposal.</returns>
-    public IDisposable EnableSystemOperationRegistration()
-    {
-      if (isSystemOperationRegistrationEnabled)
-        return null;
-      var result = new Disposable<OperationRegistry, bool>(this, isSystemOperationRegistrationEnabled,
-        (disposing, _this, previousState) => _this.isSystemOperationRegistrationEnabled = previousState);
-      isSystemOperationRegistrationEnabled = true;
-      return result;
-    }
+    public SystemOperationRegistrationScope EnableSystemOperationRegistration() => new(this, true);
 
     /// <summary>
     /// Registers the operation.

Orm/Xtensive.Orm/Orm/Services/DirectSessionAccessor.cs

@@ -28,10 +28,7 @@ public sealed class DirectSessionAccessor : SessionBound,
     /// disposal will restore previous state of
     /// <see cref="Session.IsSystemLogicOnly"/> property.
     /// </returns>
-    public IDisposable OpenSystemLogicOnlyRegion()
-    {
-      return Session.OpenSystemLogicOnlyRegion();
-    }
+    public Session.SystemLogicOnlyRegionScope OpenSystemLogicOnlyRegion() => Session.OpenSystemLogicOnlyRegion();
 
     /// <summary>
     /// Changes the value of <see cref="Session.Handler"/>.

Orm/Xtensive.Orm/Orm/Session.Persist.cs

@@ -224,7 +224,7 @@ private async ValueTask Persist(PersistReason reason, bool isAsync, Cancellation
     }
 
     /// <summary>
-    /// Temporarily disables all save changes operations (both explicit ant automatic) 
+    /// Temporarily disables all save changes operations (both explicit ant automatic)
     /// for specified <paramref name="target"/>.
     /// Such entity is prevented from being persisted to the database,
     /// when <see cref="SaveChanges"/> is called or query is executed.
@@ -234,16 +234,20 @@ private async ValueTask Persist(PersistReason reason, bool isAsync, Cancellation
     /// all entities that reference <paramref name="target"/> are also pinned automatically.
     /// </summary>
     /// <param name="target">The entity to disable persisting.</param>
-    /// <returns>A special object that controls lifetime of such behavior if <paramref name="target"/> was not previously processed by the method,
-    /// otherwise <see langword="null"/>.</returns>
+    /// <returns>
+    /// A special object that controls lifetime of such behavior if <paramref name="target"/> was not previously processed by the method
+    /// and automatic saving of changes is enabled (<see cref="SessionOptions.AutoSaveChanges"/>),
+    /// otherwise <see langword="null"/>.
+    /// </returns>
     public IDisposable DisableSaveChanges(IEntity target)
     {
       EnsureNotDisposed();
       ArgumentValidator.EnsureArgumentNotNull(target, "target");
+      if (!Configuration.Supports(SessionOptions.AutoSaveChanges))
+        return null; // No need to pin in this case
+
       var targetEntity = (Entity) target;
       targetEntity.EnsureNotRemoved();
-      if (!Configuration.Supports(SessionOptions.AutoSaveChanges))
-        return new Disposable(b => {return;}); // No need to pin in this case
       return pinner.RegisterRoot(targetEntity.State);
     }
 
@@ -252,15 +256,15 @@ public IDisposable DisableSaveChanges(IEntity target)
     /// Explicit call of <see cref="SaveChanges"/> will lead to flush changes anyway.
     /// If save changes is to be performed due to starting a nested transaction or committing a transaction,
     /// active disabling save changes scope will lead to failure.
-    /// <returns>A special object that controls lifetime of such behavior if there is no active scope,
+    /// <returns>A special object that controls lifetime of such behavior if there is no active scope
+    /// and automatic saving of changes is enabled (<see cref="SessionOptions.AutoSaveChanges"/>),
     /// otherwise <see langword="null"/>.</returns>
     /// </summary>
     public IDisposable DisableSaveChanges()
     {
-      if (!Configuration.Supports(SessionOptions.AutoSaveChanges))
-        return new Disposable(b => { return; }); // No need to pin in this case
-      if (disableAutoSaveChanges)
-        return null;
+      if (!Configuration.Supports(SessionOptions.AutoSaveChanges) || disableAutoSaveChanges) {
+        return null; // No need to pin in these cases
+      }
 
       disableAutoSaveChanges = true;
       return new Disposable(_ => {
@@ -317,7 +321,7 @@ private void CancelEntitiesChanges()
         newEntity.Update(null);
         newEntity.PersistenceState = PersistenceState.Removed;
       }
-      
+
       foreach (var modifiedEntity in EntityChangeRegistry.GetItems(PersistenceState.Modified)) {
         modifiedEntity.RollbackDifference();
         modifiedEntity.PersistenceState = PersistenceState.Synchronized;

Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs

@@ -11,17 +11,26 @@ namespace Xtensive.Orm
 {
   public partial class Session
   {
+    public readonly struct SystemLogicOnlyRegionScope : IDisposable
+    {
+      private readonly Session session;
+      private readonly bool prevIsSystemLogicOnly;
+
+      internal SystemLogicOnlyRegionScope(Session session)
+      {
+        this.session = session;
+        prevIsSystemLogicOnly = session.IsSystemLogicOnly;
+        session.IsSystemLogicOnly = true;
+      }
+
+      public void Dispose() => session.IsSystemLogicOnly = prevIsSystemLogicOnly;
+    }
+
     /// <summary>
     /// Gets a value indicating whether only a system logic is enabled.
     /// </summary>
     internal bool IsSystemLogicOnly { get; set; }
 
-    internal IDisposable OpenSystemLogicOnlyRegion()
-    {
-      var result = new Disposable<Session, bool>(this, IsSystemLogicOnly,
-        (disposing, session, previousState) => session.IsSystemLogicOnly = previousState);
-      IsSystemLogicOnly = true;
-      return result;
-    }
+    internal SystemLogicOnlyRegionScope OpenSystemLogicOnlyRegion() => new(this);
   }
 }