Performance: Cache published content instances at cache service level#20681
Conversation
AndyButland
left a comment
There was a problem hiding this comment.
Looks and works great. I've tested updates, publish, unpublish, move and delete and all works as expected. Checking with a version of your property editor also shows the expected outputs for the different levels of PropertyCacheLevel.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the published content caching implementation to move from request-level caching to element-level caching. The key objective is to ensure proper behavior of the PropertyCacheLevel enum values (None, Element, and Elements) in the hybrid cache system.
Key changes:
- Removed request cache (
AppCaches) dependency fromPublishedContentFactory - Added
ConcurrentDictionarycaching layer inDocumentCacheServiceandMediaCacheServiceto cache published content instances - Added comprehensive test coverage for property cache levels across documents, media, and members
- Removed
PublishedContentFactoryTeststhat were testing the old request cache behavior
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 30 comments.
Show a summary per file
| File | Description |
|---|---|
| PropertyCacheLevelTestsBase.cs | New base class providing test infrastructure with custom property value converter for testing cache level behavior |
| MemberPropertyCacheLevelTests.cs | Tests verifying property cache level behavior for member entities |
| MediaPropertyCacheLevelTests.cs | Tests verifying property cache level behavior for media entities including cache refresh scenarios |
| DocumentPropertyCacheLevelTests.cs | Tests verifying property cache level behavior for document entities in both published and preview modes |
| DocumentHybridCacheTests.cs | Updated test to support both preview and non-preview modes for unpublished content retrieval |
| MediaCacheService.cs | Added ConcurrentDictionary for caching published content instances and ensures cache invalidation |
| DocumentCacheService.cs | Added ConcurrentDictionary for caching published content instances and ensures cache invalidation |
| PublishedContentFactory.cs | Removed request cache dependency and simplified to return published content without caching |
| PublishedContentFactoryTests.cs | Deleted file - tests no longer relevant after removing request cache behavior |
| foreach (ContentCacheNode media in mediaCacheNodesByContentTypeKey) | ||
| { | ||
| _hybridCache.RemoveAsync(GetCacheKey(media.Key, false)); | ||
| ClearPublishedCacheAsync(media.Key).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Using .GetAwaiter().GetResult() in a synchronous method can cause deadlocks in certain contexts. Consider making the Rebuild method async or using a different synchronization approach.
| var publishedMember2 = await MemberCacheService.Get(member); | ||
| Assert.IsNotNull(publishedMember2); | ||
|
|
||
| Assert.AreNotSame(publishedMember1, publishedMember2); |
There was a problem hiding this comment.
[nitpick] There are extra spaces before publishedMember2. This appears in multiple test files and should use single spacing for consistency.
| Assert.AreNotSame(publishedMember1, publishedMember2); | |
| Assert.AreNotSame(publishedMember1, publishedMember2); |
| var publishedContent2 = await MediaCacheService.GetByKeyAsync(_mediaKey); | ||
| Assert.IsNotNull(publishedContent2); | ||
|
|
||
| Assert.AreSame(publishedContent1, publishedContent2); |
There was a problem hiding this comment.
[nitpick] There are extra spaces before publishedContent2. This appears in multiple test files and should use single spacing for consistency.
| Assert.AreSame(publishedContent1, publishedContent2); | |
| Assert.AreSame(publishedContent1, publishedContent2); |
|
|
||
| if (preview) | ||
| { | ||
| Assert.AreNotSame(publishedContent1, publishedContent2); |
There was a problem hiding this comment.
[nitpick] There are extra spaces before publishedContent2. This appears in multiple test files and should use single spacing for consistency.
| Assert.AreNotSame(publishedContent1, publishedContent2); | |
| Assert.AreNotSame(publishedContent1, publishedContent2); |
| [TestCase(false)] | ||
| public async Task Can_Get_Unpublished_Content_By_Key(bool preview) | ||
| { | ||
| // Arrange |
There was a problem hiding this comment.
Variable this.PublishedTextPage.Key may be null at this access because it has a nullable type.
| // Arrange | |
| // Arrange | |
| Assert.IsNotNull(PublishedTextPage, "PublishedTextPage should not be null"); | |
| Assert.IsNotNull(PublishedTextPage.Key, "PublishedTextPage.Key should not be null"); |
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
|
|
||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); |
There was a problem hiding this comment.
This assignment to titleValue1 is useless, since its value is never read.
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); |
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
|
|
||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); |
There was a problem hiding this comment.
This assignment to titleValue2 is useless, since its value is never read.
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); |
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); |
There was a problem hiding this comment.
This assignment to titleValue2 is useless, since its value is never read.
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); |
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
| titleValue1 = publishedMember1.Value<string>("title"); | ||
|
|
||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); |
There was a problem hiding this comment.
This assignment to titleValue2 is useless, since its value is never read.
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue1 = publishedMember1.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember1.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); |
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); | ||
| titleValue2 = publishedMember2.Value<string>("title"); |
There was a problem hiding this comment.
This assignment to titleValue2 is useless, since its value is never read.
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| titleValue2 = publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); | |
| publishedMember2.Value<string>("title"); |
Prerequisites
Description
At this point we recreate published content instances from their backing
ContentCacheNodecache (in the hybrid cache) every time they're requested. This means that property value converters utilizing theElementproperty value cache level are invoked over and over again, effectively making them behave as if they were using the property cache levelNone. They should only be invoked once per update of their containing content.The
Elementsproperty cache level works as expected, caching property values until any content update happens.This PR adds in-memory caching of all instantiated published content at cache service level, effectively retaining all applicable property values in cache once they've been constructed - subject of course to their property value cache level.
As part of this, the request caching added in
PublishedContentFactory(#19990) for published content instances has been rolled back, since the caching at cache service level serves the same purpose and is longer lived.Caching is memory intensive, and this PR can yield a lot of cached content for larger sites. This is consistent with the V13 behavior, but still perhaps something to reflect on - if not now, then later on. In an effort to minimize the memory consumption, I have opted to not cache draft content.
Benchmarks
I have benchmarked the changes locally using K6.
Delivery API
The most evident impact is seen when fetching a single piece of content using the Delivery API. Here I've measured a performance improvement starting around 30% and easily reaching 120% - the level of improvement is somewhat linear to the complexity of the content model.
Templated rendering
For templated rendering, the image becomes a little more blurred.
Besides fetching the requested content item for rendering, templated rendering also often entails fetching multiple other content items, for example to build navigation. This PR directly impacts all these operations in a positive way.
But... templated rendering involves a whole lot more, particularly when Razor starts building the output. These operations are not affected by this PR, and thus they make it harder to directly gauge the impact of the changes.
I have used a slightly modified version of the Clean starter kit to benchmark, and I've seen a 40-50% increase in throughput when rendering the front page (40% for the 90th percentile). Reach out if you want the test setup 😄
Testing this PR
First and foremost, verify that draft and published content is rendered and updated as per usual - in other words, that the new caching flushes things when it should (updates, deletes, moves).
Now test that the property cache level is respected for property value converters:
Element= keeps the property value in cache until the containing content is changed.Elements= keeps the property value in cache until any content is changed.None= does not cache the property value whatsoever.I have used the following custom property value converter to test all this. It replaces the core value converter for text strings.