Avoid a hash key generation and lookup when inserting in the LockingMechanism (#18243)

* Avoid a hash key generation and lookup when inserting in the LockingMechanism

* Added comments for CollectionsMarshal.GetValueRefOrAddDefault

* Added further comments and tests.

---------

Co-authored-by: Andy Butland <abutland73@gmail.com>

src/Umbraco.Core/Scoping/LockingMechanism.cs

@@ -1,3 +1,4 @@
+using System.Runtime.InteropServices;
 using System.Text;
 using Microsoft.Extensions.Logging;
 using Umbraco.Cms.Core.Collections;
@@ -7,7 +8,7 @@ using Umbraco.Extensions;
 namespace Umbraco.Cms.Core.Scoping;
 
 /// <summary>
-/// Mechanism for handling read and write locks
+/// Mechanism for handling read and write locks.
 /// </summary>
 public class LockingMechanism : ILockingMechanism
 {
@@ -189,24 +190,43 @@ public class LockingMechanism : ILockingMechanism
     /// <param name="lockId">Lock ID to increment.</param>
     /// <param name="instanceId">Instance ID of the scope requesting the lock.</param>
     /// <param name="locks">Reference to the dictionary to increment on</param>
-    private void IncrementLock(int lockId, Guid instanceId, ref Dictionary<Guid, Dictionary<int, int>>? locks)
+    /// <remarks>Internal for tests.</remarks>
+    internal static void IncrementLock(int lockId, Guid instanceId, ref Dictionary<Guid, Dictionary<int, int>>? locks)
     {
         // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again.
-        // If it's the very first time a lock has been requested the WriteLocks dict hasn't been instantiated yet.
-        locks ??= new Dictionary<Guid, Dictionary<int, int>>();
+        // If it's the very first time a lock has been requested the WriteLocks dictionary hasn't been instantiated yet.
+        locks ??= [];
 
-        // Try and get the dict associated with the scope id.
-        var locksDictFound = locks.TryGetValue(instanceId, out Dictionary<int, int>? locksDict);
+        // Try and get the dictionary associated with the scope id.
+
+        // The following code is a micro-optimization.
+        // GetValueRefOrAddDefault does lookup or creation with only one hash key generation, internal bucket lookup and value lookup in the bucket.
+        // This compares to doing it twice when initializing, one for the lookup and one for the insertion of the initial value, we had with the
+        // previous code:
+        //   var locksDictFound = locks.TryGetValue(instanceId, out Dictionary<int, int>? locksDict);
+        //   if (locksDictFound)
+        //   {
+        //       locksDict!.TryGetValue(lockId, out var value);
+        //       locksDict[lockId] = value + 1;
+        //   }
+        //   else
+        //   {
+        //       // The scope hasn't requested a lock yet, so we have to create a dict for it.
+        //       locks.Add(instanceId, new Dictionary<int, int>());
+        //       locks[instanceId][lockId] = 1;
+        //   }
+
+        ref Dictionary<int, int>? locksDict = ref CollectionsMarshal.GetValueRefOrAddDefault(locks, instanceId, out bool locksDictFound);
         if (locksDictFound)
         {
-            locksDict!.TryGetValue(lockId, out var value);
-            locksDict[lockId] = value + 1;
+            // By getting a reference to any existing or default 0 value, we can increment it without the expensive write back into the dictionary.
+            ref int value = ref CollectionsMarshal.GetValueRefOrAddDefault(locksDict!, lockId, out _);
+            value++;
         }
         else
         {
-            // The scope hasn't requested a lock yet, so we have to create a dict for it.
-            locks.Add(instanceId, new Dictionary<int, int>());
-            locks[instanceId][lockId] = 1;
+            // The scope hasn't requested a lock yet, so we have to create a dictionary for it.
+            locksDict = new Dictionary<int, int> { { lockId, 1 } };
         }
     }
 

tests/Umbraco.Tests.Common/Builders/ContentBuilder.cs

@@ -185,10 +185,7 @@ public class ContentBuilder
     {
         if (string.IsNullOrWhiteSpace(name))
         {
-            if (_cultureNames.TryGetValue(culture, out _))
-            {
-                _cultureNames.Remove(culture);
-            }
+            _cultureNames.Remove(culture);
         }
         else
         {

tests/Umbraco.Tests.UnitTests/Umbraco.Core/Scoping/LockingMechanismTests.cs

@@ -0,0 +1,45 @@
+// Copyright (c) Umbraco.
+// See LICENSE for more details.
+
+using NUnit.Framework;
+using Umbraco.Cms.Core.Scoping;
+
+namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Scoping;
+
+[TestFixture]
+internal class LockingMechanismTests
+{
+    private const int LockId = 1000;
+    private const int LockId2 = 1001;
+    private static readonly Guid _scopeInstanceId = Guid.NewGuid();
+
+    [Test]
+    public void IncrementLock_WithoutLocksDictionary_CreatesLock()
+    {
+        var locks = new Dictionary<Guid, Dictionary<int, int>>();
+        LockingMechanism.IncrementLock(LockId, _scopeInstanceId, ref locks);
+        Assert.AreEqual(1, locks.Count);
+        Assert.AreEqual(1, locks[_scopeInstanceId][LockId]);
+    }
+
+    [Test]
+    public void IncrementLock_WithExistingLocksDictionary_CreatesLock()
+    {
+        var locks = new Dictionary<Guid, Dictionary<int, int>>()
+        {
+            {
+                _scopeInstanceId,
+                new Dictionary<int, int>()
+                {
+                    { LockId, 100 },
+                    { LockId2, 200 }
+                }
+            }
+        };
+        LockingMechanism.IncrementLock(LockId, _scopeInstanceId, ref locks);
+        Assert.AreEqual(1, locks.Count);
+        Assert.AreEqual(2, locks[_scopeInstanceId].Count);
+        Assert.AreEqual(101, locks[_scopeInstanceId][LockId]);
+        Assert.AreEqual(200, locks[_scopeInstanceId][LockId2]);
+    }
+}