Skip to content
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

A bunch of minor performance optimizations #16335

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ee59368
Do not execute query if no macros found
bergmania Apr 15, 2024
7b4581b
Request cache the permission lookup
bergmania Apr 17, 2024
5d51d81
Unbreak change by adding obsolete ctor
bergmania Apr 17, 2024
487f480
Clean up
bergmania Apr 18, 2024
e1e584b
Wrap indexing for delivery API in a scope
bergmania Apr 18, 2024
c2d1b7a
Do not ask options every time for the timeout, instead listen for upd…
bergmania Apr 18, 2024
61908b4
Lookup content types once instead of one by one
bergmania Apr 18, 2024
5c16cfb
Use TryGetValue instead
bergmania Apr 18, 2024
07431e0
Do a distinct on user ids before building index, to avoid issue with …
bergmania Apr 23, 2024
97d3e0e
Merge remote-tracking branch 'refs/remotes/origin/release/13.3' into …
bergmania May 10, 2024
f5a264a
Don't map ContentDto (it's unused)
kjac May 16, 2024
e9a4c02
Introduce request bound block editor element cache
kjac May 16, 2024
d357c59
Merge branch 'refs/heads/release/13.3' into v13/hotfix/performance-op…
bergmania May 16, 2024
1c34218
Merge remote-tracking branch 'refs/remotes/origin/v13/hotfix/performa…
bergmania May 16, 2024
e5725eb
Merge branch 'refs/heads/v13/dev' into v13/hotfix/performance-optimiz…
bergmania May 17, 2024
290a8fc
Merge branch 'refs/heads/v13/dev' into v13/hotfix/performance-optimiz…
bergmania May 21, 2024
a011e0f
Merge remote-tracking branch 'refs/remotes/origin/release/13.3' into …
bergmania May 24, 2024
e9617af
Merge remote-tracking branch 'refs/remotes/origin/v13/dev' into v13/h…
bergmania May 29, 2024
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 @@ -16,8 +16,8 @@ namespace Umbraco.Cms.Persistence.EFCore.Locking;
internal class SqlServerEFCoreDistributedLockingMechanism<T> : IDistributedLockingMechanism
where T : DbContext
{
private readonly IOptionsMonitor<ConnectionStrings> _connectionStrings;
private readonly IOptionsMonitor<GlobalSettings> _globalSettings;
private ConnectionStrings _connectionStrings;
private GlobalSettings _globalSettings;
private readonly ILogger<SqlServerEFCoreDistributedLockingMechanism<T>> _logger;
private readonly Lazy<IEFCoreScopeAccessor<T>> _scopeAccessor; // Hooray it's a circular dependency.

Expand All @@ -32,27 +32,29 @@ public SqlServerEFCoreDistributedLockingMechanism(
{
_logger = logger;
_scopeAccessor = scopeAccessor;
_globalSettings = globalSettings;
_connectionStrings = connectionStrings;
_globalSettings = globalSettings.CurrentValue;
_connectionStrings = connectionStrings.CurrentValue;
globalSettings.OnChange(x=>_globalSettings = x);
connectionStrings.OnChange(x=>_connectionStrings = x);
}

public bool HasActiveRelatedScope => _scopeAccessor.Value.AmbientScope is not null;

/// <inheritdoc />
public bool Enabled => _connectionStrings.CurrentValue.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.CurrentValue.ProviderName, "Microsoft.Data.SqlClient", StringComparison.InvariantCultureIgnoreCase) && _scopeAccessor.Value.AmbientScope is not null;
public bool Enabled => _connectionStrings.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.ProviderName, "Microsoft.Data.SqlClient", StringComparison.InvariantCultureIgnoreCase) && _scopeAccessor.Value.AmbientScope is not null;

/// <inheritdoc />
public IDistributedLock ReadLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingReadLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingReadLockDefaultTimeout;
return new SqlServerDistributedLock(this, lockId, DistributedLockType.ReadLock, obtainLockTimeout.Value);
}

/// <inheritdoc />
public IDistributedLock WriteLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingWriteLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingWriteLockDefaultTimeout;
return new SqlServerDistributedLock(this, lockId, DistributedLockType.WriteLock, obtainLockTimeout.Value);
}

Expand Down Expand Up @@ -168,9 +170,7 @@ private void ObtainWriteLock()
"A transaction with minimum ReadCommitted isolation level is required.");
}

await dbContext.Database.ExecuteSqlRawAsync($"SET LOCK_TIMEOUT {(int)_timeout.TotalMilliseconds};");

var rowsAffected = await dbContext.Database.ExecuteSqlAsync(@$"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id={LockId}");
var rowsAffected = await dbContext.Database.ExecuteSqlAsync(@$"SET LOCK_TIMEOUT {(int)_timeout.TotalMilliseconds};UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id={LockId}");

if (rowsAffected == 0)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ namespace Umbraco.Cms.Persistence.EFCore.Locking;
internal class SqliteEFCoreDistributedLockingMechanism<T> : IDistributedLockingMechanism
where T : DbContext
{
private readonly IOptionsMonitor<ConnectionStrings> _connectionStrings;
private readonly IOptionsMonitor<GlobalSettings> _globalSettings;
private ConnectionStrings _connectionStrings;
private GlobalSettings _globalSettings;
private readonly ILogger<SqliteEFCoreDistributedLockingMechanism<T>> _logger;
private readonly Lazy<IEFCoreScopeAccessor<T>> _efCoreScopeAccessor;

Expand All @@ -29,27 +29,29 @@ public SqliteEFCoreDistributedLockingMechanism(
{
_logger = logger;
_efCoreScopeAccessor = efCoreScopeAccessor;
_connectionStrings = connectionStrings;
_globalSettings = globalSettings;
_globalSettings = globalSettings.CurrentValue;
_connectionStrings = connectionStrings.CurrentValue;
globalSettings.OnChange(x=>_globalSettings = x);
connectionStrings.OnChange(x=>_connectionStrings = x);
}

public bool HasActiveRelatedScope => _efCoreScopeAccessor.Value.AmbientScope is not null;

/// <inheritdoc />
public bool Enabled => _connectionStrings.CurrentValue.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.CurrentValue.ProviderName, "Microsoft.Data.Sqlite", StringComparison.InvariantCultureIgnoreCase) && _efCoreScopeAccessor.Value.AmbientScope is not null;
public bool Enabled => _connectionStrings.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.ProviderName, "Microsoft.Data.Sqlite", StringComparison.InvariantCultureIgnoreCase) && _efCoreScopeAccessor.Value.AmbientScope is not null;

// With journal_mode=wal we can always read a snapshot.
public IDistributedLock ReadLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingReadLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingReadLockDefaultTimeout;
return new SqliteDistributedLock(this, lockId, DistributedLockType.ReadLock, obtainLockTimeout.Value);
}

// With journal_mode=wal only a single write transaction can exist at a time.
public IDistributedLock WriteLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingWriteLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingWriteLockDefaultTimeout;
return new SqliteDistributedLock(this, lockId, DistributedLockType.WriteLock, obtainLockTimeout.Value);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ namespace Umbraco.Cms.Persistence.SqlServer.Services;
/// </summary>
public class SqlServerDistributedLockingMechanism : IDistributedLockingMechanism
{
private readonly IOptionsMonitor<ConnectionStrings> _connectionStrings;
private readonly IOptionsMonitor<GlobalSettings> _globalSettings;
private ConnectionStrings _connectionStrings;
private GlobalSettings _globalSettings;
private readonly ILogger<SqlServerDistributedLockingMechanism> _logger;
private readonly Lazy<IScopeAccessor> _scopeAccessor; // Hooray it's a circular dependency.

Expand All @@ -33,25 +33,28 @@ public SqlServerDistributedLockingMechanism(
{
_logger = logger;
_scopeAccessor = scopeAccessor;
_globalSettings = globalSettings;
_connectionStrings = connectionStrings;
_globalSettings = globalSettings.CurrentValue;
_connectionStrings = connectionStrings.CurrentValue;
globalSettings.OnChange(x => _globalSettings = x);
connectionStrings.OnChange(x => _connectionStrings = x);

}

/// <inheritdoc />
public bool Enabled => _connectionStrings.CurrentValue.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.CurrentValue.ProviderName,Constants.ProviderName, StringComparison.InvariantCultureIgnoreCase);
public bool Enabled => _connectionStrings.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.ProviderName,Constants.ProviderName, StringComparison.InvariantCultureIgnoreCase);

/// <inheritdoc />
public IDistributedLock ReadLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingReadLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingReadLockDefaultTimeout;
return new SqlServerDistributedLock(this, lockId, DistributedLockType.ReadLock, obtainLockTimeout.Value);
}

/// <inheritdoc />
public IDistributedLock WriteLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingWriteLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingWriteLockDefaultTimeout;
return new SqlServerDistributedLock(this, lockId, DistributedLockType.WriteLock, obtainLockTimeout.Value);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ namespace Umbraco.Cms.Persistence.Sqlite.Services;

public class SqliteDistributedLockingMechanism : IDistributedLockingMechanism
{
private readonly IOptionsMonitor<ConnectionStrings> _connectionStrings;
private readonly IOptionsMonitor<GlobalSettings> _globalSettings;
private ConnectionStrings _connectionStrings;
private GlobalSettings _globalSettings;
private readonly ILogger<SqliteDistributedLockingMechanism> _logger;
private readonly Lazy<IScopeAccessor> _scopeAccessor;

Expand All @@ -29,25 +29,27 @@ public SqliteDistributedLockingMechanism(
{
_logger = logger;
_scopeAccessor = scopeAccessor;
_connectionStrings = connectionStrings;
_globalSettings = globalSettings;
_connectionStrings = connectionStrings.CurrentValue;
_globalSettings = globalSettings.CurrentValue;
globalSettings.OnChange(x=>_globalSettings = x);
connectionStrings.OnChange(x=>_connectionStrings = x);
}

/// <inheritdoc />
public bool Enabled => _connectionStrings.CurrentValue.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.CurrentValue.ProviderName, Constants.ProviderName, StringComparison.InvariantCultureIgnoreCase);
public bool Enabled => _connectionStrings.IsConnectionStringConfigured() &&
string.Equals(_connectionStrings.ProviderName, Constants.ProviderName, StringComparison.InvariantCultureIgnoreCase);

// With journal_mode=wal we can always read a snapshot.
public IDistributedLock ReadLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingReadLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingReadLockDefaultTimeout;
return new SqliteDistributedLock(this, lockId, DistributedLockType.ReadLock, obtainLockTimeout.Value);
}

// With journal_mode=wal only a single write transaction can exist at a time.
public IDistributedLock WriteLock(int lockId, TimeSpan? obtainLockTimeout = null)
{
obtainLockTimeout ??= _globalSettings.CurrentValue.DistributedLockingWriteLockDefaultTimeout;
obtainLockTimeout ??= _globalSettings.DistributedLockingWriteLockDefaultTimeout;
return new SqliteDistributedLock(this, lockId, DistributedLockType.WriteLock, obtainLockTimeout.Value);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ public ContentItemDisplay()
/// This is not used for outgoing model information.
/// </remarks>
[IgnoreDataMember]
[Obsolete("No longer used. Will be removed in V15.")]
public ContentPropertyCollectionDto? ContentDto { get; set; }

/// <summary>
Expand Down
43 changes: 35 additions & 8 deletions src/Umbraco.Core/Services/UserService.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
using System.Data.Common;

Check notice on line 1 in src/Umbraco.Core/Services/UserService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v13/dev)

✅ Getting better: Primitive Obsession

The ratio of primitive types in function arguments decreases from 72.65% to 68.00%, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.
using System.Globalization;
using System.Linq.Expressions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models.Membership;
using Umbraco.Cms.Core.Notifications;
Expand All @@ -25,8 +28,11 @@
private readonly ILogger<UserService> _logger;
private readonly IRuntimeState _runtimeState;
private readonly IUserGroupRepository _userGroupRepository;
private readonly IRequestCache _requestCache;
private readonly IUserRepository _userRepository;


[Obsolete("Use non-obsolete constructor. This will be removed in Umbraco 15.")]
public UserService(
ICoreScopeProvider provider,
ILoggerFactory loggerFactory,
Expand All @@ -35,11 +41,26 @@
IUserRepository userRepository,
IUserGroupRepository userGroupRepository,
IOptions<GlobalSettings> globalSettings)
: this(provider, loggerFactory, eventMessagesFactory, runtimeState, userRepository, userGroupRepository, globalSettings, StaticServiceProvider.Instance.GetRequiredService<IRequestCache>())
{

}

public UserService(
ICoreScopeProvider provider,
ILoggerFactory loggerFactory,
IEventMessagesFactory eventMessagesFactory,
IRuntimeState runtimeState,
IUserRepository userRepository,
IUserGroupRepository userGroupRepository,
IOptions<GlobalSettings> globalSettings,
IRequestCache requestCache)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we have a requestCache in a entity service.

Think it would be better to do something like the BlockEditorElementTypeCache introduced in this PR to separate the caching behavior from the actual retrieval from persistance.

: base(provider, loggerFactory, eventMessagesFactory)
{
_runtimeState = runtimeState;
_userRepository = userRepository;
_userGroupRepository = userGroupRepository;
_requestCache = requestCache;

Check notice on line 63 in src/Umbraco.Core/Services/UserService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v13/dev)

ℹ New issue: Constructor Over-Injection

UserService has 8 arguments, threshold = 5. This constructor has too many arguments, indicating an object with low cohesion or missing function argument abstraction. Avoid adding more arguments.
_globalSettings = globalSettings.Value;
_logger = loggerFactory.CreateLogger<UserService>();
}
Expand Down Expand Up @@ -1125,17 +1146,23 @@
/// <param name="path">Path to check permissions for</param>
public EntityPermissionSet GetPermissionsForPath(IUser? user, string? path)
{
var nodeIds = path?.GetIdsFromPathReversed();

if (nodeIds is null || nodeIds.Length == 0 || user is null)
var result = (EntityPermissionSet?)_requestCache.Get($"{nameof(GetPermissionsForPath)}|{path}|{user?.Id}", () =>
{
return EntityPermissionSet.Empty();
}
var nodeIds = path?.GetIdsFromPathReversed();

if (nodeIds is null || nodeIds.Length == 0 || user is null)
{
return EntityPermissionSet.Empty();
}

// collect all permissions structures for all nodes for all groups belonging to the user
EntityPermission[] groupPermissions = GetPermissionsForPath(user.Groups.ToArray(), nodeIds, true).ToArray();
// collect all permissions structures for all nodes for all groups belonging to the user
EntityPermission[] groupPermissions = GetPermissionsForPath(user.Groups.ToArray(), nodeIds, true).ToArray();

return CalculatePermissionsForPathForUser(groupPermissions, nodeIds);
});

return result ?? EntityPermissionSet.Empty();

return CalculatePermissionsForPathForUser(groupPermissions, nodeIds);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Services;
using Umbraco.Extensions;

namespace Umbraco.Cms.Core.Cache.PropertyEditors;

internal sealed class BlockEditorElementTypeCache : IBlockEditorElementTypeCache
{
private readonly IContentTypeService _contentTypeService;
private readonly AppCaches _appCaches;

public BlockEditorElementTypeCache(IContentTypeService contentTypeService, AppCaches appCaches)
{
_contentTypeService = contentTypeService;
_appCaches = appCaches;
}

public IEnumerable<IContentType> GetAll(IEnumerable<Guid> keys)
{
// TODO: make this less dumb; don't fetch all elements, only fetch the items that aren't yet in the cache and amend the cache as more elements are loaded

const string cacheKey = $"{nameof(BlockEditorElementTypeCache)}_ElementTypes";
IEnumerable<IContentType>? cachedElements = _appCaches.RequestCache.GetCacheItem<IEnumerable<IContentType>>(cacheKey);
if (cachedElements is null)
{
cachedElements = _contentTypeService.GetAllElementTypes();
_appCaches.RequestCache.Set(cacheKey, cachedElements);
}

return cachedElements.Where(elementType => keys.Contains(elementType.Key));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using Umbraco.Cms.Core.Models;

namespace Umbraco.Cms.Core.Cache.PropertyEditors;

public interface IBlockEditorElementTypeCache
{
IEnumerable<IContentType> GetAll(IEnumerable<Guid> keys);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Serilog;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Cache.PropertyEditors;
using Umbraco.Cms.Core.Configuration;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.DeliveryApi;
Expand Down Expand Up @@ -235,6 +236,8 @@
builder.AddDeliveryApiCoreServices();
builder.Services.AddTransient<IWebhookFiringService, WebhookFiringService>();

builder.Services.AddSingleton<IBlockEditorElementTypeCache, BlockEditorElementTypeCache>();

Check warning on line 240 in src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v13/dev)

❌ Getting worse: Large Method

AddCoreInitialServices increases from 105 to 106 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
return builder;
}

Expand Down
4 changes: 2 additions & 2 deletions src/Umbraco.Infrastructure/Examine/ContentValueSetBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,9 @@ public override IEnumerable<ValueSet> GetValueSets(params IContent[] content)
// processing below instead of one by one.
using (ICoreScope scope = _scopeProvider.CreateCoreScope())
{
creatorIds = _userService.GetProfilesById(content.Select(x => x.CreatorId).ToArray())
creatorIds = _userService.GetProfilesById(content.Select(x => x.CreatorId).Distinct().ToArray())
.ToDictionary(x => x.Id, x => x);
writerIds = _userService.GetProfilesById(content.Select(x => x.WriterId).ToArray())
writerIds = _userService.GetProfilesById(content.Select(x => x.WriterId).Distinct().ToArray())
.ToDictionary(x => x.Id, x => x);
scope.Complete();
}
Expand Down
Loading
Loading