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

Improve ShapeTableManager performance #15620

Merged
merged 4 commits into from
Apr 1, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -3,7 +3,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using OrchardCore.DisplayManagement.Extensions;
Expand All @@ -19,109 +19,119 @@ namespace OrchardCore.DisplayManagement.Descriptors
/// </summary>
public class DefaultShapeTableManager : IShapeTableManager
{
private const string DefaultThemeIdKey = "_ShapeTable";

// FeatureShapeDescriptors are identical across tenants so they can be reused statically. Each shape table will
// create a unique list of these per tenant.
private static readonly ConcurrentDictionary<string, FeatureShapeDescriptor> _shapeDescriptors = new();

private static readonly object _syncLock = new();

private readonly IHostEnvironment _hostingEnvironment;
private readonly IEnumerable<IShapeTableProvider> _bindingStrategies;
private readonly IShellFeaturesManager _shellFeaturesManager;
private readonly IExtensionManager _extensionManager;
private readonly ITypeFeatureProvider _typeFeatureProvider;
private readonly IMemoryCache _memoryCache;
// Singleton cache to hold a tenant's theme ShapeTable
private readonly Dictionary<string, ShapeTable> _shapeTableCache;
Copy link
Member Author

Choose a reason for hiding this comment

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

Using a singleton dictionary as the cache, like IMemoryCache, with the difference that it is indexed by something we don't need to make unique with a prefix since it is only used here.
The dependency is keyed just in case some other component might want to also register the same type.


private readonly IServiceProvider _serviceProvider;
private readonly ILogger _logger;

public DefaultShapeTableManager(
IHostEnvironment hostingEnvironment,
IEnumerable<IShapeTableProvider> bindingStrategies,
IShellFeaturesManager shellFeaturesManager,
IExtensionManager extensionManager,
ITypeFeatureProvider typeFeatureProvider,
IMemoryCache memoryCache,
[FromKeyedServices(nameof(DefaultShapeTableManager))] Dictionary<string, ShapeTable> shapeTableCache,
IServiceProvider serviceProvider,
ILogger<DefaultShapeTableManager> logger)
{
_hostingEnvironment = hostingEnvironment;
_bindingStrategies = bindingStrategies;
_shellFeaturesManager = shellFeaturesManager;
_extensionManager = extensionManager;
_typeFeatureProvider = typeFeatureProvider;
_memoryCache = memoryCache;
_shapeTableCache = shapeTableCache;
_serviceProvider = serviceProvider;
_logger = logger;
}

public ShapeTable GetShapeTable(string themeId)
=> GetShapeTableAsync(themeId).GetAwaiter().GetResult();

public async Task<ShapeTable> GetShapeTableAsync(string themeId)
public Task<ShapeTable> GetShapeTableAsync(string themeId)
{
var cacheKey = $"ShapeTable:{themeId}";
// This method is intentionally kept non-async since most calls
// are from cache.

if (!_memoryCache.TryGetValue(cacheKey, out ShapeTable shapeTable))
if (_shapeTableCache.TryGetValue(themeId ?? DefaultThemeIdKey, out var shapeTable))
{
_logger.LogInformation("Start building shape table");
return Task.FromResult(shapeTable);
}

HashSet<string> excludedFeatures;
return BuildShapeTableAsync(themeId);
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracting the async path allows dotnet to optimize the call for the most common case which is when we read the shape table from cache.

}

// Here we don't use a lock for thread safety but for atomicity.
lock (_syncLock)
{
excludedFeatures = new HashSet<string>(_shapeDescriptors.Select(kv => kv.Value.Feature.Id));
}
private async Task<ShapeTable> BuildShapeTableAsync(string themeId)
{
_logger.LogInformation("Start building shape table");

var shapeDescriptors = new Dictionary<string, FeatureShapeDescriptor>();
// These services are resolved lazily since they are only required when initializing the shape tables
// during the first request. And binding strategies would be expensive to build since this service is called many times
// per request.

foreach (var bindingStrategy in _bindingStrategies)
{
var strategyFeature = _typeFeatureProvider.GetFeatureForDependency(bindingStrategy.GetType());
var hostingEnvironment = _serviceProvider.GetRequiredService<IHostEnvironment>();
var bindingStrategies = _serviceProvider.GetRequiredService<IEnumerable<IShapeTableProvider>>();
Copy link
Member Author

Choose a reason for hiding this comment

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

This one specifically creates a list of IShapeTableProvider which themselves have other collections resolved. And this was done multiple times per request since it was transient. Now the collections are materialized (with resolved elements) only when the shape is actually built.

var shellFeaturesManager = _serviceProvider.GetRequiredService<IShellFeaturesManager>();
var extensionManager = _serviceProvider.GetRequiredService<IExtensionManager>();
var typeFeatureProvider = _serviceProvider.GetRequiredService<ITypeFeatureProvider>();

var builder = new ShapeTableBuilder(strategyFeature, excludedFeatures);
await bindingStrategy.DiscoverAsync(builder);
var builtAlterations = builder.BuildAlterations();
HashSet<string> excludedFeatures;

BuildDescriptors(bindingStrategy, builtAlterations, shapeDescriptors);
}
// Here we don't use a lock for thread safety but for atomicity.
lock (_syncLock)
{
excludedFeatures = new HashSet<string>(_shapeDescriptors.Select(kv => kv.Value.Feature.Id));
}

// Here we don't use a lock for thread safety but for atomicity.
lock (_syncLock)
{
foreach (var kv in shapeDescriptors)
{
_shapeDescriptors[kv.Key] = kv.Value;
}
}
var shapeDescriptors = new Dictionary<string, FeatureShapeDescriptor>();

var enabledAndOrderedFeatureIds = (await _shellFeaturesManager.GetEnabledFeaturesAsync())
.Select(f => f.Id)
.ToList();
foreach (var bindingStrategy in bindingStrategies)
{
var strategyFeature = typeFeatureProvider.GetFeatureForDependency(bindingStrategy.GetType());

var builder = new ShapeTableBuilder(strategyFeature, excludedFeatures);
await bindingStrategy.DiscoverAsync(builder);
var builtAlterations = builder.BuildAlterations();

BuildDescriptors(bindingStrategy, builtAlterations, shapeDescriptors);
}

// let the application acting as a super theme for shapes rendering.
if (enabledAndOrderedFeatureIds.Remove(_hostingEnvironment.ApplicationName))
// Here we don't use a lock for thread safety but for atomicity.
lock (_syncLock)
{
foreach (var kv in shapeDescriptors)
{
enabledAndOrderedFeatureIds.Add(_hostingEnvironment.ApplicationName);
_shapeDescriptors[kv.Key] = kv.Value;
}
}

var descriptors = _shapeDescriptors
.Where(sd => enabledAndOrderedFeatureIds.Contains(sd.Value.Feature.Id))
.Where(sd => IsModuleOrRequestedTheme(sd.Value.Feature, themeId))
.OrderBy(sd => enabledAndOrderedFeatureIds.IndexOf(sd.Value.Feature.Id))
.GroupBy(sd => sd.Value.ShapeType, StringComparer.OrdinalIgnoreCase)
.Select(group => new ShapeDescriptorIndex
(
shapeType: group.Key,
alterationKeys: group.Select(kv => kv.Key),
descriptors: _shapeDescriptors
))
.ToList();
var enabledAndOrderedFeatureIds = (await shellFeaturesManager.GetEnabledFeaturesAsync())
.Select(f => f.Id)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.Select(f => f.Id)
.OrderBy(f => f.Id)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why change the previous code?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how these Ids are ordered without OrderBy?

.ToList();

// let the application acting as a super theme for shapes rendering.
if (enabledAndOrderedFeatureIds.Remove(hostingEnvironment.ApplicationName))
{
enabledAndOrderedFeatureIds.Add(hostingEnvironment.ApplicationName);
}

shapeTable = new ShapeTable
var descriptors = _shapeDescriptors
.Where(sd => enabledAndOrderedFeatureIds.Contains(sd.Value.Feature.Id))
.Where(sd => IsModuleOrRequestedTheme(extensionManager, sd.Value.Feature, themeId))
.OrderBy(sd => enabledAndOrderedFeatureIds.IndexOf(sd.Value.Feature.Id))
.GroupBy(sd => sd.Value.ShapeType, StringComparer.OrdinalIgnoreCase)
.Select(group => new ShapeDescriptorIndex
(
descriptors: descriptors.ToDictionary(sd => sd.ShapeType, x => (ShapeDescriptor)x, StringComparer.OrdinalIgnoreCase),
bindings: descriptors.SelectMany(sd => sd.Bindings).ToDictionary(kv => kv.Key, kv => kv.Value, StringComparer.OrdinalIgnoreCase)
);
shapeType: group.Key,
alterationKeys: group.Select(kv => kv.Key),
descriptors: _shapeDescriptors
))
.ToList();

_logger.LogInformation("Done building shape table");
var shapeTable = new ShapeTable
(
descriptors: descriptors.ToDictionary(sd => sd.ShapeType, x => (ShapeDescriptor)x, StringComparer.OrdinalIgnoreCase),
bindings: descriptors.SelectMany(sd => sd.Bindings).ToDictionary(kv => kv.Key, kv => kv.Value, StringComparer.OrdinalIgnoreCase)
);

_memoryCache.Set(cacheKey, shapeTable, new MemoryCacheEntryOptions { Priority = CacheItemPriority.NeverRemove });
}
_logger.LogInformation("Done building shape table");

_shapeTableCache[themeId ?? DefaultThemeIdKey] = shapeTable;

return shapeTable;
}
Expand Down Expand Up @@ -159,7 +169,7 @@ private static void BuildDescriptors(
}
}

private bool IsModuleOrRequestedTheme(IFeatureInfo feature, string themeId)
private static bool IsModuleOrRequestedTheme(IExtensionManager extensionManager, IFeatureInfo feature, string themeId)
{
if (!feature.IsTheme())
{
Expand All @@ -172,13 +182,13 @@ private bool IsModuleOrRequestedTheme(IFeatureInfo feature, string themeId)
}

return feature.Id == themeId || IsBaseTheme(feature.Id, themeId);
}

private bool IsBaseTheme(string themeFeatureId, string themeId)
{
return _extensionManager
.GetFeatureDependencies(themeId)
.Any(f => f.Id == themeFeatureId);
bool IsBaseTheme(string themeFeatureId, string themeId)
Copy link
Member

@hishamco hishamco Mar 31, 2024

Choose a reason for hiding this comment

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

I don't think it's right to use it as a local function, but no problem while you make IsModuleOrRequestedTheme static

{
return extensionManager
.GetFeatureDependencies(themeId)
.Any(f => f.Id == themeFeatureId);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
using System;
using System.Threading.Tasks;

namespace OrchardCore.DisplayManagement.Descriptors;

public interface IShapeTableManager
{
[Obsolete($"Instead, utilize the {nameof(GetShapeTableAsync)} method. This current method is slated for removal in upcoming releases.")]
ShapeTable GetShapeTable(string themeId);

Task<ShapeTable> GetShapeTableAsync(string themeId);
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Collections.Generic;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ApplicationParts;
using Microsoft.AspNetCore.Mvc.Razor.Compilation;
Expand Down Expand Up @@ -57,7 +58,8 @@ public static OrchardCoreBuilder AddTheming(this OrchardCoreBuilder builder)
services.AddScoped<IViewLocationExpanderProvider, ThemeViewLocationExpanderProvider>();

services.AddScoped<IShapeTemplateHarvester, BasicShapeTemplateHarvester>();
services.AddTransient<IShapeTableManager, DefaultShapeTableManager>();
services.AddKeyedSingleton<Dictionary<string, ShapeTable>>(nameof(DefaultShapeTableManager));
services.AddScoped<IShapeTableManager, DefaultShapeTableManager>();

services.AddScoped<IShapeTableProvider, ShapeAttributeBindingStrategy>();
services.AddScoped<IShapeTableProvider, ShapePlacementParsingStrategy>();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using DocumentFormat.OpenXml.Office2016.Drawing.ChartDrawing;
Copy link
Member

Choose a reason for hiding this comment

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

It seems this has been added accidentally?!!

sebastienros marked this conversation as resolved.
Show resolved Hide resolved
using OrchardCore.DisplayManagement.Descriptors;
using OrchardCore.DisplayManagement.Extensions;
using OrchardCore.DisplayManagement.Implementation;
Expand Down Expand Up @@ -118,6 +119,7 @@ public DefaultShapeTableManagerTests()
serviceCollection.AddMemoryCache();
serviceCollection.AddScoped<IShellFeaturesManager, TestShellFeaturesManager>();
serviceCollection.AddScoped<IShapeTableManager, DefaultShapeTableManager>();
serviceCollection.AddKeyedSingleton<Dictionary<string, ShapeTable>>(nameof(DefaultShapeTableManager));
serviceCollection.AddSingleton<ITypeFeatureProvider, TypeFeatureProvider>();
serviceCollection.AddSingleton<IHostEnvironment>(new StubHostingEnvironment());

Expand Down
Loading