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

Cache block item constructors for block based editors #15264

Merged
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 @@ -162,6 +162,9 @@

builder.Services.AddSingleton<RichTextEditorPastedImages>();
builder.Services.AddSingleton<BlockEditorConverter>();
builder.Services.AddSingleton<BlockListPropertyValueConstructorCache>();
builder.Services.AddSingleton<BlockGridPropertyValueConstructorCache>();
builder.Services.AddSingleton<RichTextBlockPropertyValueConstructorCache>();

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/13.0)

❌ Getting worse: Large Method

AddCoreInitialServices increases from 102 to 105 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.

// both TinyMceValueConverter (in Core) and RteMacroRenderingValueConverter (in Web) will be
// discovered when CoreBootManager configures the converters. We will remove the basic one defined
Expand Down Expand Up @@ -367,7 +370,9 @@
.AddNotificationHandler<ContentDeletedNotification, ImageCropperPropertyEditor>()
.AddNotificationHandler<MediaDeletedNotification, ImageCropperPropertyEditor>()
.AddNotificationHandler<MediaSavingNotification, ImageCropperPropertyEditor>()
.AddNotificationHandler<MemberDeletedNotification, ImageCropperPropertyEditor>();
.AddNotificationHandler<MemberDeletedNotification, ImageCropperPropertyEditor>()
.AddNotificationHandler<ContentTypeCacheRefresherNotification, ConstructorCacheClearNotificationHandler>()
.AddNotificationHandler<DataTypeCacheRefresherNotification, ConstructorCacheClearNotificationHandler>();

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

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/13.0)

❌ Getting worse: Large Method

AddCoreNotifications increases from 86 to 88 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.

// add notification handlers for redirect tracking
builder
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using System.Diagnostics.CodeAnalysis;
using Umbraco.Cms.Core.Models.Blocks;
using Umbraco.Cms.Core.Models.PublishedContent;

namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

public abstract class BlockEditorPropertyValueConstructorCacheBase<T>
where T : IBlockReference<IPublishedElement, IPublishedElement>
{
private readonly
Dictionary<(Guid, Guid?), Func<Udi, IPublishedElement, Udi?, IPublishedElement?, T>>
_constructorCache = new();

public bool TryGetValue((Guid ContentTypeKey, Guid? SettingsTypeKey) key, [MaybeNullWhen(false)] out Func<Udi, IPublishedElement, Udi?, IPublishedElement?, T> value)
=> _constructorCache.TryGetValue(key, out value);

public void SetValue((Guid ContentTypeKey, Guid? SettingsTypeKey) key, Func<Udi, IPublishedElement, Udi?, IPublishedElement?, T> value)
=> _constructorCache[key] = value;

public void Clear()
=> _constructorCache.Clear();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
using Umbraco.Cms.Core.Models.Blocks;

namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

public class BlockGridPropertyValueConstructorCache : BlockEditorPropertyValueConstructorCacheBase<BlockGridItem>
{
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ public class BlockGridPropertyValueConverter : PropertyValueConverterBase, IDeli
private readonly BlockEditorConverter _blockConverter;
private readonly IJsonSerializer _jsonSerializer;
private readonly IApiElementBuilder _apiElementBuilder;
private readonly BlockGridPropertyValueConstructorCache _constructorCache;

[Obsolete("Please use non-obsolete cconstrutor. This will be removed in Umbraco 14.")]
[Obsolete("Please use non-obsolete construtor. This will be removed in Umbraco 14.")]
public BlockGridPropertyValueConverter(
IProfilingLogger proflog,
BlockEditorConverter blockConverter,
Expand All @@ -32,16 +33,28 @@ public BlockGridPropertyValueConverter(

}

[Obsolete("Please use non-obsolete construtor. This will be removed in Umbraco 15.")]
public BlockGridPropertyValueConverter(
IProfilingLogger proflog,
BlockEditorConverter blockConverter,
IJsonSerializer jsonSerializer,
IApiElementBuilder apiElementBuilder)
: this(proflog, blockConverter, jsonSerializer, apiElementBuilder, StaticServiceProvider.Instance.GetRequiredService<BlockGridPropertyValueConstructorCache>())
{
}

public BlockGridPropertyValueConverter(
IProfilingLogger proflog,
BlockEditorConverter blockConverter,
IJsonSerializer jsonSerializer,
IApiElementBuilder apiElementBuilder,
BlockGridPropertyValueConstructorCache constructorCache)
{
_proflog = proflog;
_blockConverter = blockConverter;
_jsonSerializer = jsonSerializer;
_apiElementBuilder = apiElementBuilder;
_constructorCache = constructorCache;
}

/// <inheritdoc />
Expand Down Expand Up @@ -129,7 +142,7 @@ ApiBlockGridArea CreateApiBlockGridArea(BlockGridArea area)
return null;
}

var creator = new BlockGridPropertyValueCreator(_blockConverter, _jsonSerializer);
var creator = new BlockGridPropertyValueCreator(_blockConverter, _jsonSerializer, _constructorCache);
return creator.CreateBlockModel(referenceCacheLevel, intermediateBlockModelValue, preview, configuration.Blocks, configuration.GridColumns);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;
internal class BlockGridPropertyValueCreator : BlockPropertyValueCreatorBase<BlockGridModel, BlockGridItem, BlockGridLayoutItem, BlockGridConfiguration.BlockGridBlockConfiguration>
{
private readonly IJsonSerializer _jsonSerializer;
private readonly BlockGridPropertyValueConstructorCache _constructorCache;

public BlockGridPropertyValueCreator(BlockEditorConverter blockEditorConverter, IJsonSerializer jsonSerializer)
public BlockGridPropertyValueCreator(BlockEditorConverter blockEditorConverter, IJsonSerializer jsonSerializer, BlockGridPropertyValueConstructorCache constructorCache)
: base(blockEditorConverter)
=> _jsonSerializer = jsonSerializer;
{
_jsonSerializer = jsonSerializer;
_constructorCache = constructorCache;
}

public BlockGridModel CreateBlockModel(PropertyCacheLevel referenceCacheLevel, string intermediateBlockModelValue, bool preview, BlockGridConfiguration.BlockGridBlockConfiguration[] blockConfigurations, int? gridColumns)
{
Expand Down Expand Up @@ -55,11 +59,12 @@ public BlockGridModel CreateBlockModel(PropertyCacheLevel referenceCacheLevel, s

protected override BlockEditorDataConverter CreateBlockEditorDataConverter() => new BlockGridEditorDataConverter(_jsonSerializer);

protected override BlockItemActivator<BlockGridItem> CreateBlockItemActivator() => new BlockGridItemActivator(BlockEditorConverter);
protected override BlockItemActivator<BlockGridItem> CreateBlockItemActivator() => new BlockGridItemActivator(BlockEditorConverter, _constructorCache);

private class BlockGridItemActivator : BlockItemActivator<BlockGridItem>
{
public BlockGridItemActivator(BlockEditorConverter blockConverter) : base(blockConverter)
public BlockGridItemActivator(BlockEditorConverter blockConverter, BlockGridPropertyValueConstructorCache constructorCache)
: base(blockConverter, constructorCache)
{
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
using Umbraco.Cms.Core.Models.Blocks;

namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

public class BlockListPropertyValueConstructorCache : BlockEditorPropertyValueConstructorCacheBase<BlockListItem>
{
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public class BlockListPropertyValueConverter : PropertyValueConverterBase, IDeli
private readonly IProfilingLogger _proflog;
private readonly BlockEditorConverter _blockConverter;
private readonly IApiElementBuilder _apiElementBuilder;
private readonly BlockListPropertyValueConstructorCache _constructorCache;

[Obsolete("Use the constructor that takes all parameters, scheduled for removal in V14")]
public BlockListPropertyValueConverter(IProfilingLogger proflog, BlockEditorConverter blockConverter)
Expand All @@ -36,12 +37,19 @@ public BlockListPropertyValueConverter(IProfilingLogger proflog, BlockEditorConv
{
}

[Obsolete("Use the constructor that takes all parameters, scheduled for removal in V15")]
public BlockListPropertyValueConverter(IProfilingLogger proflog, BlockEditorConverter blockConverter, IContentTypeService contentTypeService, IApiElementBuilder apiElementBuilder)
: this(proflog, blockConverter, contentTypeService, apiElementBuilder, StaticServiceProvider.Instance.GetRequiredService<BlockListPropertyValueConstructorCache>())
{
}

public BlockListPropertyValueConverter(IProfilingLogger proflog, BlockEditorConverter blockConverter, IContentTypeService contentTypeService, IApiElementBuilder apiElementBuilder, BlockListPropertyValueConstructorCache constructorCache)
{
_proflog = proflog;
_blockConverter = blockConverter;
_contentTypeService = contentTypeService;
_apiElementBuilder = apiElementBuilder;
_constructorCache = constructorCache;
}

/// <inheritdoc />
Expand Down Expand Up @@ -153,7 +161,7 @@ public Type GetDeliveryApiPropertyValueType(IPublishedPropertyType propertyType)
return null;
}

var creator = new BlockListPropertyValueCreator(_blockConverter);
var creator = new BlockListPropertyValueCreator(_blockConverter, _constructorCache);
return creator.CreateBlockModel(referenceCacheLevel, intermediateBlockModelValue, preview, configuration.Blocks);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

internal class BlockListPropertyValueCreator : BlockPropertyValueCreatorBase<BlockListModel, BlockListItem, BlockListLayoutItem, BlockListConfiguration.BlockConfiguration>
{
public BlockListPropertyValueCreator(BlockEditorConverter blockEditorConverter)
: base(blockEditorConverter)
{
}
private readonly BlockListPropertyValueConstructorCache _constructorCache;

public BlockListPropertyValueCreator(BlockEditorConverter blockEditorConverter, BlockListPropertyValueConstructorCache constructorCache)
: base(blockEditorConverter) =>
_constructorCache = constructorCache;

public BlockListModel CreateBlockModel(PropertyCacheLevel referenceCacheLevel, string intermediateBlockModelValue, bool preview, BlockListConfiguration.BlockConfiguration[] blockConfigurations)
{
Expand All @@ -22,11 +23,12 @@ public BlockListModel CreateBlockModel(PropertyCacheLevel referenceCacheLevel, s

protected override BlockEditorDataConverter CreateBlockEditorDataConverter() => new BlockListEditorDataConverter();

protected override BlockItemActivator<BlockListItem> CreateBlockItemActivator() => new BlockListItemActivator(BlockEditorConverter);
protected override BlockItemActivator<BlockListItem> CreateBlockItemActivator() => new BlockListItemActivator(BlockEditorConverter, _constructorCache);

private class BlockListItemActivator : BlockItemActivator<BlockListItem>
{
public BlockListItemActivator(BlockEditorConverter blockConverter) : base(blockConverter)
public BlockListItemActivator(BlockEditorConverter blockConverter, BlockListPropertyValueConstructorCache constructorCache)
: base(blockConverter, constructorCache)
{
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Umbraco.

Check notice on line 1 in src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockPropertyValueCreatorBase.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/13.0)

ℹ Getting worse: Missing Arguments Abstractions

The average number of function arguments increases from 4.43 to 4.57, threshold = 4.00. The functions in this file have too many arguments, indicating a lack of encapsulation or too many responsibilities in the same functions. Avoid adding more.
// See LICENSE for more details.

using System.Reflection;
Expand Down Expand Up @@ -216,26 +216,28 @@

// Cache constructors locally (it's tied to the current IPublishedSnapshot and IPublishedModelFactory)
protected abstract class BlockItemActivator<T>
where T : IBlockReference<IPublishedElement, IPublishedElement>
{
protected abstract Type GenericItemType { get; }

private readonly BlockEditorConverter _blockConverter;

private readonly
Dictionary<(Guid, Guid?), Func<Udi, IPublishedElement, Udi?, IPublishedElement?, T>>
_constructorCache = new();
private readonly BlockEditorPropertyValueConstructorCacheBase<T> _constructorCache;

public BlockItemActivator(BlockEditorConverter blockConverter)
=> _blockConverter = blockConverter;
public BlockItemActivator(BlockEditorConverter blockConverter, BlockEditorPropertyValueConstructorCacheBase<T> constructorCache)
{
_blockConverter = blockConverter;
_constructorCache = constructorCache;
}

public T CreateInstance(Guid contentTypeKey, Guid? settingsTypeKey, Udi contentUdi, IPublishedElement contentData, Udi? settingsUdi, IPublishedElement? settingsData)
{
if (!_constructorCache.TryGetValue(
(contentTypeKey, settingsTypeKey),
out Func<Udi, IPublishedElement, Udi?, IPublishedElement?, T>? constructor))
{
constructor = _constructorCache[(contentTypeKey, settingsTypeKey)] =
EmitConstructor(contentTypeKey, settingsTypeKey);
constructor = EmitConstructor(contentTypeKey, settingsTypeKey);
_constructorCache.SetValue((contentTypeKey, settingsTypeKey), constructor);
}

return constructor(contentUdi, contentData, settingsUdi, settingsData);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Notifications;

namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

public class ConstructorCacheClearNotificationHandler :
INotificationHandler<ContentTypeCacheRefresherNotification>,
INotificationHandler<DataTypeCacheRefresherNotification>
{
private readonly BlockListPropertyValueConstructorCache _blockListPropertyValueConstructorCache;
private readonly BlockGridPropertyValueConstructorCache _blockGridPropertyValueConstructorCache;
private readonly RichTextBlockPropertyValueConstructorCache _richTextBlockPropertyValueConstructorCache;

public ConstructorCacheClearNotificationHandler(
BlockListPropertyValueConstructorCache blockListPropertyValueConstructorCache,
BlockGridPropertyValueConstructorCache blockGridPropertyValueConstructorCache,
RichTextBlockPropertyValueConstructorCache richTextBlockPropertyValueConstructorCache)
{
_blockListPropertyValueConstructorCache = blockListPropertyValueConstructorCache;
_blockGridPropertyValueConstructorCache = blockGridPropertyValueConstructorCache;
_richTextBlockPropertyValueConstructorCache = richTextBlockPropertyValueConstructorCache;
}

public void Handle(ContentTypeCacheRefresherNotification notification)
=> ClearCaches();

public void Handle(DataTypeCacheRefresherNotification notification)
=> ClearCaches();

private void ClearCaches()
{
// must clear the block item constructor caches whenever content types and data types change,
// otherwise InMemoryAuto generated models will not work.
_blockListPropertyValueConstructorCache.Clear();
_blockGridPropertyValueConstructorCache.Clear();
_richTextBlockPropertyValueConstructorCache.Clear();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
using Umbraco.Cms.Core.Models.Blocks;

namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

public class RichTextBlockPropertyValueConstructorCache : BlockEditorPropertyValueConstructorCacheBase<RichTextBlockItem>
{
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;

internal class RichTextBlockPropertyValueCreator : BlockPropertyValueCreatorBase<RichTextBlockModel, RichTextBlockItem, RichTextBlockLayoutItem, RichTextConfiguration.RichTextBlockConfiguration>
{
public RichTextBlockPropertyValueCreator(BlockEditorConverter blockEditorConverter)
private readonly RichTextBlockPropertyValueConstructorCache _constructorCache;

public RichTextBlockPropertyValueCreator(BlockEditorConverter blockEditorConverter, RichTextBlockPropertyValueConstructorCache constructorCache)
: base(blockEditorConverter)
{
}
=> _constructorCache = constructorCache;

public RichTextBlockModel CreateBlockModel(PropertyCacheLevel referenceCacheLevel, BlockValue blockValue, bool preview, RichTextConfiguration.RichTextBlockConfiguration[] blockConfigurations)
{
Expand All @@ -25,11 +26,12 @@ public RichTextBlockModel CreateBlockModel(PropertyCacheLevel referenceCacheLeve

protected override BlockEditorDataConverter CreateBlockEditorDataConverter() => new RichTextEditorBlockDataConverter();

protected override BlockItemActivator<RichTextBlockItem> CreateBlockItemActivator() => new RichTextBlockItemActivator(BlockEditorConverter);
protected override BlockItemActivator<RichTextBlockItem> CreateBlockItemActivator() => new RichTextBlockItemActivator(BlockEditorConverter, _constructorCache);

private class RichTextBlockItemActivator : BlockItemActivator<RichTextBlockItem>
{
public RichTextBlockItemActivator(BlockEditorConverter blockConverter) : base(blockConverter)
public RichTextBlockItemActivator(BlockEditorConverter blockConverter, RichTextBlockPropertyValueConstructorCache constructorCache)
: base(blockConverter, constructorCache)
{
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
private readonly IJsonSerializer _jsonSerializer;
private readonly ILogger<RteMacroRenderingValueConverter> _logger;
private readonly IApiElementBuilder _apiElementBuilder;
private readonly RichTextBlockPropertyValueConstructorCache _constructorCache;
private DeliveryApiSettings _deliveryApiSettings;

[Obsolete("Please use the constructor that takes all arguments. Will be removed in V14.")]
Expand Down Expand Up @@ -79,6 +80,7 @@
StaticServiceProvider.Instance.GetRequiredService<BlockEditorConverter>(),
StaticServiceProvider.Instance.GetRequiredService<IJsonSerializer>(),
StaticServiceProvider.Instance.GetRequiredService<IApiElementBuilder>(),
StaticServiceProvider.Instance.GetRequiredService<RichTextBlockPropertyValueConstructorCache>(),
StaticServiceProvider.Instance.GetRequiredService<ILogger<RteMacroRenderingValueConverter>>(),
deliveryApiSettingsMonitor
)
Expand All @@ -89,20 +91,21 @@
HtmlLocalLinkParser linkParser, HtmlUrlParser urlParser, HtmlImageSourceParser imageSourceParser,
IApiRichTextElementParser apiRichTextElementParser, IApiRichTextMarkupParser apiRichTextMarkupParser,
IPartialViewBlockEngine partialViewBlockEngine, BlockEditorConverter blockEditorConverter, IJsonSerializer jsonSerializer,
IApiElementBuilder apiElementBuilder, ILogger<RteMacroRenderingValueConverter> logger,
IApiElementBuilder apiElementBuilder, RichTextBlockPropertyValueConstructorCache constructorCache, ILogger<RteMacroRenderingValueConverter> logger,
IOptionsMonitor<DeliveryApiSettings> deliveryApiSettingsMonitor)
{
_umbracoContextAccessor = umbracoContextAccessor;
_macroRenderer = macroRenderer;
_linkParser = linkParser;
_urlParser = urlParser;
_imageSourceParser = imageSourceParser;
_apiRichTextElementParser = apiRichTextElementParser;
_apiRichTextMarkupParser = apiRichTextMarkupParser;
_partialViewBlockEngine = partialViewBlockEngine;
_blockEditorConverter = blockEditorConverter;
_jsonSerializer = jsonSerializer;
_apiElementBuilder = apiElementBuilder;
_constructorCache = constructorCache;

Check notice on line 108 in src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/RteMacroRenderingValueConverter.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (release/13.0)

ℹ Getting worse: Constructor Over-Injection

RteMacroRenderingValueConverter increases from 13 to 14 arguments, threshold = 5. This constructor has too many arguments, indicating an object with low cohesion or missing function argument abstraction. Avoid adding more arguments.
_logger = logger;
_deliveryApiSettings = deliveryApiSettingsMonitor.CurrentValue;
deliveryApiSettingsMonitor.OnChange(settings => _deliveryApiSettings = settings);
Expand Down Expand Up @@ -267,7 +270,7 @@
return null;
}

var creator = new RichTextBlockPropertyValueCreator(_blockEditorConverter);
var creator = new RichTextBlockPropertyValueCreator(_blockEditorConverter, _constructorCache);
return creator.CreateBlockModel(referenceCacheLevel, blocks, preview, configuration.Blocks);
}

Expand Down
Loading