silverstripe-elemental: Add helper methods for ElementalArea cache invalidation
Problem statement
Implementing caching strategies for a page containing an ElementalArea is hard.
Description
The key problem is we have no API providing any info from blocks within the area if they can be cached or not. If we loop through the elements manually and check them for cacheability, we cannot do that in a generic way, since BaseElement has no API for providing any info about its cacheability.
Proposal
We introduce several cache invalidation helper methods to ElementalArea and BaseElement.
public ElementalArea::isCacheable() : bool
public ElementalArea::getCacheKey() : string
public BaseElement::isCacheable() : bool
public BaseElement::getCacheKey() : string
Both isCacheable
and getCacheKey
are context sensitive and should be calculated in runtime (non-static implementations) depending on the particular data rendered and business-logic in the module.
To maintain backward compatibility, by default isCacheable
would return false
and getCacheKey
would return a random value. Any block that may be cached should provide its own implementations for both methods.
Since both methods may add some overhead (e.g. extra SQL queries) on every request, I suggest we leave them opt-in
. That means we do not implement any default OOTB caching. Instead we should provide the documentation necessary to implement proper caching, utilising the API suggested.
PRs
Discussion points
Pros:
- Simple and generic (2 methods with 10 and 7 lines of code accordingly)
- Applicable for at least template partial caching and HTTP caching. May or may not be useful for other caching types and strategies.
- Introduces conventions for implementing caching in blocks consistently (atm every block has to have its own adhoc APIs for caching and cache invalidation)
- Gives people an actual working feature, ready for use and with safe defaults (can be used without too much thinking about how caching works)
Cons
- Extra feature to maintain
- The API doesn’t do much on its own. Still have to implement caching in every block to have any performance benefits
- The API mingles with model, lacking separate clear interfaces (no separation of concerns). Introducing separate caching components makes little sense in this module (that ideally should be in the framework, but this is not feasible)
- If used without thinking about how caching works, may give people wrong expectations (e.g. wouldn’t cache anything sometimes and may even reduce performance in some edge cases)
Alternative solutions mentioned
Document this API instead of actually adding it
Can even go as far as doing all the code that you would PR to this repository, but then converting it to docs.
Pros:
- No need to maintain new APIs
- Responsibility for such implementation is on users, not maintainers
Cons:
- Won’t give users a painless way to cache elemental blocks
- We would still have to maintain the documentation (documentation complexity increases)
- We still won’t be able to implement caching in blocks consistently
Implement more generic API in the framework
Such a generic feature would be supported by other framework components (e.g. SSViewer) and would allow more transparent caching and cache invalidation.
Pros:
- More generic and flexible
- Would allow integration with other framework components (e.g. View, Middlewares)
Cons:
- Much more complex to implement. Requires much more investigation, planning and discussions
- May break BC, so we’ll have to wait next major release (CMS 5) to ship that
- For blocks it’s not much different with what exact higher level API to reconcile its caching implementation. To have any actual performance benefits, the heavy-lifting still has to be implemented in the blocks.
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 23 (22 by maintainers)
Discussion points
section in the head of this issue summarizes the points from the discussion above.@silverstripe/core-team please cast your vote by emojis on this post (this is the
Option I
from the issue 83)P.S. looks like the team invitation doesn’t work here because the team is from another org. As a workaround I’m inviting you all personally: @sminnee @wilr @chillu @clarkepaul @dhensby @unclecheese @flamerohr @maxime-rainville @kinglozzer @stevie-mayhew @ScopeyNZ @robbieaverill
Option A (😄) : Already good enough (do nothing, leave as is)
Option B (🎉) : Documentation only
Option C (🚀) : Ship the new API
Option D (👀) : Let us find another way
Option A
Thanks @ChrissiQ . The documentation that we’re planning to add will be on framework, but we can follow up with a mention of those new docs in this module too.
Here’s the issue that tracks the docs being added to framework FYI: https://github.com/silverstripe/silverstripe-framework/issues/9508
Hey guys, so I’m the one who submitted the issue that sparked this one. The problem we had was that it was non-obvious that we should not cache
$ElementalArea
, so we did. All one has to do is install https://github.com/dnadesign/silverstripe-elemental-userforms while having cached elemental areas and immediately you’ve got the potential for catastrophe.I’m perfectly happy with option A (obviously it’d be great to have an API for elemental block cache-ability, but it’s not strictly necessary), but the documentation will have to be prominent enough so that devs who are wondering about security/caching elemental blocks will be sure to see it. I think it’s very easy for devs to miss the issue - for one thing, we assume that installed modules are going to work properly and we don’t see any warnings about this when implementing caching or adding elemental blocks. Good documentation will be plenty good enough though.
Thanks for the response to the issue, I’m glad it’s getting looked at.
I’m going for option A, because IMO the generation of complex cache keys is where core partial caching falls down. E.g. how do you efficiently generate a cache key that will change when a
many_many
relation has been re-ordered? Even assuming a re-order updates theLastEdited
date of related items, it’s still an unnecessary database query to get that value.Personally, I’ve long since given up on partial caching and instead use https://github.com/heyday/silverstripe-cacheinclude, which invalidates caches on DataObject write instead.