Problem/Motivation
Quoting the most relevant part of #2329101-21: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations:
Now, when we render a block, the things it must be varied by (language & theme) "infect" the parents/containers of the block too: regions in the page, and the page itself. In other words: if a block must be varied by theme and language, then so must the region that contains it, and so must the page.
In other words, we need cache contexts to bubble.
Proposed resolution
Bubble cache contexts:
- add them to
BubbleableMetadata
- also make every creator of
BubbleableMetadata
set contexts, if they have any. This includesFilterProcessResult
. - … which means we can now also easily make text format filters vary their filtering based on the contexts they associate. Prime example: language-dependent filtering.
Remaining tasks
Review.
User interface changes
None.
API changes
- Addition: cache contexts are now bubbled automatically. Where necessary, a redirecting cache item is created. This does not break any APIs.
- Addition:
Cache::mergeContexts()
(analogous toCache::mergeTags()
- Addition:
BubbleableMetadata
now handles cache contexts also. - Addition:
FilterProcessResult
gainsgetCacheContexts()
,addCacheContexts()
andsetCacheContexts()
methods, symmetrical with the existing methods.
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff.txt | 5.65 KB | effulgentsia |
#76 | bubble_cache_contexts-2429257-76.patch | 64.67 KB | effulgentsia |
#74 | bubble_cache_contexts-2429257-74.patch | 61.1 KB | Wim Leers |
#18 | smartcache-plus-poc.txt | 1.91 KB | Fabianx |
#11 | interdiff-fabianx.txt | 6.68 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim LeersFixed omission in IS.
I already have a patch, will post soon.
Comment #3
Wim LeersComment #4
Wim LeersIs rolled on top of #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations.
Comment #5
Fabianx CreditAttribution: Fabianx commentedc&p fail - cacheContexts
nit - missing space
Overall looks really good, we need another case though.
While we achieve correctness through the bubbling, the following case leads to a cache written that can never be read:
- in hook_entity_view(), add $build['#cache']['contexts'][] = 'user';
now all bubbled up cache IDs change.
But we must never store a cache ID, where we can't find it.
Proposed resolution:
Store the original cache ID we attempted the cache_get with in Renderer->cacheGet() and give it to Renderer->cacheSet() as third argument.
And if cid != original_cid return without storing.
Comment #6
catchCache contexts aren't really bubbleable in the way cache tags and attached stuff is, the bubbled information isn't available where the cache ID is created.
I can see three ways around this:
1. What I think Fabianx is suggesting in #5 - two cache gets, one returns the actual cache ID, the second returns the cached data itself (similar to menu tree caching in 7.x). This relies on nested rendered items never returning different cache contexts though. For example two different entity displays for different bundles might have different formatters configured which in turn require different cache contexts. If a view lists ten recent nodes of one bundle, but later ten nodes of a different bundle get posted, the required cache contexts could change. This might be OK given we'd also be invalidating the cache anyway with cache tags.
2. Parent items, rather than having cache contexts bubbled, explicitly, erm, suck up the cache contexts (so a view asks the entity displays which asks the formatters and adds them all together). This works if you always know the direct children, but not if you don't.
3. Set the cache contexts globally for all rendered items all the time. Means many cache items could be unnecessarily granular leading to bad hit rates.
Comment #7
Fabianx CreditAttribution: Fabianx commented2. is orthogonal to 1.
The more we do 2) (with as less effort as possible), the less we need to do 1.
To 1)
There are two variations that make this possible:
- using a union set of all cache_contexts (that is similar to 3) ['foo', 'moon']
- using a set of all possible cache contexts ( e.g. [ ['foo'], ['foo','moon'] ] depending on cache->getMultiple() being fast even for misses, which usually should be TRUE for DB up to 100 keys and for memcache up to 30 keys or so? Might need some static caching in convertTokensToKeys to make this efficient.
However all this bubbling enables also automatic detection of incompatible cache contexts.
There is two things to this:
- A cache context hierarchy (optional) (node grants < user < role, menu.trail < page, book.trail < page, etc.)
- Knowing the parents context
Once this is done, an incompatible cache context can:
- either be bubbled up and treated with 1)
OR
- be replaced with a placeholder by using the #pre_render(_cache) and turning it into a #post_render_cache.
So that things that can be independently rendered can be rendered after the main page is rendered.
To show that placeholdering (explained in more detail in the google doc) is not just a 'fantasy' here is a PoC: https://gist.github.com/LionsAd/e8ffd028a3dde63b0492
Overall bubbling of cache contexts is important because:
- it makes Drupal correct (Make it correct, THEN make it fast)
Different strategies can then be applied to deal with the dynamicness of the cache granularity this introduces. (as explained above)
But proper placeholdering (which also enables render strategies like Big Pipe, ESI, etc.) also needs cache_context bubbling.
Comment #8
Wim LeersMuch of #7 is captured in https://docs.google.com/a/acquia.com/document/d/1Gw7ohBOUKu38t4kMbN9zj6c..., where Fabian and I have been collaborating to prove this all works.
Comment #9
Wim LeersBasically, I originally intended this issue to be for "simple bubbling only". But that indeed doesn't work for the reasons outlined in #6: if this patch is committed as-is, then it'd cause guaranteed cache misses whenever a parent receives an additional bubbled cache context from a child.
The solution is Fabianx's two-tier caching. Working on that now.
That will also allow us to close #2099137: Entity/field access and node grants not taken into account with core cache contexts.
Comment #10
catchPostponed #2099137: Entity/field access and node grants not taken into account with core cache contexts on this. We may still have a bit of work on that issue to double check node grants and field access are properly reflected in the cache contexts assigned to entities and formatters.
Bumping this to critical since it blocks.
Comment #11
Wim LeersImplemented Fabianx' (GENIUS!!!!!!!!) proposal. I started from his sample code in https://gist.github.com/LionsAd/fef2568413569c13e952. See my implementation of that in
interdiff-fabianx.txt
. Changes compared to his sample code:Added extensive comments to ensure it's understandable.
Hunk 1: Renamed
#cache_old_id
to#cid_pre_bubbling
. (The documentation talks about "pre-bubbling" and "post-bubbling" cache IDs.)Hunk 2: Renamed
#cache_try_again
to#cache_redirect
.Hunk 3: Put
$data['#cache']['tags']
in a$tags
variable.So basically: docs to aid understanding, and tweaked variable names to hopefully clarify it.
Consider this patch more like a review to gain a thorough understanding (thus confirming that I can't find any problems with it) and share that understanding. It is truly Fabianx who rolled this patch!
My test case was #2099137: Entity/field access and node grants not taken into account with core cache contexts, and then the most obvious case for it: the
timezone
cache context inEntityViewBuilder
, which actually belongs onTimestampFormatter
(as well asDateTimeDefaultFormatter
andDateTimePlainFormatter
, where I also added it).I can confirm it works flawlessly! :)
interdiff.txt
==interdiff-fabianx.txt
+interdiff-timezone_field_formatters.txt
.Comment #13
Wim LeersAll simple-to-fix & expected test failures — yay! Now fixing those.
In this reroll: a first pass over all field formatters (
FormatterInterface
implementations), to specify the right contexts as needed. Not doing field access yet, just taking into account the logic for the field *formatters*. Also added a helper methodCache::mergeContexts()
, symmetrical toCache::mergeTags()
. Still need to update the code from previous patches to use that.Comment #14
catchThis looks really encouraging.
Drupal 6 does the two-tier caching for menu trees, and we've discussed using it was a workaround for cache contexts before, but figuring things out on set and using redirect both makes it easier to follow and optimizes for the case where it's not needed rather than doing it indiscriminately. Not having the double-get all the time is fantastic.
Note the google doc also has a section on dynamic cache contexts (i.e. where users with different versions of the same cache contexts could end up with different sets of cache contexts due to a particular formatter being used or not). The doc explains that this situation self-heals since whenever there's a cache miss, the set of cache contexts (and whether there's a cid redirect needed or not) will get recalculated if it needs to - and when that happens, existing cache entries will become invalid.
That covers all the concerns in #6.
Comment #16
Wim LeersFor the same reasons as #10, also postponing #2396333: BlockContentBlock ignores cache contexts required by the block_content entity, which is a completely analogous issue.
Comment #17
yched CreditAttribution: yched commentedSlightly above my head still (I'm slowly getting there though :-)), but the outcome is massively yay !
This puzzles me a bit - is that really correct ? The referenced entities could be of different bundles (or even different entity types with DER), each with different translatability configuration. So the first entity might be "not translatable" while the other ones are ?
Also, wondering if / how we should make that logic available for all ER formatters (and formatters for sub field types like file / image). This is sort of related to how #2405469: FileFormatterBase should extend EntityReferenceFormatterBase tries to standardize / centralize the tricky bits of entity ref logic for all "reference formatters".
Comment #18
Fabianx CreditAttribution: Fabianx commentedQuick comments from IRC:
1. The #cid_pre_bubbling should be a local variable as elements can be changed or exchanged by something so we can't rely on it.
Fortunately a local scope variable will work fine.
I know I wrote it that way, but I have since thought about it.
2. We need the same union set approach for the cache redirect, same as smart cache.
Both smart cache and this also are prone to race conditions and the smart cache collector is similar to a cache collector in reality, maybe we can re-use code?
It feels similar at least.
Will give it a more thorough review later, uploading my original PoC just for fun :).
Comment #19
Wim Leers#17: as I said in #13 — it's a first pass :P That's one of the bits of code where I was least sure, and could use your expertise there!
#18: history preserved on d.o > gist.github.com :)
Comment #20
Wim Leers#17: that code you cited actually caused test failures in
ShortcutCacheTagsTest
:) Fixed that specific problem now — we still need to think about the general problem for sure.This reroll should fix all test failures except the one in
CommentAnonymousTest
. Dinner time — starving :)Comment #21
yched CreditAttribution: yched commentedInterdiff #20 :
Thanks, this looks more reliable :-)
Then, what I'm not sure is whether we need to check if the entity currently has more than one translation, or simply whether it isTranslatable() ?
Also, I guess this code is currently in EntityReferenceLabelFormatter specifically because the patch is in progress ? Cache contexts are a concern for all ER formatters, right ?
Comment #23
Wim LeersFirst: making this green.
Next: replying to #21 and adding tests.
Comment #24
Wim Leersn00b
Comment #25
Wim Leers#21:
I went with the former, because that's also what
\Drupal\Core\Entity\EntityViewBuilder::getBuildDefaults()
does. But I agree that just checking::isTranslatable()
is better.Yes. But with a few caveats — let's look at each subclass of the base class:
On top of that:
EntityReferenceFormatterBase::getEntitiesToView()
had only just landed, and I didn't want to go change it prematurely :)Absolutely! But with the above caveats.
That being said, every entity reference formatter, including the ones exempted in the list above, do need to bubble the cacheability metadata (tags & contexts) of the access checking (i.e.
$entity->access('view)
).Working on that will likely require us to modify the return value of
EntityReferenceFormatterBase::getEntitiesToView()
. We might want to defer doing that to #2099137: Entity/field access and node grants not taken into account with core cache contexts though, because that would help keep this issue focused.What do people think about limiting the scope of this issue to:
?
Comment #27
yched CreditAttribution: yched commented@Wim : thanks for the replies :-)
EntityReferenceFormatterBase::getEntitiesToView() is indeed shaping to be a central piece. Could be best to wait for #2405469: FileFormatterBase should extend EntityReferenceFormatterBase.
Then, figuring out which formatter needs what, and how/if this can be split into some generic mechanism in EntityReferenceFormatterBase and overridable pieces in each concrete formatter, is likely to require some thought.
re: issue scoping - as you see fit :-) I'm thinking maybe EntityRef fields and formatters are tricky enough that they might deserve their own issue ?
Comment #28
Wim Leers#27: sounds great — I'll see how this plays out :) If we end up doing entity reference field formatters here, I'll make sure we wait for #2405469: FileFormatterBase should extend EntityReferenceFormatterBase. Thanks for your feedback, much appreciated!
Comment #29
catchEntity reference is largely #2099137: Entity/field access and node grants not taken into account with core cache contexts so we could either use that issue, or open a new one and mark that as duplicate.
Comment #30
Wim Leers#29: I think it's fine to do that there, yes. So let's do that then.
For the needed test coverage, I'd ideally write unit tests. @dawehner has done 99% of the work to convert our existing
RenderTest
(which usesKernelTestBase
) to aRendererTest
PHPUnit test. So I'm first going to help land #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests. That makes writing tests here simpler, and it prevents his work from going to waste.Comment #31
Wim LeersClarifying this is blocked on #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests (per #30).
Comment #32
Wim LeersFirst, a straight reroll of #25 on top of #2378883-37: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests (hence this patch does NOT apply to HEAD!). Since the prior patches updated
RenderTest
, but #2378883 makes that pretty much empty, I've applied the #25 patch, but omitted theRenderTest
hunks. The interdiff here shows the test changes necessary to accommodate this patch.This patch is only bigger than #25 due to more context being present, and the one or two additional things that had to be mocked in the unit tests. Witness are the diffstats:
25 files changed, 376 insertions(+), 36 deletions(-)
27 files changed, 384 insertions(+), 38 deletions(-)
Next: writing the actual test coverage for cache context bubbling.
Comment #33
webchickThis should be unblocked now, since the parent is committed. And it sounds like #32 should apply as a result now, so moving to needs review.
Comment #35
Wim LeersTest coverage added in this reroll. This does not #18.1, nor fix the test failures in #32, nor the consensus of #27–30 to move the entity reference changes out of this patch (which will probably fix all those test failures). Doing those things now.
Thanks to #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests, adding test coverage became significantly simpler.
In writing that test coverage, I found a few important bugs:
'rendered'
cache tag.This also includes a unit test to prove that catch's concern in #14 (second paragraph) is answered by passing tests, rather than only written text — see
RendererBubblingTest::testConditionalCacheContextBubblingSelfHealing()
.Having written this test coverage, and trying to break things (and having succeeded in breaking things, see the list of bugs fixed above), I'm now feeling more confident than ever before that this 1) works correctly, 2) will allow us to significantly speed D8 up in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) (for which this is a hard blocker), 3) will enable a lot of performance innovations in contrib.
Comment #36
Wim LeersRemoved the entity reference field and taxonomy field formatters. (Taxonomy fields are just a special case of entity reference formatter.) Those are now deferred to #2099137: Entity/field access and node grants not taken into account with core cache contexts, to keep this issue focused.
This will reduce the number of test failures, but it won't bring it down to zero, unfortunately.
Comment #37
Wim LeersFixing #18.1. That's the last bit of feedback that needed to be addressed. Now all that needs to happen is:
Comment #41
Fabianx CreditAttribution: Fabianx commentedReviewed especially the logic in cacheSet - looks good to me.
I would suggest as a follow-up to make this logic configurable independently of the renderer.
The reason being that we could also use the number of variations instead of the union set and cacheGetMultiple() (with knowing we can get cache misses).
The advantage to the simple union set approach is that old cache items are not left over (don't have to be written to another location).
--
I think I would argue even with the union set approach we need a cache tag for the "hash" of the old stored cache contexts and invalidate that on cacheSet.
e.g.
Base CID == 'block'
First hit:
First CIDs => block:%role
then when get variation set:
New CIDs => block:%role:%user
the block:%role items are now dangling - as unless the redirecting cache ID is cleared, we never can get back to block:%role and hopefully the redirecting CID is only cleared when a cache tag affecting all the cache items is cleared.
Therefore setting and invalidating a cache tag with the old stored_contexts should solve this.
Comment #42
Wim Leers+1
I'd say this is about as harmful as a rarely used cache item? It's up to the cache back-end to evict it (e.g. LRU-style); there's no need to add cache tags just to clear short-lived cache items.
Comment #43
Wim LeersFixed the failures in
NodeCacheTagsTest
. Should be down from 44 to 38 failures.Comment #45
Wim LeersI can't reproduce the failures in
SearchMultilingualEntityTest
… :( Testbot keeps saying there are 2 failed assertions there, but I get zero. Leaving that for last.All other failures are for views listing translated entities. The root cause is the change suggested by @yched in #21:
I implemented this, but turns out that it causes all these failures. Simply reverting that fixes things:
At first sight, it doesn't make sense that
isTranslatable() === FALSE
andcount(getTranslationLanguages()) > 1 === TRUE</code. But, Gábor was able to explain why: the entity might have translations that are created <em>before</em> the ability to translate was disabled! Furthermore, since these are tests, we're likely creating translations even when <code>isTranslatable()
=== FALSE.So, reverting that change for now; it was only intended as a minor clean-up, but to be able to apply it, we probably need to improve some tests, which is definitely out of scope for this issue.
Comment #46
Wim LeersLook at that! I must've mistested! Hurray! :)
This comment:
And this issue/patch:
… this brings us a huge step closer to the 2x speed-up demonstrated by #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). At the same time, it provides the necessary infrastructure to solve the criticals #2099137: Entity/field access and node grants not taken into account with core cache contexts and #2396333: BlockContentBlock ignores cache contexts required by the block_content entity.
Please review! :)
Comment #48
Wim LeersOops. Small oversight :)
Comment #49
Fabianx CreditAttribution: Fabianx commented"I'd say this is about as harmful as a rarely used cache item? It's up to the cache back-end to evict it (e.g. LRU-style); there's no need to add cache tags just to clear short-lived cache items."
This is different however as we actively make an old cache key invalid. Given all those keys are CACHE_PERSISTENT, we need to solve it.
It can be a major bug follow-up, but not doing anything is not correct.
The trick is to put a prefixed cache tag on the tags.
render_cache_context_count:0,
render_cache_context_count:1,
etc.
Given we only add contexts when creating the union set, this should be simple to do.
Invalidate previous count(), add new count().
With 7 contexts, this is i.e. 7 additional cache tags, which is manageable in terms of invalidation performance (DB UPDATE), space needed and cache checksum calculation.
And we write anyway at that point so its okay to invalidate there - though maybe you are right and this invalidation should be queued up for end-of-request instead - like GC. (as invalidate on cacheSet as done by e.g. config can have bad performance characteristics as seen in installer).
We can keep it simple and create a follow-up issue to move all things for when to expire cache tags that can be expired later to end-of-request.
So follow-up issue plan would be:
- Remove dangling cache items that can no longer be reached when cache context union changes.
- Move cache tags expiration to end of request for non-critical evictions. (e.g. GC or house keeping) -- generally useful, too
Comment #50
Fabianx CreditAttribution: Fabianx commented#cid_pre_bubbling to $cid_pre_bibbling.
Uhm, lets not merge Tags and contexts, please ...
Missing test coverage?
I strongly believe convertTokensToKeys should be doing the sorting.
Better to use $this->createCacheId()?
CNW, but besides that and the one follow-up issue to create this is close to RTBC.
Comment #51
Berdircache redirecting is an interesting concept ;) Can't say that I fully grok it, but it looks solid. Only thing that I'm a bit worried about is amount of cache get/set that are happening as part of render caching. We already have quite a lot of them and they're not free. render entries where caching is actually more expensive than rendering it in the first place might be something else to look for in the profile issue, I guess?
I meant to open an issue for this already. Is it really worth doing the array unique and sort in here?
Cache::mergeTags() is quite visible in profiling, and avoiding that might save a bit processing time?
At least cache tags are sorted anyway before they are persisted...
Uh, are you sure? ConfigurableLanguage is a config entity, and getName() just forwards to label().
Config entities are automatically translated, without their knowledge, so I am pretty sure that *will* return a translated language name.
One problem with cache contexts in general I guess is knowing what kind of contexts exist and when you need to add them.
Cache tags is often easier, with all the methods on config/entity that we have now, where we basically say, if the thing you are using has a getCacheTags() method, add it to your render array. but for contexts, that is a lot harder.
Not a problem of this issue of course, just thinking out loud...
Comment #52
yched CreditAttribution: yched commentedYep, I was kind of thinking the same about discoverability / documentation of available cache contexts and their exact semantics.
hook_info() in D7, Plugin definitions in D8, provide identifiable patterns for finding which "things of a given kind" (from field types to render/form elements) exist and are defined by who (I'm talking about "human" discoverability). Not sure who defines cache contexts and where developpers can look for them ?
Comment #53
Wim Leers#50:
sort()
call (inRenderer
).#51:
I share that concern, but it's wholly offset by #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). Furthermore, if we don't do it this way, then #2099137: Entity/field access and node grants not taken into account with core cache contexts would have to go back to the approach it was taking earlier, which was: for every request, determine which field formatters are used by this entity/bundle/view mode, iterate over all field formatters and get their cache contexts. Which means 1) instantiating a lot of (costly) objects that we don't truly need, 2) it can't support conditional cache contexts, so it then still only solves part of the problem.
If we didn't have the promise and hard numbers of #2429617, I'd also be doubtful. But that is within reach, and IMO addresses the concerns.
Cache::mergeTags()
. Both are sets, so they are handled the same. Why unique? Because it's a set. Why sorted? Because it improves the DX (all tags & contexts, everywhere, are sorted, making them easier to read & compare while working/debugging).LanguageInterface
, which says this:That's what I based that comment on.
#51.3 + #52:
Yes, we need to improve the DX around that. But that's precisely the purpose of the meta issue this is a child of: : #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance.
Right now:
cache_context.*
to find them. Command to do that:grep -R --color --include=*.services.yml 'cache_context\..*:$' .
. E.g.:If any of you have ideas on how to improve that: please ping me in IRC or open a new issue. But it's indeed not a problem introduced here. I agree we should improve it. Perhaps a drush command to list all cache contexts would also be helpful?
We have to instill the following thinking steps in D8 developers:
Now, in an ideal world, all of this would happen automatically. And I'm certain that we'll want to make that happen automatically in Drupal 9, or perhaps through additional OO APIs added in 8.x releases that generate render arrays automatically. i.e. if I'm rendering something that's only visible for users with a certain permission, the
user.roles
permission will get added.Comment #54
effulgentsia CreditAttribution: effulgentsia commentedWhat's cool about the approach in this issue is that the 2nd cache get only happens if there actually are contexts for a child element not known beforehand to the parent element. Which means that in places where it would be cheap for the parent element to discover a child element's contexts beforehand and add it to its own render array (such as in #2396333: BlockContentBlock ignores cache contexts required by the block_content entity), we can do that and avoid the 2nd cache get. Whereas in places where the 2nd cache get is cheaper than the computation of discovering it otherwise (such as the example of #2099137: Entity/field access and node grants not taken into account with core cache contexts in #53), we can allow it to take place.
Comment #55
Fabianx CreditAttribution: Fabianx commented#54: Indeed! (Thanks!)
And more if / when we introduce global cache contexts that are always set, you can also optimize such cases easily out for an optimized build of your site - as you know what you want to vary on.
Further if we decide to do / when we use placeholdering, we won't be bubbling up by design (for those elements being replaced by placeholders) and such again avoid the double cache hit.
This issue is first and foremost about ensuring correctness of our cache system at all levels, while being as smart and optimized about this as possible.
Comment #56
Fabianx CreditAttribution: Fabianx commentedI re-reviewed the patch and this has:
- Appropriate test coverage
- All concerns addressed
- The last interdiff.txt is present in the patch ;-)
- The union-set logic we designed as part of the doc.
- Tests for the self-healing aspect of the system.
I will open an issue for the dangling Cache IDs, but thinking about this again, we indeed never delete items from cache, when invalidating CacheTags, so Garbage Collection is a larger / other issue anyway.
RTBC!
Let a new era of caching begin!
Comment #57
effulgentsia CreditAttribution: effulgentsia commentedI reviewed the patch in-depth as well, and +1 to the RTBC. Awesome work!
The attached patch just has some docs changes for things that I noticed during review. "Needs review" just for that: if these are acceptable, please reset to RTBC.
Additionally:
To be honest, I'm confused by why we're trying to support contexts whose presence can vary by something other than what is covered by one of the other contexts or cache keys. It seems brittle to me: if you truly have that situation, then your cache gets will be poisoned until the self-healing takes place, and when that is is too random. I read through
testConditionalCacheContextBubblingSelfHealing()
and that didn't clarify it for me. I get the idea that #access on an item can affect whether child item contexts bubble up, but whatever code sets that #access should also be responsible for adding the appropriate context by which that access decision was made to either that item or its parent. Not by a random global variable as is done in the test. However, the rest of this patch is awesome, so I don't want to hold it up on dealing with this point: that can be a followup discussion.Comment #58
amateescu CreditAttribution: amateescu commentedThe doc additions from this patch are awesome, they make it really easy to understand the whole concept :)
What's up with those three dots?
Do we need to worry about or prevent $pre_bubbling_cid from being an empty string?
We usually put the concrete data type before null.
Comment #59
effulgentsia CreditAttribution: effulgentsia commentedRe #58.1: Variable number of params, so they can't be named. Same pattern as in NestedArray::mergeDeep().
Re #58.2: In theory, that would probably be a good hardening. But the only place that $pre_bubbling_cid is set is inside an if statement that already checks for that on the assigned value.
Re #58.3: Fixed in this patch.
Comment #60
larowlanas per #56 and #57, docs look good to me
Comment #61
Fabianx CreditAttribution: Fabianx commentedThe doc additions are awesome, back to RTBC.
#57:
The example is the following and yes the cache_context is correctly set, but thats not enough.
Consider:
[ 'foo' ]
as the base cache context.
Now there is a field formatter making access per role, therefore we end up with:
[ 'foo' , 'user.roles']
Now when the user has role 1, they get #access => TRUE, with role 2 they get #access => FALSE
Now with #access => TRUE they get an additional cache context ['moon'] (Phase of the moon ;) !)
So lets assume a request with role 2 first with #access => FALSE, hence no moon!:
So we have Cache ID:
%foo:%role translated to foo:2 as the base cache ID was just foo we now have:
foo => ['#cache_redirect' => ['contexts' => ['foo', 'user.roles']]]
foo:2 => ['#markup']
Now we get a request for role => 1, #access => TRUE
As the cache context is correctly set, we get a MISS checking for:
foo getting redirected to
foo:1 => cache miss
Now we render that item and end up with ['foo', 'user.roles', 'moon'].
And now it looks like this:
foo => ['#cache_redirect' => ['contexts' => ['foo', 'user.roles', 'moon']]]
foo:2 => ['#markup'] ------ This now is a valid cache entry that can however no longer be reached as its missing moon, that is the GC I spoke about.
foo:1:half_moon => ['#markup']
But the next time role 2 comes we get:
foo getting redirected to
foo:2:half_moon => cache miss
and we end up at:
foo => ['#cache_redirect' => ['contexts' => ['foo', 'user.roles', 'moon']]]
foo:2 => ['#markup'] ------ This now is a valid cache entry that can however no longer be reached as its missing moon, that is the GC I spoke about.
foo:2:half_moon => ['#markup']
foo:1:half_moon => ['#markup']
Now the system is warm and we always get cache hits until the moon phase changes or the content's cache tags expire.
This is what is meant with self-healing.
As outlined above there are three different approaches how to mitigate this:
- Use a different strategy for the union set (set of sets): In our example [['foo', 'user.roles'], ['foo', 'user.roles', 'moon']]) and have Cache::getMultiple() willingly do cache misses (might be good depending if cache misses are costly or not and how many keys can be gotten at once) -- current plan: contrib/
- Allow to set the phase of the moon and role globally and vary everything on it (cache expires when moon phase changes at once, but no redirect needed)
- Create a placeholder for the moon field, render it cached by moon via #post_render_cache, cache the rest by role normally.
All of those can be combined endlessly of course, but I hope the above example makes the self-healing process more clear.
Comment #62
catchCan't review in depth this morning, but grabbing this one.
Comment #63
Wim Leers#54 + #55: Yes, all that too — thanks for adding that :)
#57:
First, note that this the self-healing test is basically a test implementing exactly what is described in the Google Doc, in the proof provided for
Second, note that we don't want Drupal's render caching to be broken for code that is suboptimal. So we need some robustness, and this provides that.
Third: an actual explanation :) Indeed,
#access
can affect whether child items' cache contexts bubble up. And indeed, the access result should provide the cache context (AccessResult::getCacheContexts()
) by which the access result is varied, and that should be set on the render array. But this is about the level beyond that: about the render array that is only displayed if#access === TRUE
, which then itself contains content that varies by a cache context. Let's say a render array varies bylanguage
, but a subtree of that is only shown if#access === TRUE
, and access is varied byuser.role
, then that subtree itself (when shown) may itself vary bytimezone
. Without this self-healing capability, this would be broken: if the first time this render array is rendered#access === FALSE
, we never are aware of thetimezone
cache context. But then if the next time we do have#access === TRUE
, we'll seetimezone
, so the cache contexts for the overall tree need to be updated. Hence: self-healing.In other words: it's not about
#access
's cache contexts, it's about the cache contexts beyond that!#58: yay! :) Very glad to have a +1 from you!
Comment #64
yched CreditAttribution: yched commentedre @Wim #53 :
Yeah, the grep example for discoverability of available context is definitely scary ;-) Also, it lists usages, not "who defines it where and what's its semantics". Definitely a separate issue, so I opened #2443323: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock with relevant quotes from this thread - wording is based on my beginners understanding of the current API and problem space, please feel free to rename / reword it more accurately :-)
Also, the 1-4 bullet points after "We have to instill the following thinking steps in D8 developers" are super valuable. Where in our docs can we print it in bold flaming letters ?
Comment #65
dawehnerThe IS doesn't mention the new concept of
#cache_redirect
Its also not obvious for me what / how
#cache_redirect
works, based upon the following comment in code:It would be nice to have an explanation WHY you have to redirect in some cases.
Comment #66
BerdirWe have a weird test fail in TMGMT 8.x that appeared recently, that I think is related to recent cache context changes, although not specifically this issue, but I wanted to bring it up somewhere.
Consider this scenario:
- You have comment translations into de and es (for example) on an untranslated host entity (e.g. a node).
- Now you access both the de and es page after another.
- Based on our testing, the second language you visit shows the comment in the translation of the first language you visited.
- The reason for this seems to be that the comments are cached within the node, and the node doesn't add the language to the cache key, but there is no translation of the node.
What I don't understand yet is why this doesn't fail anywhere in core and why it only started to fail recently for us, I don't see anything that would have made this work?
So...
a) Shouldn't render caching always consider the interface language, because there could be t()'s anywhere?
b) Should the language in the entity view builder always be added and act like a configurable cache context, so that it bubbles up? Looking at Wim's very nice explanation of key/tags/context, I'd say that the language isn't something that identifies my thing, it's something that the output varies by. Or not? (The boundaries between those two things are not always 100% clear I think)
c) Thinking more generically, are there more things that should be cache contexts, so that they can bubble up?
Comment #67
Wim Leers#64:
#65:
RendererInterface
: it is an implementation detail that could be different in an alternative implementation. If we want to make it an actual supported API, that's fine, but that's IMO out of scope for this issue. Also removing it from the issue title for that reason.RendererInterface
(since every renderer implementation should do this), hence the explanation in the implementation is very brief:Comment #68
Wim Leers#66:
Because we don't have tests for that use case, probably.
That I'm also puzzled by :/ The only reasons I can think of: something was causing render caches to be invalidated in between tests, or your tests were not testing the right thing.
Unless your comments actually live in a block, in which case #2318437: Replace the hardcoded langcode key on blocks with the 'language' cache context might be why.
Me neither.
a)
It was the plan to move in that direction anyway, but yes, that is a solid explanation for it to be absolutely essential.
b)
It depends on how your translation works. AFAIK, in D7 you can choose to e.g. translate entities by having a separate entity (node 3 translated to Dutch is node 25; node 3 is always English, node 25 is always Dutch) or by translating the entity itself (node 3 is available in both English and Dutch). In Drupal 8, AFAIK, it's always the latter case.
In the latter case, the language is a variation.
In the former case, the language is an independent thing. And if anything on the site uses this approach, that's when the boundaries become unclear.
So, yes, I agree that the (content) language cache context should always be added by the entity view builder for translated entities.
c)
Absolutely. We add them as we encounter them. Examples:
user.roles
) cache context. It'd be better to have an "is authenticated user" cache context, so that we only have two possible values: TRUE or FALSE, rather than many variations of "is authenticated" when using the "per role" cache context. This will also be added in #1805054.That's why it's so important that Drupal 8 allows one to define additional cache contexts. Because Drupal core alone cannot possibly know all the possible cache contexts to vary by; it may depend on your site/modules.
Comment #69
catchHaven't done a proper patch review yet but one question on the self-healing.
So this makes sense and is great. From the code it looks like we always compare the current set of cache contexts (i.e. any static + bubbled ones) on cache misses, then merge any new cache contexts found with that array and never subtract.
The examples (and tests) though don't explicitly cover the following case, and I want to double check that it's fine to avoid ping pong. Also even if the current code works for this, should we add explicit test coverage?
For role A the sub-element doesn't show at all.
For role B the sub-element results in an additional timezone cache context that bubbles.
For role C the sub-element results in a 'daylight' cache context that bubbles.
(and etc. potentially)
So in this case not only is the resulting cache context different from the one where no bubbling occurred, but also two different cache contexts bubble that are only seen exclusively.
Comment #70
Wim LeersI considered adding test coverage for that scenario. But it's actually the same principle — unless I'm mistaken.
testConditionalCacheContextBubblingSelfHealing()
currently has that boolean "foo" cache context. In one case, the bubbled cache contexts is the empty set, in the other case it's "bar".The situation you describe has a multi-value "user.roles" cache context. In case 1, the empty set is bubbled, in case 2 "timezone" and in case 3 "daylight".
That's still the same principle: give the different possible values for an unconditional cache context X, there may be different cache contexts bubbled (A, B, C …) for each of the possible values for X.
I'm fine with adding explicit test coverage for the use case you describe if you prefer that though :)
Comment #71
BerdirOk, opened #2444267: EntityViewBuilder should add the language of the entity as context, always, lets continue there with the entity view language context stuff. I guess we also need a separate follow-up for the interface language context?
Comment #72
catchHmm I don't think we gain much from the test coverage itself in its own right, but I do think the expanded example is worth having somewhere for documentation that the case is covered - and test coverage seems not the worst place.
We've been talking about the requirement of cache context bubbling for at least three years, i.e. #1233232-132: Add unified context system to core then again in #2099137: Entity/field access and node grants not taken into account with core cache contexts without having a name for it and without having figured out #cache_redirect + self healing as way to solve it. Since there are documentation follow-ups here I don't think it's necessary to cover that history in the comments of this patch, but do feel like we could use a more high-level explanation of why we've chosen the 'lazy two-tier' caching as the default approach, and why 'cache context sucking' (i.e. parent objects like entity displays grabbing information from formatters) or 'cache context firehose' (i.e. varying all items by all cache contexts in use) are insufficient.
Comment #73
Wim Leers#71: note that the language cache context today generates a cache key that takes *all* language types into account (interface + content + URL). We should indeed add separate cache contexts for each of those individually, but then we should remove the current generic one. If we'd also do #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles'), then that would be guaranteed to not add any more variations than necessary.
#72: Ok, let's do it.
Comment #74
Wim Leers#72:
Comment #75
Fabianx CreditAttribution: Fabianx commentedBack to RTBC, the test example is way cleaner now! Thanks, Wim!
Comment #76
effulgentsia CreditAttribution: effulgentsia commentedThanks so much for the explanation in #61 and test improvements in #74. Since this is a pretty nuanced feature, I took the liberty of adding extensive docs for it as well. Just a docs addition, that can be refined in follow-ups if needed, so leaving at RTBC.
Comment #78
catchThe doc improvements look good. We might want to move that to a topic somewhere, but can do that in a follow-up and it's good to have it rather than not.
Minor things:
Nit: no need for count().
And here.
Slightly missed opportunity to have this be the lyrics of Fiddle-I-Dee.
The globals in the test are a bit odd, but also make it very easy to read the logic, so not complaining.
Fixed the count() calls on commit. This has been one of the trickiest conceptual issues with render caching. Cache tag and asset bubbling we knew what we wanted very early on, but took a long time to get to #cache_redirect and figure out self-healing. It's fantastic to get this far and end up with what is a very straightforward implementation in the end.
Committed/pushed to 8.0.x, thanks!