Performance: Reduce number of database calls in save and publish operations (#20485)

* Added request caching to media picker media retrieval, to improve performance in save operations.

* WIP: Update or insert in bulk when updating property data.

* Add tests verifying UpdateBatch.

* Fixed issue with UpdateBatch and SQL Server.

* Removed stopwatch.

* Fix test on SQLite (failing on SQLServer).

* Added temporary test for direct call to NPoco UpdateBatch.

* Fixed test on SQLServer.

* Add integration test verifying the same property data is persisted as before the performance refactor.

* Log expected warning in DocumentUrlService as debug.

(cherry picked from commit 12adfd5)

Author: AndyButland

src/Umbraco.Core/Services/DocumentUrlService.cs

@@ -432,7 +432,8 @@ private void HandleCaching(IScopeContext scopeContext, IContent document, string
 
         if (draftUrlSegments.Any() is false)
         {
-            _logger.LogWarning("No draft URL segments found for document {DocumentKey} in culture {Culture}", document.Key, culture ?? "{null}");
+            // Log at debug level because this is expected when a document is not published in a given language.
+            _logger.LogDebug("No draft URL segments found for document {DocumentKey} in culture {Culture}", document.Key, culture ?? "{null}");
         }
         else
         {

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs

@@ -1155,25 +1155,38 @@ protected void ReplacePropertyValues(TEntity entity, int versionId, int publishe
 
             IEnumerable<PropertyDataDto> propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, publishedVersionId, entity.Properties, LanguageRepository, out edited, out editedCultures);
 
+            var toUpdate = new List<PropertyDataDto>();
+            var toInsert = new List<PropertyDataDto>();
             foreach (PropertyDataDto propertyDataDto in propertyDataDtos)
             {
                 // Check if this already exists and update, else insert a new one
                 if (propertyTypeToPropertyData.TryGetValue((propertyDataDto.PropertyTypeId, propertyDataDto.VersionId, propertyDataDto.LanguageId, propertyDataDto.Segment), out PropertyDataDto? propData))
                 {
                     propertyDataDto.Id = propData.Id;
-                    Database.Update(propertyDataDto);
+                    toUpdate.Add(propertyDataDto);
                 }
                 else
                 {
-                    // TODO: we can speed this up: Use BulkInsert and then do one SELECT to re-retrieve the property data inserted with assigned IDs.
-                    // This is a perfect thing to benchmark with Benchmark.NET to compare perf between Nuget releases.
-                    Database.Insert(propertyDataDto);
+                    toInsert.Add(propertyDataDto);
                 }
 
                 // track which ones have been processed
                 existingPropDataIds.Remove(propertyDataDto.Id);
             }
 
+            if (toUpdate.Count > 0)
+            {
+                var updateBatch = toUpdate
+                    .Select(x => UpdateBatch.For(x))
+                    .ToList();
+                Database.UpdateBatch(updateBatch, new BatchOptions { BatchSize = 100 });
+            }
+
+            if (toInsert.Count > 0)
+            {
+                Database.InsertBulk(toInsert);
+            }
+
             // For any remaining that haven't been processed they need to be deleted
             if (existingPropDataIds.Count > 0)
             {

src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs

@@ -54,13 +54,16 @@ protected override IDataValueEditor CreateValueEditor() =>
     /// </summary>
     internal sealed class MediaPicker3PropertyValueEditor : DataValueEditor, IDataValueReference
     {
+        private const string MediaCacheKeyFormat = nameof(MediaPicker3PropertyValueEditor) + "_Media_{0}";
+
         private readonly IDataTypeConfigurationCache _dataTypeReadCache;
         private readonly IJsonSerializer _jsonSerializer;
         private readonly IMediaImportService _mediaImportService;
         private readonly IMediaService _mediaService;
         private readonly ITemporaryFileService _temporaryFileService;
         private readonly IScopeProvider _scopeProvider;
         private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor;
+        private readonly AppCaches _appCaches;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="MediaPicker3PropertyValueEditor"/> class.
@@ -93,6 +96,8 @@ public MediaPicker3PropertyValueEditor(
             _scopeProvider = scopeProvider;
             _backOfficeSecurityAccessor = backOfficeSecurityAccessor;
             _dataTypeReadCache = dataTypeReadCache;
+            _appCaches = appCaches;
+
             var validators = new TypedJsonValidatorRunner<List<MediaWithCropsDto>, MediaPicker3Configuration>(
                 jsonSerializer,
                 new MinMaxValidator(localizedTextService),
@@ -203,21 +208,39 @@ private List<MediaWithCropsDto> UpdateMediaTypeAliases(List<MediaWithCropsDto> m
 
             foreach (MediaWithCropsDto mediaWithCropsDto in mediaWithCropsDtos)
             {
-                IMedia? media = _mediaService.GetById(mediaWithCropsDto.MediaKey);
+                IMedia? media = GetMediaById(mediaWithCropsDto.MediaKey);
                 mediaWithCropsDto.MediaTypeAlias = media?.ContentType.Alias ?? unknownMediaType;
             }
 
             return mediaWithCropsDtos.Where(m => m.MediaTypeAlias != unknownMediaType).ToList();
         }
 
+        private IMedia? GetMediaById(Guid key)
+        {
+            // Cache media lookups in case the same media is handled multiple times across a save operation,
+            // which is possible, particularly if we have multiple languages and blocks.
+            var cacheKey = string.Format(MediaCacheKeyFormat, key);
+            IMedia? media = _appCaches.RequestCache.GetCacheItem<IMedia?>(cacheKey);
+            if (media is null)
+            {
+                media = _mediaService.GetById(key);
+                if (media is not null)
+                {
+                    _appCaches.RequestCache.Set(cacheKey, media);
+                }
+            }
+
+            return media;
+        }
+
         private List<MediaWithCropsDto> HandleTemporaryMediaUploads(List<MediaWithCropsDto> mediaWithCropsDtos, MediaPicker3Configuration configuration)
         {
             var invalidDtos = new List<MediaWithCropsDto>();
 
             foreach (MediaWithCropsDto mediaWithCropsDto in mediaWithCropsDtos)
             {
                 // if the media already exist, don't bother with it
-                if (_mediaService.GetById(mediaWithCropsDto.MediaKey) != null)
+                if (GetMediaById(mediaWithCropsDto.MediaKey) != null)
                 {
                     continue;
                 }

tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentPublishingServiceTests.Publish.cs

@@ -3,6 +3,7 @@
 using Umbraco.Cms.Core.Models.ContentPublishing;
 using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Core.Services.OperationStatus;
+using Umbraco.Cms.Infrastructure.Persistence.Dtos;
 using Umbraco.Cms.Tests.Integration.Testing;
 
 namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services;
@@ -388,4 +389,31 @@ public async Task Republishing_Single_Culture_Does_Not_Change_Publish_Or_Update_
                                 ?? throw new InvalidOperationException("Expected a publish date for EN");
         Assert.Greater(lastPublishDateEn, firstPublishDateEn);
     }
+
+    [Test]
+    public async Task Publishing_Single_Culture_Persists_Expected_Property_Data()
+    {
+        var (langEn, langDa, langBe, contentType) = await SetupVariantDoctypeAsync();
+        var content = await CreateVariantContentAsync(langEn, langDa, langBe, contentType);
+
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            var dtos = scope.Database.Fetch<PropertyDataDto>();
+            Assert.AreEqual(18, dtos.Count);  // 3 properties * 3 cultures * 2 (published + edited).
+            scope.Complete();
+        }
+
+        var publishAttempt = await ContentPublishingService.PublishAsync(
+            content.Key,
+            [new() { Culture = langEn.IsoCode }],
+            Constants.Security.SuperUserKey);
+        Assert.IsTrue(publishAttempt.Success);
+
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            var dtos = scope.Database.Fetch<PropertyDataDto>();
+            Assert.AreEqual(19, dtos.Count); // + 3 for published populated title property, - 2 for existing published properties of other cultures.
+            scope.Complete();
+        }
+    }
 }

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoFetchTests.cs

@@ -1,13 +1,10 @@
 // Copyright (c) Umbraco.
 // See LICENSE for more details.
 
-using System.Collections.Generic;
-using System.Linq;
 using NPoco;
 using NUnit.Framework;
 using Umbraco.Cms.Tests.Common.Testing;
 using Umbraco.Cms.Tests.Integration.Testing;
-using Umbraco.Extensions;
 
 namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence.NPocoTests;
 

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoUpdateBatchTests.cs

@@ -0,0 +1,70 @@
+// Copyright (c) Umbraco.
+// See LICENSE for more details.
+
+using NPoco;
+using NUnit.Framework;
+using Umbraco.Cms.Infrastructure.Persistence.Dtos;
+using Umbraco.Cms.Tests.Common.Testing;
+using Umbraco.Cms.Tests.Integration.Testing;
+
+namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence.NPocoTests;
+
+[TestFixture]
+[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)]
+internal sealed class NPocoUpdateBatchTests : UmbracoIntegrationTest
+{
+    [Test]
+    public void Can_Update_Batch()
+    {
+        // Arrange
+        var servers = new List<ServerRegistrationDto>();
+        for (var i = 0; i < 3; i++)
+        {
+            servers.Add(new ServerRegistrationDto
+            {
+                Id = i + 1,
+                ServerAddress = "address" + i,
+                ServerIdentity = "computer" + i,
+                DateRegistered = DateTime.Now,
+                IsActive = false,
+                DateAccessed = DateTime.Now,
+            });
+        }
+
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            scope.Database.BulkInsertRecords(servers);
+            scope.Complete();
+        }
+
+        // Act
+        for (var i = 0; i < 3; i++)
+        {
+            servers[i].ServerAddress = "newaddress" + i;
+            servers[i].IsActive = true;
+        }
+
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            var updateBatch = servers
+                .Select(x => UpdateBatch.For(x))
+                .ToList();
+            var updated = scope.Database.UpdateBatch(updateBatch, new BatchOptions { BatchSize = 100 });
+            Assert.AreEqual(3, updated);
+            scope.Complete();
+        }
+
+        // Assert
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            var dtos = scope.Database.Fetch<ServerRegistrationDto>();
+            Assert.AreEqual(3, dtos.Count);
+            for (var i = 0; i < 3; i++)
+            {
+                Assert.AreEqual(servers[i].ServerAddress, dtos[i].ServerAddress);
+                Assert.AreEqual(servers[i].ServerIdentity, dtos[i].ServerIdentity);
+                Assert.AreEqual(servers[i].IsActive, dtos[i].IsActive);
+            }
+        }
+    }
+}