-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add request caching around published content factory #19990
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.
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
Add request caching around published content factory #19990
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements request-level caching for the PublishedContentFactory
to improve performance by avoiding repeated instantiation of the same IPublishedContent
objects within a single request. The cache reduces content instantiation from 35 to 9 instances when loading a product page in the Umbraco starter kit.
- Adds request caching to
ToIPublishedContent
,ToIPublishedMedia
, andToPublishedMember
methods - Introduces comprehensive integration tests to verify caching behavior
- Adds XML documentation to improve code maintainability
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
File | Description |
---|---|
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs |
Implements request caching logic and adds comprehensive logging and documentation |
src/Umbraco.PublishedCache.HybridCache/Factories/IPublishedContentFactory.cs |
Adds XML documentation for the interface methods |
tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactoryTests.cs |
New integration test file verifying caching behavior for all factory methods |
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Factories/PublishedContentFactory.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.PublishedCache.HybridCache/Factories/IPublishedContentFactory.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Looks good, tests good 👍 I Inverted the if statements to make the code more readable 😄 |
Prerequisites
Relates to #18892
Description
We've had some discussion on the linked issue and internally about the potential performance concern of having to translate from the object stored into the hybrid cache and the
IPublishedContent
instance. It can be argued if this is likely to be a concern in typical page generation, but, as noted, it seems straightforward to at least request cache this.As such I've applied that in this PR.
Given it's only cached for the request, we don't have to be concerned about cache invalidation.
Looking at a small site - the Umbraco starter kit - it does seem we can instantiate the same
IPublishedContent
multiple times in a single page request.When this update is applied, loading a product page, which has the navigation bar, from the starter kit creates 9
IPublishedContent
instances. Without the request cache, it creates 35.And so this should have a positive, even if often small or even negligible, impact - and I can't see it can cause any harm.
Testing
Verify pages render from the cache as expected.
Using a breakpoint in
PublishedContentFactory
verify that the items are retrieved from the cache on all but the first time for a page request.