From aec2333dd4501e17f7b7574ba2b7020e2922e863 Mon Sep 17 00:00:00 2001 From: Sergei Pavlov Date: Thu, 19 Oct 2023 19:52:35 -0700 Subject: [PATCH 1/4] Optimize IDisposable implementations (#153) * Optimize IDisposable implementations * optimize new List<>(1) * Optimize EnableSystemOperationRegistration() * Fix Race condition in tests * Fix Race condition in tests * Add [MethodImpl(MethodImplOptions.NoInlining)] --- .../Prefetch/PrefetchManagerBasicTest.cs | 14 +++---- .../Collections/BindingCollection.cs | 4 +- Orm/Xtensive.Orm/Collections/Graphs/Graph.cs | 3 +- Orm/Xtensive.Orm/Orm/Entity.cs | 3 +- Orm/Xtensive.Orm/Orm/Internals/Pinner.cs | 19 +++++---- .../Orm/Model/FullTextIndexInfoCollection.cs | 3 +- Orm/Xtensive.Orm/Orm/OperationLog.cs | 2 +- .../Orm/Operations/OperationRegistry.cs | 41 ++++++++----------- .../Orm/Services/DirectSessionAccessor.cs | 5 +-- Orm/Xtensive.Orm/Orm/Session.Persist.cs | 13 +++--- Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs | 23 +++++++---- 11 files changed, 65 insertions(+), 65 deletions(-) diff --git a/Orm/Xtensive.Orm.Tests/Storage/Prefetch/PrefetchManagerBasicTest.cs b/Orm/Xtensive.Orm.Tests/Storage/Prefetch/PrefetchManagerBasicTest.cs index d03bdda37e..ca375845f8 100644 --- a/Orm/Xtensive.Orm.Tests/Storage/Prefetch/PrefetchManagerBasicTest.cs +++ b/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()); diff --git a/Orm/Xtensive.Orm/Collections/BindingCollection.cs b/Orm/Xtensive.Orm/Collections/BindingCollection.cs index 0d8efef136..de594dbe0e 100644 --- a/Orm/Xtensive.Orm/Collections/BindingCollection.cs +++ b/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); } /// diff --git a/Orm/Xtensive.Orm/Collections/Graphs/Graph.cs b/Orm/Xtensive.Orm/Collections/Graphs/Graph.cs index 97799678e3..4d476f797e 100644 --- a/Orm/Xtensive.Orm/Collections/Graphs/Graph.cs +++ b/Orm/Xtensive.Orm/Collections/Graphs/Graph.cs @@ -56,9 +56,8 @@ public Graph, Edge> 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(nodeMap[edge.Source], nodeMap[edge.Target], (TEdge) edge); - processedEdges.Add(edge); } } } diff --git a/Orm/Xtensive.Orm/Orm/Entity.cs b/Orm/Xtensive.Orm/Orm/Entity.cs index acfb714124..b9d5103351 100644 --- a/Orm/Xtensive.Orm/Orm/Entity.cs +++ b/Orm/Xtensive.Orm/Orm/Entity.cs @@ -138,8 +138,7 @@ public VersionInfo VersionInfo { List columnsToPrefetch = null; foreach (var columnInfo in versionColumns) { if (!tuple.GetFieldState(columnInfo.Field.MappingInfo.Offset).IsAvailable()) { - if (columnsToPrefetch==null) - columnsToPrefetch = new List(); + columnsToPrefetch ??= new List(1); columnsToPrefetch.Add(new PrefetchFieldDescriptor(columnInfo.Field)); } } diff --git a/Orm/Xtensive.Orm/Orm/Internals/Pinner.cs b/Orm/Xtensive.Orm/Orm/Internals/Pinner.cs index b18c2bc902..fc73f3f9fb 100644 --- a/Orm/Xtensive.Orm/Orm/Internals/Pinner.cs +++ b/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 roots = new HashSet(); 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( - 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() { diff --git a/Orm/Xtensive.Orm/Orm/Model/FullTextIndexInfoCollection.cs b/Orm/Xtensive.Orm/Orm/Model/FullTextIndexInfoCollection.cs index 97af6944e8..c2c3894938 100644 --- a/Orm/Xtensive.Orm/Orm/Model/FullTextIndexInfoCollection.cs +++ b/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); } diff --git a/Orm/Xtensive.Orm/Orm/OperationLog.cs b/Orm/Xtensive.Orm/Orm/OperationLog.cs index eea570e86b..333a83a430 100644 --- a/Orm/Xtensive.Orm/Orm/OperationLog.cs +++ b/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) diff --git a/Orm/Xtensive.Orm/Orm/Operations/OperationRegistry.cs b/Orm/Xtensive.Orm/Orm/Operations/OperationRegistry.cs index dfd867091a..61fb2b8b88 100644 --- a/Orm/Xtensive.Orm/Orm/Operations/OperationRegistry.cs +++ b/Orm/Xtensive.Orm/Orm/Operations/OperationRegistry.cs @@ -16,10 +16,24 @@ namespace Xtensive.Orm.Operations /// 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 scopes = new Collections.Deque(); /// @@ -41,10 +55,7 @@ public bool IsRegistrationEnabled { /// /// Gets a value indicating whether system operation registration is enabled. /// - public bool IsSystemOperationRegistrationEnabled { - get { return isSystemOperationRegistrationEnabled; } - internal set { isSystemOperationRegistrationEnabled = value; } - } + public bool IsSystemOperationRegistrationEnabled { get; internal set; } = true; /// /// Gets a value indicating whether this instance can register operation @@ -219,29 +230,13 @@ public IDisposable DisableUndoOperationRegistration() /// Temporarily disables system operation logging. /// /// An object enabling the logging back on its disposal. - public IDisposable DisableSystemOperationRegistration() - { - if (!isSystemOperationRegistrationEnabled) - return null; - var result = new Disposable(this, isSystemOperationRegistrationEnabled, - (disposing, _this, previousState) => _this.isSystemOperationRegistrationEnabled = previousState); - isSystemOperationRegistrationEnabled = false; - return result; - } + public SystemOperationRegistrationScope DisableSystemOperationRegistration() => new(this, false); /// /// Temporarily enables system operation logging. /// /// An object disabling the logging back on its disposal. - public IDisposable EnableSystemOperationRegistration() - { - if (isSystemOperationRegistrationEnabled) - return null; - var result = new Disposable(this, isSystemOperationRegistrationEnabled, - (disposing, _this, previousState) => _this.isSystemOperationRegistrationEnabled = previousState); - isSystemOperationRegistrationEnabled = true; - return result; - } + public SystemOperationRegistrationScope EnableSystemOperationRegistration() => new(this, true); /// /// Registers the operation. diff --git a/Orm/Xtensive.Orm/Orm/Services/DirectSessionAccessor.cs b/Orm/Xtensive.Orm/Orm/Services/DirectSessionAccessor.cs index 93fa845709..0d5e115273 100644 --- a/Orm/Xtensive.Orm/Orm/Services/DirectSessionAccessor.cs +++ b/Orm/Xtensive.Orm/Orm/Services/DirectSessionAccessor.cs @@ -28,10 +28,7 @@ public sealed class DirectSessionAccessor : SessionBound, /// disposal will restore previous state of /// property. /// - public IDisposable OpenSystemLogicOnlyRegion() - { - return Session.OpenSystemLogicOnlyRegion(); - } + public Session.SystemLogicOnlyRegionScope OpenSystemLogicOnlyRegion() => Session.OpenSystemLogicOnlyRegion(); /// /// Changes the value of . diff --git a/Orm/Xtensive.Orm/Orm/Session.Persist.cs b/Orm/Xtensive.Orm/Orm/Session.Persist.cs index 1e9fb3c2d5..fae2c14d11 100644 --- a/Orm/Xtensive.Orm/Orm/Session.Persist.cs +++ b/Orm/Xtensive.Orm/Orm/Session.Persist.cs @@ -18,6 +18,8 @@ namespace Xtensive.Orm { public partial class Session { + private static readonly IDisposable EmptyDisposable = new Disposable(b => { return; }); + private bool disableAutoSaveChanges; private KeyRemapper remapper; private bool persistingIsFailed; @@ -242,9 +244,9 @@ public IDisposable DisableSaveChanges(IEntity target) ArgumentValidator.EnsureArgumentNotNull(target, "target"); 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); + return Configuration.Supports(SessionOptions.AutoSaveChanges) + ? pinner.RegisterRoot(targetEntity.State) + : EmptyDisposable; // No need to pin in this case } /// @@ -257,8 +259,9 @@ public IDisposable DisableSaveChanges(IEntity target) /// public IDisposable DisableSaveChanges() { - if (!Configuration.Supports(SessionOptions.AutoSaveChanges)) - return new Disposable(b => { return; }); // No need to pin in this case + if (!Configuration.Supports(SessionOptions.AutoSaveChanges)) { + return EmptyDisposable; // No need to pin in this case + } if (disableAutoSaveChanges) return null; diff --git a/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs b/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs index c14a77535f..5b31a1b9da 100644 --- a/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs +++ b/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; + + public SystemLogicOnlyRegionScope(Session session) + { + this.session = session; + prevIsSystemLogicOnly = session.IsSystemLogicOnly; + session.IsSystemLogicOnly = true; + } + + public void Dispose() => session.IsSystemLogicOnly = prevIsSystemLogicOnly; + } + /// /// Gets a value indicating whether only a system logic is enabled. /// internal bool IsSystemLogicOnly { get; set; } - internal IDisposable OpenSystemLogicOnlyRegion() - { - var result = new Disposable(this, IsSystemLogicOnly, - (disposing, session, previousState) => session.IsSystemLogicOnly = previousState); - IsSystemLogicOnly = true; - return result; - } + internal SystemLogicOnlyRegionScope OpenSystemLogicOnlyRegion() => new(this); } } \ No newline at end of file From 63e3b5278c6254b8f788c46568a5af080176ee04 Mon Sep 17 00:00:00 2001 From: Sergei Pavlov Date: Thu, 21 Dec 2023 20:52:10 -0800 Subject: [PATCH 2/4] Return `null` instead of EmptyDisposable --- Orm/Xtensive.Orm/Orm/Session.Persist.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Orm/Xtensive.Orm/Orm/Session.Persist.cs b/Orm/Xtensive.Orm/Orm/Session.Persist.cs index fae2c14d11..8b1283d6a5 100644 --- a/Orm/Xtensive.Orm/Orm/Session.Persist.cs +++ b/Orm/Xtensive.Orm/Orm/Session.Persist.cs @@ -18,8 +18,6 @@ namespace Xtensive.Orm { public partial class Session { - private static readonly IDisposable EmptyDisposable = new Disposable(b => { return; }); - private bool disableAutoSaveChanges; private KeyRemapper remapper; private bool persistingIsFailed; @@ -246,7 +244,7 @@ public IDisposable DisableSaveChanges(IEntity target) targetEntity.EnsureNotRemoved(); return Configuration.Supports(SessionOptions.AutoSaveChanges) ? pinner.RegisterRoot(targetEntity.State) - : EmptyDisposable; // No need to pin in this case + : null; // No need to pin in this case } /// @@ -260,7 +258,7 @@ public IDisposable DisableSaveChanges(IEntity target) public IDisposable DisableSaveChanges() { if (!Configuration.Supports(SessionOptions.AutoSaveChanges)) { - return EmptyDisposable; // No need to pin in this case + return null; // No need to pin in this case } if (disableAutoSaveChanges) return null; From 354043aad26a3c01686e9386f0004cd3324182ed Mon Sep 17 00:00:00 2001 From: Sergei Pavlov Date: Fri, 22 Dec 2023 17:23:14 -0800 Subject: [PATCH 3/4] Update summary --- Orm/Xtensive.Orm/Orm/Session.Persist.cs | 27 ++++++++++++++----------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/Orm/Xtensive.Orm/Orm/Session.Persist.cs b/Orm/Xtensive.Orm/Orm/Session.Persist.cs index 8b1283d6a5..a8990861f4 100644 --- a/Orm/Xtensive.Orm/Orm/Session.Persist.cs +++ b/Orm/Xtensive.Orm/Orm/Session.Persist.cs @@ -224,7 +224,7 @@ private async ValueTask Persist(PersistReason reason, bool isAsync, Cancellation } /// - /// Temporarily disables all save changes operations (both explicit ant automatic) + /// Temporarily disables all save changes operations (both explicit ant automatic) /// for specified . /// Such entity is prevented from being persisted to the database, /// when is called or query is executed. @@ -234,17 +234,21 @@ private async ValueTask Persist(PersistReason reason, bool isAsync, Cancellation /// all entities that reference are also pinned automatically. /// /// The entity to disable persisting. - /// A special object that controls lifetime of such behavior if was not previously processed by the method, - /// otherwise . + /// + /// A special object that controls lifetime of such behavior if was not previously processed by the method + /// and automatic saving of changes is enabled (), + /// otherwise . + /// 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(); - return Configuration.Supports(SessionOptions.AutoSaveChanges) - ? pinner.RegisterRoot(targetEntity.State) - : null; // No need to pin in this case + return pinner.RegisterRoot(targetEntity.State); } /// @@ -252,16 +256,15 @@ public IDisposable DisableSaveChanges(IEntity target) /// Explicit call of 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. - /// A special object that controls lifetime of such behavior if there is no active scope, + /// A special object that controls lifetime of such behavior if there is no active scope + /// and automatic saving of changes is enabled (), /// otherwise . /// public IDisposable DisableSaveChanges() { - if (!Configuration.Supports(SessionOptions.AutoSaveChanges)) { - return null; // No need to pin in this case + if (!Configuration.Supports(SessionOptions.AutoSaveChanges) || disableAutoSaveChanges) { + return null; // No need to pin in these cases } - if (disableAutoSaveChanges) - return null; disableAutoSaveChanges = true; return new Disposable(_ => { @@ -318,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; From 2b4d0d434521ae25b55a59034e37f0da21e080d1 Mon Sep 17 00:00:00 2001 From: Sergei Pavlov Date: Mon, 25 Dec 2023 23:47:35 -0800 Subject: [PATCH 4/4] Make SystemLogicOnlyRegionScope() ctor internal --- Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs b/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs index 5b31a1b9da..a33e69beba 100644 --- a/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs +++ b/Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs @@ -16,7 +16,7 @@ public partial class Session private readonly Session session; private readonly bool prevIsSystemLogicOnly; - public SystemLogicOnlyRegionScope(Session session) + internal SystemLogicOnlyRegionScope(Session session) { this.session = session; prevIsSystemLogicOnly = session.IsSystemLogicOnly;