Ported over #15928 changes for 13.3 RC

[kjac]

Prerequisites

• I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #15827

Description

Due to some V14 branch merges, #15928 is unable to target V13.3, so I have moved over the changes to this PR. See #15928 for more details. All credits go to the original contributors ❤️

For the time being I have not moved over the Lazy dictionary changes, because I wanted to see how the other changes worked without Lazy. It seems to work just as fine. Please comment if they should be re-introduced.

Here's the K6 script execution (from #15827) on this branch:

The first execution is without this PR applied, second is with this PR applied.

[JasonElkin]

Thanks @kjac,

The reason I added lazy was to to make the dictionary creation thread safe. The simple null check can result in race conditions that will wipe out the dictionary unnecessarily. Also, Lazy dries up the initialization code for the dictionary, which doesn't need to happen in two places.

Currently, all the interaction with the dictionary is idempotent so nothing breaks, but it doesn't have to be - this could blow up in future and cause side effects if the dictionary is used differently.

[JasonElkin]

@kjac, @bergmania I've put back the GetOrAdd() call that I added in the previous PR as it will avoid extra writes to the dictionary if two (or more) threads try and add a non-existent entry at the same time.

[JasonElkin]

One other thing I've noticed...

Does the Dictionary ever actually get instantiated here?

Umbraco-CMS/src/Umbraco.PublishedCache.NuCache/Property.cs

Lines 226 to 241 in c142427

	if (culture == string.Empty && segment == string.Empty)
	{
	if (_interInitialized)
	{
	return _interValue;
	}
	_interValue = PropertyType.ConvertSourceToInter(_content, _sourceValue, _isPreviewing);
	_interInitialized = true;
	return _interValue;
	}
	if (_sourceValues == null)
	{
	_sourceValues = InitializeConcurrentDictionary<CompositeStringStringKey, SourceInterValue>();
	}

When in the constructor, the same condition determines if the dictionary is instantiated?

Umbraco-CMS/src/Umbraco.PublishedCache.NuCache/Property.cs

Lines 63 to 72 in c142427

	if (sourceValue.Culture == string.Empty && sourceValue.Segment == string.Empty)
	{
	_sourceValue = sourceValue.Value;
	}
	else
	{
	if (_sourceValues == null)
	{
	_sourceValues = InitializeConcurrentDictionary<CompositeStringStringKey, SourceInterValue>();
	}

In local testing (albeit with a very limited amount of content and properties) L240 is never hit..

[Nuklon]

Is it ever possible to get here in a state where a null _sourceValues is useful here? This piece of code is very badly documented, but GetSourceValue seems to return null for SourceValue always when there are no source values passed in ctor?

[Nuklon]

Suggested change

	private readonly object _locko = new();
	private ConcurrentDictionary<CompositeStringStringKey, CacheValue>? _values;
	private ConcurrentDictionary<CompositeStringStringKey, CacheValue>? _values = InitializeConcurrentDictionary<CompositeStringStringKey, CacheValue>();

[kjac]

Hi @Nuklon,

We shouldn't allocate the dictionary up front. It does not need allocating for non-variant properties:

[Nuklon]

Hi @Nuklon,

We shouldn't allocate the dictionary up front. It does not need allocating for non-variant properties:

Hmm, I see. That can be checked with _sourceValueIsInvariant I think? If you add ctor with isInvariant parameter, and create dictionary then if necessary?

[Nuklon]

Suggested change

	lock (_locko)
	{
	_values ??= InitializeConcurrentDictionary<CompositeStringStringKey, CacheValue>();
	}

[diger74]

Hi guys, thanks for porting this fix to v13, I appreciate it!
Would it be possible to add it to v12 as well, please?