We have moved to GitHub Issues
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.
In the source code I notice few strange things (IMHO)
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)
You use values as part of the cache key which looks ok, but GetPreValuesCollectionByDataTypeId method use the key WITHOUT values
var cached = RuntimeCache.GetCacheItemsByKeySearch
Looking forward for your answer
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.
More notes: would need to benchmark doing the RegEx search on cache keys (expensive) vs retrieving the collections and directly looking at their content.
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.
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
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.
@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.
@zpqrtbnk, @Shandem Great job, thanks.
Backwards Compatible: True
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