U4-9623 - DataTypeDefinitionRepository caches for prevalues aren't cached properly

Created by alex_training 14 Mar 2017, 03:36:38 Updated by alex_training 30 Mar 2017, 20:50:16

Relates to: U4-9682

Subtask of: U4-9609

In code I use UmbracoMapper package to map a media to my model _mapper.Map(media, model). Recently I found that on every request Umbraco goes to DB (instead of using cache) to get "crops" property.

Below you can see a miniprofile trace log and actual sql query.

b__3 ExecuteAction ExecuteReaderWithRetry ExecuteReaderWithRetry ExecuteReaderWithRetry MoveNext Fetch GetAndCachePreValueCollection GetPreValuesCollectionByDataTypeId GetPreValuesCollectionByDataTypeId MergePreValues ConvertDataToSource ConvertDataToSource ConvertDataToSource CreateValue LazyInitValue <.ctor>b__1 CreateValue LazyInitValue get_Url InvokeMethod UnsafeInvokeInternal Invoke GetValue MapNativeContentProperty MapContentProperty Map DECLARE @0 int = 1043; SELECT [cmsDataTypePreValues].[id], [cmsDataTypePreValues].[datatypeNodeId], [cmsDataTypePreValues].[value], [cmsDataTypePreValues].[sortorder], [cmsDataTypePreValues].[alias] FROM [cmsDataTypePreValues] WHERE datatypeNodeId = @0 }

In the source code I notice few strange things (IMHO)

  1. https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs#L468 //30 mins new TimeSpan(0, 0, 30) it initiates 30 seconds (NOT minutes) cache

https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs#L462 var key = GetPrefixedCacheKey(dataTypeId)

  • string.Join(",", collection.FormatAsDictionary().Select(x => x.Value.Id).ToArray());

You use values as part of the cache key which looks ok, but GetPreValuesCollectionByDataTypeId method use the key WITHOUT values https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs#L279 var cached = RuntimeCache.GetCacheItemsByKeySearch(GetPrefixedCacheKey(dataTypeId));

Looking forward for your answer

1 Attachments

Comments

Stephan 14 Mar 2017, 09:58:05

About (1) there's definitively a mismatch between the comment and the code. Need to fix.

About (2) GetCacheItemsByKeySearch searches for keys that start with the value, so it should work

But the whole thing looks... not really optimized.


Stephan 14 Mar 2017, 10:17:26

More notes: would need to benchmark doing the RegEx search on cache keys (expensive) vs retrieving the collections and directly looking at their content.


alex_training 14 Mar 2017, 16:04:21

Hi @zpqrtbnk (About 2) You're right, it looks for the value using "keyStart".

Another thing that the value is cached with "sliding" expiration flag RuntimeCache.InsertCacheItem(key, func, sliding: true);

Due to my tests it doesn't expire properly, that's why I see each 30 seconds a new request to DB, even if I have a requests each 5 seconds.


Stephan 23 Mar 2017, 09:42:51

PR https://github.com/umbraco/Umbraco-CMS/pull/1819

what I've done: properly use the runtime cache so that the delays, timeouts and slidings all work correctly. this can be reviewed: create a page that renders the prevalues for a datatype, then refresh the page, should not see SQL for prevalues.

expiration is set to 20 mins sliding, so you may want to change it (in DataTypeDefinitionRepository.GetCachedPreValueCollection) to something slower to see it expire.

also, have changed the way we cache things, previously we'd use long keys then do regex matching on keys, which makes it complicated to put things in cache (since the key depends on the value) plus regex are not optimal, perf-wise - now, we just loop over collections to find the proper value


Shannon Deminick 28 Mar 2017, 04:18:59

I had a breakpoint set and have been trying to figure out why the PreValueCollection gets deep cloned so many times when we want to retrieve it... like 5 or more times!?

I mentioned on the PR that we are calling DeepClone manually which is not needed because the cache provider is already a deep clone provider, so we can definitely reduce it by one but then it was still doing a ton of cloning. See attached screen shot:

We've somehow ended up having nested DeepCloneRuntimeCacheProviders as the runtime provider.... Wat?! I'll see where this went wrong and if it's the case for all repo's that will double cloning performance to fix this.


Shannon Deminick 28 Mar 2017, 04:46:41

@zpqrtbnk I've pushed changes to fix the above (http://issues.umbraco.org/issue/U4-9682), please have a look at the changes I've made, if you are happy with those I'm happy with your changes so you can merge and close these two.


Stephan 28 Mar 2017, 15:36:35

Happy.


alex_training 30 Mar 2017, 20:50:16

@zpqrtbnk, @Shandem Great job, thanks.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.4.3, 7.5.1, 7.5.2, 7.5.3, 7.5.4, 7.5.5, 7.5.6, 7.5.7, 7.5.8, 7.5.9, 7.5.10

Due in version: 7.5.12

Sprint: Sprint 55

Story Points: 1

Cycle: