Micro-optimisation: Avoid Activator.CreateInstance when creating MediaWithCrops

src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/MediaPickerWithCropsValueConverter.cs

@@ -1,3 +1,5 @@
+using System.Collections.Concurrent;
+using System.Reflection;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.DeliveryApi;
 using Umbraco.Cms.Core.Models.PublishedContent;
@@ -17,6 +19,8 @@ namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;
 [DefaultPropertyValueConverter]
 public class MediaPickerWithCropsValueConverter : PropertyValueConverterBase, IDeliveryApiPropertyValueConverter
 {
+    private static readonly ConcurrentDictionary<Type, ConstructorInvoker> _mediaWithCropsFactories = new();
+
     private readonly IJsonSerializer _jsonSerializer;
     private readonly IPublishedMediaCache _publishedMediaCache;
     private readonly IPublishedUrlProvider _publishedUrlProvider;
@@ -134,9 +138,7 @@ public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType
 
                 localCrops.ApplyConfiguration(configuration);
 
-                // TODO: This should be optimized/cached, as calling Activator.CreateInstance is slow
-                Type mediaWithCropsType = typeof(MediaWithCrops<>).MakeGenericType(mediaItem.GetType());
-                var mediaWithCrops = (MediaWithCrops)Activator.CreateInstance(mediaWithCropsType, mediaItem, _publishedValueFallback, localCrops)!;
+                MediaWithCrops mediaWithCrops = CreateMediaWithCrops(mediaItem, _publishedValueFallback, localCrops);
 
                 mediaItems.Add(mediaWithCrops);
 
@@ -201,12 +203,29 @@ public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType
         }
         if (isMultiple == false && converted is MediaWithCrops mediaWithCrops)
         {
-            return new [] { ToApiMedia(mediaWithCrops) };
+            return new[] { ToApiMedia(mediaWithCrops) };
         }
 
         return Array.Empty<IApiMediaWithCrops>();
     }
 
     private bool IsMultipleDataType(PublishedDataType dataType) =>
         dataType.ConfigurationAs<MediaPicker3Configuration>()?.Multiple ?? false;
+
+    private static MediaWithCrops CreateMediaWithCrops(
+        IPublishedContent mediaItem,
+        IPublishedValueFallback publishedValueFallback,
+        ImageCropperValue localCrops)
+    {
+        ConstructorInvoker factory =
+            _mediaWithCropsFactories.GetOrAdd(mediaItem.GetType(), static mediaType =>
+            {
+                Type closedType = typeof(MediaWithCrops<>).MakeGenericType(mediaType);
+                ConstructorInfo ctor = closedType.GetConstructor(
+                    [mediaType, typeof(IPublishedValueFallback), typeof(ImageCropperValue)])!;
+                return ConstructorInvoker.Create(ctor);
+            });
+
+        return (MediaWithCrops)factory.Invoke(mediaItem, publishedValueFallback, localCrops);
+    }
 }

tests/Umbraco.Tests.Benchmarks/MediaCropsBenchmark.cs

@@ -0,0 +1,153 @@
+// Copyright (c) Umbraco.
+// See LICENSE for more details.
+
+using System.Collections.Concurrent;
+using System.Reflection;
+using BenchmarkDotNet.Attributes;
+using Umbraco.Cms.Core.Models;
+using Umbraco.Cms.Core.Models.PublishedContent;
+using Umbraco.Cms.Core.PropertyEditors.ValueConverters;
+using Umbraco.Tests.Benchmarks.Config;
+
+namespace Umbraco.Tests.Benchmarks
+{
+    [QuickRunWithMemoryDiagnoserConfig]
+    public class MediaCropsBenchmark
+    {
+        private sealed class StubPublishedContent : IPublishedContent
+        {
+            public int Id => 1;
+            public string Name => "Test";
+            public string? UrlSegment => "test";
+            public int SortOrder => 0;
+            public int Level => 1;
+            public string Path => "-1,1";
+            public int? TemplateId => null;
+            public int CreatorId => 0;
+            public DateTime CreateDate => DateTime.MinValue;
+            public int WriterId => 0;
+            public DateTime UpdateDate => DateTime.MinValue;
+            public IReadOnlyDictionary<string, PublishedCultureInfo> Cultures => new Dictionary<string, PublishedCultureInfo>();
+            public PublishedItemType ItemType => PublishedItemType.Media;
+
+            [Obsolete("Use extension methods.")]
+            public IPublishedContent? Parent => null;
+
+            [Obsolete("Use extension methods.")]
+            public IEnumerable<IPublishedContent> Children => Enumerable.Empty<IPublishedContent>();
+
+            public bool IsDraft(string? culture = null) => false;
+            public bool IsPublished(string? culture = null) => true;
+
+            public IPublishedContentType ContentType => null!;
+            public Guid Key => Guid.Empty;
+            public IEnumerable<IPublishedProperty> Properties => Enumerable.Empty<IPublishedProperty>();
+            public IPublishedProperty? GetProperty(string alias) => null;
+        }
+
+        private sealed class StubPublishedValueFallback : IPublishedValueFallback
+        {
+            public bool TryGetValue(IPublishedProperty property, string? culture, string? segment, Fallback fallback, object? defaultValue, out object? value)
+            { value = defaultValue; return false; }
+
+            public bool TryGetValue<T>(IPublishedProperty property, string? culture, string? segment, Fallback fallback, T? defaultValue, out T? value)
+            { value = defaultValue; return false; }
+
+            public bool TryGetValue(IPublishedElement content, string alias, string? culture, string? segment, Fallback fallback, object? defaultValue, out object? value)
+            { value = defaultValue; return false; }
+
+            public bool TryGetValue<T>(IPublishedElement content, string alias, string? culture, string? segment, Fallback fallback, T? defaultValue, out T? value)
+            { value = defaultValue; return false; }
+
+            public bool TryGetValue(IPublishedContent content, string alias, string? culture, string? segment, Fallback fallback, object? defaultValue, out object? value, out IPublishedProperty? noValueProperty)
+            { value = defaultValue; noValueProperty = null; return false; }
+
+            public bool TryGetValue<T>(IPublishedContent content, string alias, string? culture, string? segment, Fallback fallback, T defaultValue, out T? value, out IPublishedProperty? noValueProperty)
+            { value = defaultValue; noValueProperty = null; return false; }
+        }
+
+        // -------------------------------------------------------------------------
+        // Shared state
+        // -------------------------------------------------------------------------
+
+        private static readonly IPublishedContent MediaItem = new StubPublishedContent();
+        private static readonly IPublishedValueFallback Fallback = new StubPublishedValueFallback();
+        private static readonly ImageCropperValue LocalCrops = new() { Src = "/media/test.jpg" };
+        private static readonly IPublishedContent[] TenMediaItems = Enumerable.Range(0, 10).Select(_ => new StubPublishedContent()).ToArray();
+
+        private static readonly ConcurrentDictionary<Type, ConstructorInvoker> _factories = new();
+
+        // -------------------------------------------------------------------------
+        // After: compiled Expression delegate (production code path)
+        // -------------------------------------------------------------------------
+
+        [Benchmark(Baseline = true, Description = "After: single item (compiled delegate, warm)")]
+        public MediaWithCrops After_Single() =>
+            CreateMediaWithCropsNew(_factories, MediaItem, Fallback, LocalCrops);
+
+        [Benchmark(Description = "After: ten items (compiled delegate, warm)")]
+        public MediaWithCrops After_Ten()
+        {
+            MediaWithCrops last = null!;
+            foreach (IPublishedContent item in TenMediaItems)
+            {
+                last = CreateMediaWithCropsNew(_factories, item, Fallback, LocalCrops);
+            }
+            return last;
+        }
+
+        // -------------------------------------------------------------------------
+        // Before: Activator.CreateInstance (original code path)
+        // -------------------------------------------------------------------------
+
+        [Benchmark(Description = "Before: single item (Activator.CreateInstance)")]
+        public MediaWithCrops Before_Single() => CreateMediaWithCropsOld(MediaItem, Fallback, LocalCrops);
+
+        [Benchmark(Description = "Before: ten items (Activator.CreateInstance)")]
+        public MediaWithCrops Before_Ten()
+        {
+            MediaWithCrops last = null!;
+            foreach (IPublishedContent item in TenMediaItems)
+            {
+                last = CreateMediaWithCropsOld(item, Fallback, LocalCrops);
+            }
+            return last;
+        }
+
+        // -------------------------------------------------------------------------
+        // Old implementation
+        // -------------------------------------------------------------------------
+
+        private static MediaWithCrops CreateMediaWithCropsOld(
+            IPublishedContent mediaItem,
+            IPublishedValueFallback publishedValueFallback,
+            ImageCropperValue localCrops)
+        {
+            Type mediaType = mediaItem.GetType();
+            Type closedType = typeof(MediaWithCrops<>).MakeGenericType(mediaType);
+            return (MediaWithCrops)Activator.CreateInstance(closedType, mediaItem, publishedValueFallback, localCrops)!;
+        }
+
+        // -------------------------------------------------------------------------
+        // New implementation
+        // -------------------------------------------------------------------------
+
+        private static MediaWithCrops CreateMediaWithCropsNew(
+            ConcurrentDictionary<Type, ConstructorInvoker> factories,
+            IPublishedContent mediaItem,
+            IPublishedValueFallback publishedValueFallback,
+            ImageCropperValue localCrops)
+        {
+            ConstructorInvoker factory = factories.GetOrAdd(mediaItem.GetType(), static mediaType => CompileFactory(mediaType));
+            return (MediaWithCrops)factory.Invoke(mediaItem, publishedValueFallback, localCrops);
+        }
+
+        private static ConstructorInvoker CompileFactory(Type mediaType)
+        {
+            Type closedType = typeof(MediaWithCrops<>).MakeGenericType(mediaType);
+            ConstructorInfo ctor = closedType.GetConstructor(
+                [mediaType, typeof(IPublishedValueFallback), typeof(ImageCropperValue)])!;
+            return ConstructorInvoker.Create(ctor);
+        }
+    }
+}

tests/Umbraco.Tests.UnitTests/Umbraco.Core/DeliveryApi/MediaPickerWithCropsValueConverterTests.cs

@@ -1,7 +1,8 @@
 using Moq;
 using NUnit.Framework;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.DeliveryApi;
+using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.DeliveryApi;
 using Umbraco.Cms.Core.Models.PublishedContent;
 using Umbraco.Cms.Core.PropertyEditors;
@@ -296,6 +297,66 @@
         Assert.IsEmpty(result);
     }
 
+    [Test]
+    public void MediaPickerWithCropsValueConverter_InSingleMode_ConvertsValueToStronglyTypedMediaWithCrops()
+    {
+        var publishedPropertyType = SetupMediaPropertyType(false);
+
+        TestMediaModelOne? media = null;
+        var mediaKey = SetupMedia("My media", ".jpg", 200, 400, "My alt text", 800, asModel: inner => media = new TestMediaModelOne(inner));
+
+        var valueConverter = MediaPickerWithCropsValueConverter();
+        var inter = SerializeMediaWithCropsDtos(mediaKey);
+
+        var result = valueConverter.ConvertIntermediateToObject(Mock.Of<IPublishedElement>(), publishedPropertyType, PropertyCacheLevel.Element, inter, false);
+
+        Assert.AreEqual(typeof(MediaWithCrops<TestMediaModelOne>), result.GetType());
+        Assert.AreSame(media, ((MediaWithCrops<TestMediaModelOne>)result).Content);
+    }
+
+    [Test]
+    public void MediaPickerWithCropsValueConverter_InMultiMode_ConvertsEachValueToItsOwnStronglyTypedMediaWithCrops()
+    {
+        var publishedPropertyType = SetupMediaPropertyType(true);
+
+        TestMediaModelOne? firstMedia = null;
+        TestMediaModelTwo? secondMedia = null;
+        var firstMediaKey = SetupMedia("First media", ".jpg", 200, 400, "First alt text", 800, asModel: inner => firstMedia = new TestMediaModelOne(inner));
+        var secondMediaKey = SetupMedia("Second media", ".png", 300, 600, "Second alt text", 900, asModel: inner => secondMedia = new TestMediaModelTwo(inner));
+
+        var valueConverter = MediaPickerWithCropsValueConverter();
+        var inter = SerializeMediaWithCropsDtos(firstMediaKey, secondMediaKey);
+
+        // convert twice; the first pass populates the constructor cache, the second one exercises it
+        for (var iteration = 0; iteration < 2; iteration++)
+        {
+            var result = valueConverter.ConvertIntermediateToObject(Mock.Of<IPublishedElement>(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable<MediaWithCrops>;
+            Assert.NotNull(result);
+
+            var mediaWithCrops = result.ToArray();
+            Assert.AreEqual(2, mediaWithCrops.Length);
+
+            Assert.AreEqual(typeof(MediaWithCrops<TestMediaModelOne>), mediaWithCrops[0].GetType());
+            Assert.AreEqual(typeof(MediaWithCrops<TestMediaModelTwo>), mediaWithCrops[1].GetType());
+
+            Assert.AreSame(firstMedia, ((MediaWithCrops<TestMediaModelOne>)mediaWithCrops[0]).Content);
+            Assert.AreSame(secondMedia, ((MediaWithCrops<TestMediaModelTwo>)mediaWithCrops[1]).Content);
+        }
+    }
+
+    private string SerializeMediaWithCropsDtos(params Guid[] mediaKeys)
+    {
+        var serializer = new SystemTextJsonSerializer(new DefaultJsonSerializerEncoderFactory());
+        return serializer.Serialize(mediaKeys.Select(mediaKey =>
+            new MediaPicker3PropertyEditor.MediaPicker3PropertyValueEditor.MediaWithCropsDto
+            {
+                Key = Guid.NewGuid(),
+                MediaKey = mediaKey,
+                Crops = Array.Empty<ImageCropperValue.ImageCropperCrop>(),
+                FocalPoint = new ImageCropperValue.ImageCropperFocalPoint { Left = .2m, Top = .4m }
+            }).ToArray());
+    }
+
     private IPublishedPropertyType SetupMediaPropertyType(bool multiSelect)
     {
         var publishedDataType = new PublishedDataType(123, "test", "test", new Lazy<object>(() => new MediaPicker3Configuration
@@ -316,7 +377,7 @@
         return publishedPropertyType.Object;
     }
 
-    private Guid SetupMedia(string name, string extension, int width, int height, string altText, int bytes, ImageCropperValue? imageCropperValue = null)
+    private Guid SetupMedia(string name, string extension, int width, int height, string altText, int bytes, ImageCropperValue? imageCropperValue = null, Func<IPublishedContent, IPublishedContent>? asModel = null)
     {
         var publishedMediaType = new Mock<IPublishedContentType>();
         publishedMediaType.SetupGet(c => c.ItemType).Returns(PublishedItemType.Media);
@@ -344,15 +405,17 @@
         AddProperty(Constants.Conventions.Media.File, imageCropperValue);
         AddProperty("altText", altText);
 
+        IPublishedContent mediaItem = asModel is null ? media.Object : asModel(media.Object);
+
         PublishedMediaCacheMock
             .Setup(pcc => pcc.GetById(mediaKey))
-            .Returns(media.Object);
+            .Returns(mediaItem);
         PublishedMediaCacheMock
             .Setup(pcc => pcc.GetById(It.IsAny<bool>(), mediaKey))
-            .Returns(media.Object);
+            .Returns(mediaItem);
 
         PublishedUrlProviderMock
-            .Setup(p => p.GetMediaUrl(media.Object, It.IsAny<UrlMode>(), It.IsAny<string?>(), It.IsAny<string?>(), It.IsAny<Uri?>()))
+            .Setup(p => p.GetMediaUrl(mediaItem, It.IsAny<UrlMode>(), It.IsAny<string?>(), It.IsAny<string?>(), It.IsAny<Uri?>()))
             .Returns(name.ToLowerInvariant().Replace(" ", "-"));
 
         return mediaKey;
@@ -402,4 +465,22 @@
         Assert.AreEqual(expectedY1, actual.Coordinates.Y1);
         Assert.AreEqual(expectedY2, actual.Coordinates.Y2);
     }
+
+    // two distinct media model types, shaped like the models ModelsBuilder generates, so the converter
+    // has to close MediaWithCrops<> over a different type per media item
+    private sealed class TestMediaModelOne : PublishedContentWrapped
+    {
+        public TestMediaModelOne(IPublishedContent content)
+            : base(content)
+        {
+        }
+    }
+
+    private sealed class TestMediaModelTwo : PublishedContentWrapped
+    {
+        public TestMediaModelTwo(IPublishedContent content)
+            : base(content)
+        {
+        }
+    }
 }