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 all 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
140 changes: 51 additions & 89 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;

Check notice on line 1 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: Overall Code Complexity 

The mean cyclomatic complexity in this module is no longer above the threshold
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(() => new(-1, 5));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be set depending on the expected number of entries, what's common?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Nuklon,

This SO sums up the choice here...

If you want to

  • optimize speed: take an initial capacity which is a prime about 30% bigger than the expected maximum dictionary size. This avoids resizing.
  • optimize the memory footprint: take a prime which is about 30% bigger than the minimum expected size.
  • a balance between speed and memory footprint: Take a number in between the two from above. But in any case, take a prime.

I'll double check the most common number, but it's _low, as it's effectively number of languages x variations so 5 seemed pretty sensible here.


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);
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.ObjectInitialized)
{
return cacheValues.ObjectValue;
}

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,18 +271,15 @@
_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 @@ -359,26 +329,18 @@

private class CacheValues : CacheValue
{
private Dictionary<CompositeStringStringKey, CacheValue>? _values;
private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, CacheValue>> _values = new(() => new(-1, 5));

// 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