Skip to content

Optimize IDisposable implementations #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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());
Expand Down
4 changes: 1 addition & 3 deletions Orm/Xtensive.Orm/Collections/BindingCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 1 addition & 2 deletions Orm/Xtensive.Orm/Collections/Graphs/Graph.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions Orm/Xtensive.Orm/Orm/Entity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Expand Down
19 changes: 10 additions & 9 deletions Orm/Xtensive.Orm/Orm/Internals/Pinner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()
{
Expand Down
3 changes: 1 addition & 2 deletions Orm/Xtensive.Orm/Orm/Model/FullTextIndexInfoCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion Orm/Xtensive.Orm/Orm/OperationLog.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
41 changes: 18 additions & 23 deletions Orm/Xtensive.Orm/Orm/Operations/OperationRegistry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 1 addition & 4 deletions Orm/Xtensive.Orm/Orm/Services/DirectSessionAccessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>.
Expand Down
13 changes: 8 additions & 5 deletions Orm/Xtensive.Orm/Orm/Session.Persist.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -245,9 +247,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
}

/// <summary>
Expand All @@ -260,8 +262,9 @@ public IDisposable DisableSaveChanges(IEntity target)
/// </summary>
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;

Expand Down
23 changes: 16 additions & 7 deletions Orm/Xtensive.Orm/Orm/Session.SystemLogic.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/// <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);
}
}