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.

Sign up for GitHub

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

Jump to bottom

Remove locks and switch to ConcurrentDictionary implementation. #15928

Closed
wants to merge 4 commits into from
Closed

Remove locks and switch to ConcurrentDictionary implementation. #15928

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39362ff
Remove locks and switch to ConcurrentDictionary implementation. Origi…
Mar 21, 2024
814864e
Concurrent dictionary performance improvements
JasonElkin Apr 2, 2024
6f7210c
Set default size of concurrent dictionary to 5
JasonElkin Apr 8, 2024
abe7238
Merge branch 'contrib' into pr/15928
nul800sebastiaan Apr 9, 2024
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
161 changes: 60 additions & 101 deletions src/Umbraco.PublishedCache.NuCache/Property.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Collections.Concurrent;
using System.Xml.Serialization;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Collections;
Expand All @@ -19,7 +20,6 @@
private readonly bool _isMember;
private readonly bool _isPreviewing;

private readonly object _locko = new();
private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor;

// the invariant-neutral source and inter values
Expand All @@ -33,7 +33,7 @@
private object? _interValue;

// the variant source and inter values
private Dictionary<CompositeStringStringKey, SourceInterValue>? _sourceValues;
private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, SourceInterValue>> _sourceValues = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just create the dictionary and avoid Lazy completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, if creating every time is more efficient overall then happy to go with that - but keen to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Lazy has historically caused a lot of trouble with disposal and memory leakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Come to think of it, the _sourceValues dictionary is only created for variant properties. In a LOT of cases, the property is not going to be variant, so the up-front allocation of a dictionary is an overhead compared to today.

Perhaps the Lazy approach is justified here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just browsing a bit through the code it seems this _sourceValues is used quite a lot (either through HasValue or GetSourceValue).


private string? _valuesCacheKey;

Expand All @@ -56,22 +56,17 @@
PropertyCacheLevel referenceCacheLevel = PropertyCacheLevel.Element)
: base(propertyType, referenceCacheLevel)
{
if (sourceValues != null)
if (sourceValues is not null)
{
foreach (PropertyData sourceValue in sourceValues)
{
if (sourceValue.Culture == string.Empty && sourceValue.Segment == string.Empty)
{
_sourceValue = sourceValue.Value;
}
else
{
if (_sourceValues == null)
{
_sourceValues = new Dictionary<CompositeStringStringKey, SourceInterValue>();
}

_sourceValues[new CompositeStringStringKey(sourceValue.Culture, sourceValue.Segment)]
_sourceValues.Value[new CompositeStringStringKey(sourceValue.Culture, sourceValue.Segment)]

Check notice on line 69 in src/Umbraco.PublishedCache.NuCache/Property.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (contrib)

✅ No longer an issue: Bumpy Road Ahead 

Property is no longer above the threshold for logical blocks with deeply nested code

Check notice on line 69 in src/Umbraco.PublishedCache.NuCache/Property.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (contrib)

✅ No longer an issue: Deep, Nested Complexity 

Property is no longer above the threshold for nested complexity depth
= new SourceInterValue
{
Culture = sourceValue.Culture,
Expand Down Expand Up @@ -125,30 +120,27 @@
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)
Expand All @@ -160,19 +152,11 @@
return _sourceValue;
}

lock (_locko)
{
if (_sourceValues == null)
{
return null;
}

return _sourceValues.TryGetValue(
new CompositeStringStringKey(culture, segment),
out SourceInterValue? sourceValue)
? sourceValue.SourceValue
: null;
}
return _sourceValues.Value.TryGetValue(
new CompositeStringStringKey(culture, segment),
out SourceInterValue? sourceValue)
? sourceValue.SourceValue
: null;
}

private CacheValues GetCacheValues(PropertyCacheLevel cacheLevel)
Expand Down Expand Up @@ -227,7 +211,6 @@
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)
Expand All @@ -242,21 +225,14 @@
return _interValue;
}

if (_sourceValues == null)
{
_sourceValues = new Dictionary<CompositeStringStringKey, SourceInterValue>();
}

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)
{
Expand All @@ -273,23 +249,20 @@
_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;
}

Expand All @@ -298,41 +271,35 @@
{
_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;

if (cacheValues.XPathInitialized)
{
return cacheValues.XPathValue;
}
// initial reference cache level always is .Content
const PropertyCacheLevel initialCacheLevel = PropertyCacheLevel.Element;

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)
{
_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;
}
Expand Down Expand Up @@ -382,26 +349,18 @@

private class CacheValues : CacheValue
{
private Dictionary<CompositeStringStringKey, CacheValue>? _values;
private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, CacheValue>> _values = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Lazy shouldn't be necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Nuklon, this is something I was wondering about.

Anecdotally, always creating looks to be faster, but I'm not sure of the memory implications so popped it in a Lazy (just so then we're creating the same number of dictionaries that we were before).


// 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)
{
return this;
}

if (_values == null)
{
_values = new Dictionary<CompositeStringStringKey, CacheValue>();
}

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;
}
Expand Down
Loading