From 39362ffb3562cabe8a0bda7c1b03505bea4e943a Mon Sep 17 00:00:00 2001 From: Dmitry Bastron Date: Thu, 21 Mar 2024 15:04:21 +0100 Subject: [PATCH 1/3] Remove locks and switch to ConcurrentDictionary implementation. Original implementation was causing deadlocks when calling Content Delivery API in some cases. --- .../Property.cs | 129 ++++++++---------- 1 file changed, 56 insertions(+), 73 deletions(-) diff --git a/src/Umbraco.PublishedCache.NuCache/Property.cs b/src/Umbraco.PublishedCache.NuCache/Property.cs index 2892a04f9022..c42a616873b8 100644 --- a/src/Umbraco.PublishedCache.NuCache/Property.cs +++ b/src/Umbraco.PublishedCache.NuCache/Property.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Xml.Serialization; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Collections; @@ -19,7 +20,6 @@ internal class Property : PublishedPropertyBase private readonly bool _isMember; private readonly bool _isPreviewing; - private readonly object _locko = new(); private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor; // the invariant-neutral source and inter values @@ -33,7 +33,7 @@ internal class Property : PublishedPropertyBase private object? _interValue; // the variant source and inter values - private Dictionary? _sourceValues; + private ConcurrentDictionary? _sourceValues; private string? _valuesCacheKey; @@ -68,7 +68,7 @@ public Property( { if (_sourceValues == null) { - _sourceValues = new Dictionary(); + _sourceValues = new ConcurrentDictionary(); } _sourceValues[new CompositeStringStringKey(sourceValue.Culture, sourceValue.Segment)] @@ -125,30 +125,27 @@ public override bool HasValue(string? culture = null, string? segment = null) return hasValue.Value; } - lock (_locko) + value = GetInterValue(culture, segment); + hasValue = PropertyType.IsValue(value, PropertyValueLevel.Inter); + if (hasValue.HasValue) { - value = GetInterValue(culture, segment); - hasValue = PropertyType.IsValue(value, PropertyValueLevel.Inter); - if (hasValue.HasValue) - { - return hasValue.Value; - } - - CacheValue cacheValues = GetCacheValues(PropertyType.CacheLevel).For(culture, segment); + return hasValue.Value; + } - // initial reference cache level always is .Content - const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; + CacheValue cacheValues = GetCacheValues(PropertyType.CacheLevel).For(culture, segment); - if (!cacheValues.ObjectInitialized) - { - cacheValues.ObjectValue = - PropertyType.ConvertInterToObject(_content, initialCacheLevel, value, _isPreviewing); - cacheValues.ObjectInitialized = true; - } + // initial reference cache level always is .Content + const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; - value = cacheValues.ObjectValue; - return PropertyType.IsValue(value, PropertyValueLevel.Object) ?? false; + if (!cacheValues.ObjectInitialized) + { + cacheValues.ObjectValue = + PropertyType.ConvertInterToObject(_content, initialCacheLevel, value, _isPreviewing); + cacheValues.ObjectInitialized = true; } + + value = cacheValues.ObjectValue; + return PropertyType.IsValue(value, PropertyValueLevel.Object) ?? false; } public override object? GetSourceValue(string? culture = null, string? segment = null) @@ -160,19 +157,16 @@ public override bool HasValue(string? culture = null, string? segment = null) return _sourceValue; } - lock (_locko) + if (_sourceValues == null) { - if (_sourceValues == null) - { - return null; - } - - return _sourceValues.TryGetValue( - new CompositeStringStringKey(culture, segment), - out SourceInterValue? sourceValue) - ? sourceValue.SourceValue - : null; + return null; } + + return _sourceValues.TryGetValue( + new CompositeStringStringKey(culture, segment), + out SourceInterValue? sourceValue) + ? sourceValue.SourceValue + : null; } private CacheValues GetCacheValues(PropertyCacheLevel cacheLevel) @@ -227,7 +221,6 @@ private CacheValues GetCacheValues(IAppCache? cache) return (CacheValues)cache.Get(ValuesCacheKey, () => new CacheValues())!; } - // this is always invoked from within a lock, so does not require its own lock private object? GetInterValue(string? culture, string? segment) { if (culture == string.Empty && segment == string.Empty) @@ -244,7 +237,7 @@ private CacheValues GetCacheValues(IAppCache? cache) if (_sourceValues == null) { - _sourceValues = new Dictionary(); + _sourceValues = new ConcurrentDictionary(); } var k = new CompositeStringStringKey(culture, segment); @@ -273,23 +266,20 @@ private CacheValues GetCacheValues(IAppCache? cache) _content.VariationContextAccessor.ContextualizeVariation(_variations, _content.Id, ref culture, ref segment); object? value; - lock (_locko) - { - CacheValue cacheValues = GetCacheValues(PropertyType.CacheLevel).For(culture, segment); - - // initial reference cache level always is .Content - const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; + CacheValue cacheValues = GetCacheValues(PropertyType.CacheLevel).For(culture, segment); - if (cacheValues.ObjectInitialized) - { - return cacheValues.ObjectValue; - } + // initial reference cache level always is .Content + const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; - cacheValues.ObjectValue = PropertyType.ConvertInterToObject(_content, initialCacheLevel, GetInterValue(culture, segment), _isPreviewing); - cacheValues.ObjectInitialized = true; - value = cacheValues.ObjectValue; + if (cacheValues.ObjectInitialized) + { + return cacheValues.ObjectValue; } + cacheValues.ObjectValue = PropertyType.ConvertInterToObject(_content, initialCacheLevel, GetInterValue(culture, segment), _isPreviewing); + cacheValues.ObjectInitialized = true; + value = cacheValues.ObjectValue; + return value; } @@ -298,22 +288,19 @@ private CacheValues GetCacheValues(IAppCache? cache) { _content.VariationContextAccessor.ContextualizeVariation(_variations, _content.Id, ref culture, ref segment); - lock (_locko) - { - CacheValue cacheValues = GetCacheValues(PropertyType.CacheLevel).For(culture, segment); + CacheValue cacheValues = GetCacheValues(PropertyType.CacheLevel).For(culture, segment); - // initial reference cache level always is .Content - const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; + // initial reference cache level always is .Content + const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; - if (cacheValues.XPathInitialized) - { - return cacheValues.XPathValue; - } - - cacheValues.XPathValue = PropertyType.ConvertInterToXPath(_content, initialCacheLevel, GetInterValue(culture, segment), _isPreviewing); - cacheValues.XPathInitialized = true; + if (cacheValues.XPathInitialized) + { return cacheValues.XPathValue; } + + cacheValues.XPathValue = PropertyType.ConvertInterToXPath(_content, initialCacheLevel, GetInterValue(culture, segment), _isPreviewing); + cacheValues.XPathInitialized = true; + return cacheValues.XPathValue; } public override object? GetDeliveryApiValue(bool expanding, string? culture = null, string? segment = null) @@ -321,18 +308,15 @@ private CacheValues GetCacheValues(IAppCache? cache) _content.VariationContextAccessor.ContextualizeVariation(_variations, _content.Id, ref culture, ref segment); object? value; - lock (_locko) - { - CacheValue cacheValues = GetCacheValues(expanding ? PropertyType.DeliveryApiCacheLevelForExpansion : PropertyType.DeliveryApiCacheLevel).For(culture, segment); + CacheValue cacheValues = GetCacheValues(expanding ? PropertyType.DeliveryApiCacheLevelForExpansion : PropertyType.DeliveryApiCacheLevel).For(culture, segment); - // initial reference cache level always is .Content - const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; + // initial reference cache level always is .Content + const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element; - object? GetDeliveryApiObject() => PropertyType.ConvertInterToDeliveryApiObject(_content, initialCacheLevel, GetInterValue(culture, segment), _isPreviewing, expanding); - value = expanding - ? GetDeliveryApiExpandedObject(cacheValues, GetDeliveryApiObject) - : GetDeliveryApiDefaultObject(cacheValues, GetDeliveryApiObject); - } + object? GetDeliveryApiObject() => PropertyType.ConvertInterToDeliveryApiObject(_content, initialCacheLevel, GetInterValue(culture, segment), _isPreviewing, expanding); + value = expanding + ? GetDeliveryApiExpandedObject(cacheValues, GetDeliveryApiObject) + : GetDeliveryApiDefaultObject(cacheValues, GetDeliveryApiObject); return value; } @@ -382,9 +366,8 @@ private class CacheValue private class CacheValues : CacheValue { - private Dictionary? _values; + private ConcurrentDictionary? _values; - // this is always invoked from within a lock, so does not require its own lock public CacheValue For(string? culture, string? segment) { if (culture == string.Empty && segment == string.Empty) @@ -394,7 +377,7 @@ public CacheValue For(string? culture, string? segment) if (_values == null) { - _values = new Dictionary(); + _values = new ConcurrentDictionary(); } var k = new CompositeStringStringKey(culture, segment); From 814864e82546d1986b81b555499c78417dc1f824 Mon Sep 17 00:00:00 2001 From: Jason Elkin Date: Tue, 2 Apr 2024 23:18:01 +0100 Subject: [PATCH 2/3] Concurrent dictionary performance improvements --- .../Property.cs | 50 +++++-------------- 1 file changed, 13 insertions(+), 37 deletions(-) diff --git a/src/Umbraco.PublishedCache.NuCache/Property.cs b/src/Umbraco.PublishedCache.NuCache/Property.cs index c42a616873b8..ec5ecf33eaf7 100644 --- a/src/Umbraco.PublishedCache.NuCache/Property.cs +++ b/src/Umbraco.PublishedCache.NuCache/Property.cs @@ -33,7 +33,7 @@ internal class Property : PublishedPropertyBase private object? _interValue; // the variant source and inter values - private ConcurrentDictionary? _sourceValues; + private readonly Lazy> _sourceValues = new(); private string? _valuesCacheKey; @@ -56,7 +56,7 @@ public Property( PropertyCacheLevel referenceCacheLevel = PropertyCacheLevel.Element) : base(propertyType, referenceCacheLevel) { - if (sourceValues != null) + if (sourceValues is not null) { foreach (PropertyData sourceValue in sourceValues) { @@ -66,12 +66,7 @@ public Property( } else { - if (_sourceValues == null) - { - _sourceValues = new ConcurrentDictionary(); - } - - _sourceValues[new CompositeStringStringKey(sourceValue.Culture, sourceValue.Segment)] + _sourceValues.Value[new CompositeStringStringKey(sourceValue.Culture, sourceValue.Segment)] = new SourceInterValue { Culture = sourceValue.Culture, @@ -157,12 +152,7 @@ public override bool HasValue(string? culture = null, string? segment = null) return _sourceValue; } - if (_sourceValues == null) - { - return null; - } - - return _sourceValues.TryGetValue( + return _sourceValues.Value.TryGetValue( new CompositeStringStringKey(culture, segment), out SourceInterValue? sourceValue) ? sourceValue.SourceValue @@ -235,21 +225,14 @@ private CacheValues GetCacheValues(IAppCache? cache) return _interValue; } - if (_sourceValues == null) - { - _sourceValues = new ConcurrentDictionary(); - } - var k = new CompositeStringStringKey(culture, segment); - if (!_sourceValues.TryGetValue(k, out SourceInterValue? vvalue)) + + SourceInterValue vvalue = _sourceValues.Value.GetOrAdd(k, _ => new SourceInterValue { - _sourceValues[k] = vvalue = new SourceInterValue - { - Culture = culture, - Segment = segment, - SourceValue = GetSourceValue(culture, segment), - }; - } + Culture = culture, + Segment = segment, + SourceValue = GetSourceValue(culture, segment), + }); if (vvalue.InterInitialized) { @@ -366,7 +349,7 @@ private class CacheValue private class CacheValues : CacheValue { - private ConcurrentDictionary? _values; + private readonly Lazy> _values = new(); public CacheValue For(string? culture, string? segment) { @@ -375,16 +358,9 @@ public CacheValue For(string? culture, string? segment) return this; } - if (_values == null) - { - _values = new ConcurrentDictionary(); - } - var k = new CompositeStringStringKey(culture, segment); - if (!_values.TryGetValue(k, out CacheValue? value)) - { - _values[k] = value = new CacheValue(); - } + + CacheValue value = _values.Value.GetOrAdd(k, _ => new CacheValue()); return value; } From 6f7210cb34b2456aa7dce93cc42cd53be4e12866 Mon Sep 17 00:00:00 2001 From: Jason Elkin Date: Mon, 8 Apr 2024 14:25:49 +0100 Subject: [PATCH 3/3] Set default size of concurrent dictionary to 5 --- src/Umbraco.PublishedCache.NuCache/Property.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.PublishedCache.NuCache/Property.cs b/src/Umbraco.PublishedCache.NuCache/Property.cs index ec5ecf33eaf7..e8fd9054f8d8 100644 --- a/src/Umbraco.PublishedCache.NuCache/Property.cs +++ b/src/Umbraco.PublishedCache.NuCache/Property.cs @@ -33,7 +33,7 @@ internal class Property : PublishedPropertyBase private object? _interValue; // the variant source and inter values - private readonly Lazy> _sourceValues = new(); + private readonly Lazy> _sourceValues = new(() => new(-1, 5)); private string? _valuesCacheKey; @@ -349,7 +349,7 @@ private class CacheValue private class CacheValues : CacheValue { - private readonly Lazy> _values = new(); + private readonly Lazy> _values = new(() => new(-1, 5)); public CacheValue For(string? culture, string? segment) {