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:

  1. add them to BubbleableMetadata
  2. also make every creator of BubbleableMetadata set contexts, if they have any. This includes FilterProcessResult.
  3. … 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 to Cache::mergeTags()
  • Addition: BubbleableMetadata now handles cache contexts also.
  • Addition: FilterProcessResult gains getCacheContexts(), addCacheContexts() and setCacheContexts() methods, symmetrical with the existing methods.
CommentFileSizeAuthor
#76 interdiff.txt5.65 KBeffulgentsia
#76 bubble_cache_contexts-2429257-76.patch64.67 KBeffulgentsia
#74 interdiff.txt8.61 KBWim Leers
#74 bubble_cache_contexts-2429257-74.patch61.1 KBWim Leers
#59 interdiff.txt604 byteseffulgentsia
#59 bubble_cache_contexts-2429257-59.patch57.08 KBeffulgentsia
#57 interdiff.txt4.74 KBeffulgentsia
#57 bubble_cache_contexts-2429257-57.patch57.08 KBeffulgentsia
#53 interdiff.txt6.01 KBWim Leers
#53 bubble_cache_contexts-2429257-53.patch58.11 KBWim Leers
#48 interdiff.txt1.58 KBWim Leers
#48 bubble_cache_contexts-2429257-48.patch57.44 KBWim Leers
#46 interdiff.txt9.42 KBWim Leers
#46 bubble_cache_contexts-2429257-46.patch57.45 KBWim Leers
#45 interdiff.txt764 bytesWim Leers
#45 bubble_cache_contexts-2429257-45.patch57.44 KBWim Leers
#43 interdiff.txt6.26 KBWim Leers
#43 bubble_cache_contexts-2429257-43.patch57.69 KBWim Leers
#37 interdiff.txt3.69 KBWim Leers
#37 bubble_cache_contexts-2429257-37.patch55.77 KBWim Leers
#36 interdiff.txt6.84 KBWim Leers
#36 bubble_cache_contexts-2429257-36.patch54.38 KBWim Leers
#35 interdiff.txt18.98 KBWim Leers
#35 bubble_cache_contexts-2429257-35.patch61.06 KBWim Leers
#32 interdiff.txt5.83 KBWim Leers
#32 bubble_cache_contexts-2429257-32.patch45.88 KBWim Leers
#25 interdiff.txt1.61 KBWim Leers
#25 bubble_cache_contexts-2429257-25.patch43.85 KBWim Leers
#23 interdiff.txt2.56 KBWim Leers
#23 bubble_cache_contexts-2429257-23.patch43.61 KBWim Leers
#20 interdiff.txt14.39 KBWim Leers
#20 bubble_cache_contexts-2429257-20.patch42.93 KBWim Leers
#18 smartcache-plus-poc.txt1.91 KBFabianx
#13 interdiff.txt11.44 KBWim Leers
#13 bubble_cache_contexts-2429257-13.patch31.62 KBWim Leers
#3 bubble_cache_contexts-2429257-3.patch10.74 KBWim Leers
#11 bubble_cache_contexts-2429257-11.patch20.28 KBWim Leers
#11 interdiff-fabianx.txt6.68 KBWim Leers
#11 interdiff-timezone_field_formatters.txt3.31 KBWim Leers
#11 interdiff.txt9.87 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers’s picture

Component: base system » cache system
Assigned: Unassigned » Wim Leers
Issue summary: View changes

Fixed omission in IS.

I already have a patch, will post soon.

Wim Leers’s picture

Status: Active » Needs work
FileSize
10.74 KB
Wim Leers’s picture

Title: Bubble cache contexts » [PP-1] Bubble cache contexts
Status: Needs work » Postponed
Issue tags: +Performance
Fabianx’s picture

  1. +++ b/core/modules/filter/src/FilterProcessResult.php
    @@ -175,6 +183,41 @@ public function setCacheTags(array $cache_tags) {
    +    $this->cacheContexts = array_unique(array_merge($this->cacheTags, $cache_contexts));
    

    c&p fail - cacheContexts

  2. +++ b/core/modules/filter/src/FilterProcessResult.php
    @@ -175,6 +183,41 @@ public function setCacheTags(array $cache_tags) {
    +    $this->cacheContexts= $cache_contexts;
    

    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.

catch’s picture

Cache 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.

Fabianx’s picture

2. 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.

Wim Leers’s picture

Much 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.

Wim Leers’s picture

Title: [PP-1] Bubble cache contexts » Bubble cache contexts
Status: Postponed » Active

Basically, 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.

catch’s picture

Priority: Major » Critical

Postponed #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.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
9.87 KB
3.31 KB
6.68 KB
20.28 KB

Implemented 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 in EntityViewBuilder, which actually belongs on TimestampFormatter (as well as DateTimeDefaultFormatter and DateTimePlainFormatter, where I also added it).

I can confirm it works flawlessly! :)


interdiff.txt == interdiff-fabianx.txt + interdiff-timezone_field_formatters.txt.

The last submitted patch, 11: bubble_cache_contexts-2429257-11.patch, failed testing.

Wim Leers’s picture

FileSize
31.62 KB
11.44 KB

All 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 method Cache::mergeContexts(), symmetrical to Cache::mergeTags(). Still need to update the code from previous patches to use that.

catch’s picture

This 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.

Status: Needs review » Needs work

The last submitted patch, 13: bubble_cache_contexts-2429257-13.patch, failed testing.

Wim Leers’s picture

Issue tags: +blocker

For the same reasons as #10, also postponing #2396333: BlockContentBlock ignores cache contexts required by the block_content entity, which is a completely analogous issue.

yched’s picture

Slightly above my head still (I'm slowly getting there though :-)), but the outcome is massively yay !

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceLabelFormatter.php
@@ -64,7 +64,16 @@ public function viewElements(FieldItemListInterface $items) {
+    // Cache contexts: if one entity is translatable, then they all are.
+    $cache_contexts = [];
+    $first_entity = reset($entities);
+    if (count($first_entity->getTranslationLanguages()) > 1) {
+      $cache_contexts[] = 'language';
+    }

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".

Fabianx’s picture

FileSize
1.91 KB

Quick 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 :).

Wim Leers’s picture

#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 :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
42.93 KB
14.39 KB

#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 :)

yched’s picture

Interdiff #20 :

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceLabelFormatter.php
@@ -66,13 +67,6 @@ public function viewElements(FieldItemListInterface $items) {
-    // Cache contexts: if one entity is translatable, then they all are.
-    $cache_contexts = [];
-    $first_entity = reset($entities);
-    if (count($first_entity->getTranslationLanguages()) > 1) {
-      $cache_contexts[] = 'language';
-    }
-
     foreach ($entities as $delta => $entity) {
       $label = $entity->label();
       // If the link is to be displayed and the entity has a uri, display a
@@ -109,6 +103,12 @@ public function viewElements(FieldItemListInterface $items) {

@@ -109,6 +103,12 @@ public function viewElements(FieldItemListInterface $items) {
       else {
         $elements[$delta] = array('#markup' => String::checkPlain($label));
       }
+
+      $cache_contexts = [];
+      if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
+        $cache_contexts[] = 'language';
+      }
+

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 ?

Status: Needs review » Needs work

The last submitted patch, 20: bubble_cache_contexts-2429257-20.patch, failed testing.

Wim Leers’s picture

FileSize
43.61 KB
2.56 KB

First: making this green.

Next: replying to #21 and adding tests.

Wim Leers’s picture

Status: Needs work » Needs review

n00b

Wim Leers’s picture

FileSize
43.85 KB
1.61 KB

#21:

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() ?

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.

Also, I guess this code is currently in EntityReferenceLabelFormatter specifically because the patch is in progress ?

Yes. But with a few caveats — let's look at each subclass of the base class:

  1. the taxonomy term RSS formatter doesn't need cache contexts because it doesn't use render arrays
  2. the ID formatter doesn't need cache contexts because the ID can never be different
  3. the entity formatter does full-blown entity rendering… which means the cache contexts will bubble automatically
  4. … which leaves only the label formatter, which is the only one I modified.

On top of that: EntityReferenceFormatterBase::getEntitiesToView() had only just landed, and I didn't want to go change it prematurely :)

Cache contexts are a concern for all ER formatters, right ?

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:

  1. setting appropriate cache contexts on fields & filters (already done, but needs to be refined/cleaned up) — everything else (like access results) deferred to follow-ups — that would avoid this becoming a huge patch
  2. implementing cache context bubbling (already done)
  3. add solid test coverage guaranteeing the correctness of cache context bubbling (to do)

?

Status: Needs review » Needs work

The last submitted patch, 25: bubble_cache_contexts-2429257-25.patch, failed testing.

yched’s picture

@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 ?

Wim Leers’s picture

#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!

catch’s picture

Entity 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.

Wim Leers’s picture

#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 uses KernelTestBase) to a RendererTest 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.

Wim Leers’s picture

Title: Bubble cache contexts » [PP-1] Bubble cache contexts
Status: Needs work » Postponed
Wim Leers’s picture

FileSize
45.88 KB
5.83 KB

First, 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 the RenderTest 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: 25 files changed, 376 insertions(+), 36 deletions(-)
  • #32 (this patch): 27 files changed, 384 insertions(+), 38 deletions(-)

Next: writing the actual test coverage for cache context bubbling.

webchick’s picture

Title: [PP-1] Bubble cache contexts » Bubble cache contexts
Status: Postponed » Needs review

This 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.

Status: Needs review » Needs work

The last submitted patch, 32: bubble_cache_contexts-2429257-32.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
61.06 KB
18.98 KB

Test 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:

  • the cache contexts should be sorted when creating a cache ID, to ensure that we generate the same cache ID
  • the redirecting cache items didn't get the 'rendered' cache tag.
  • the redirecting cache items didn't get the union of all cache tags of any variation that causes the redirecting cache item to be updated.
  • the bug Fabianx' already pointed out in #18.2, without which we end up doing "cache ping-pong".
  • related to that bug that Fabianx pointed out: we also need to use the set union of cache contexts when creating a cache item that itself only uses a subset of the cache contexts defined in the cache redirect, otherwise the cache redirect points to a CID where we expect the cache item to exist, but it actually exists in a different place.

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.

Wim Leers’s picture

FileSize
54.38 KB
6.84 KB

Removed 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.

Wim Leers’s picture

FileSize
55.77 KB
3.69 KB

Fixing #18.1. That's the last bit of feedback that needed to be addressed. Now all that needs to happen is:

  • zero test failures
  • more reviews!

The last submitted patch, 35: bubble_cache_contexts-2429257-35.patch, failed testing.

The last submitted patch, 36: bubble_cache_contexts-2429257-36.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: bubble_cache_contexts-2429257-37.patch, failed testing.

Fabianx’s picture

Reviewed 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.

Wim Leers’s picture

I would suggest as a follow-up to make this logic configurable independently of the renderer.

+1

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.

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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
57.69 KB
6.26 KB

Fixed the failures in NodeCacheTagsTest. Should be down from 44 to 38 failures.

Status: Needs review » Needs work

The last submitted patch, 43: bubble_cache_contexts-2429257-43.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
57.44 KB
764 bytes

I 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:

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()

I implemented this, but turns out that it causes all these failures. Simply reverting that fixes things:

-      if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
+      if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {

At first sight, it doesn't make sense that isTranslatable() === FALSE and count(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.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes
Issue tags: +API addition
FileSize
57.45 KB
9.42 KB

Look at that! I must've mistested! Hurray! :)

This comment:

  1. includes a clean-up reroll
  2. updates the IS

And this issue/patch:

  1. only makes API additions, not changes, tagging as such
  2. includes a lot of test coverage to ensure every edge case is handled as expected

… 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! :)

Status: Needs review » Needs work

The last submitted patch, 46: bubble_cache_contexts-2429257-46.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
57.44 KB
1.58 KB

Oops. Small oversight :)

Fabianx’s picture

"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

Fabianx’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
    @@ -111,6 +118,20 @@ public function renderPlain(&$elements);
    +   *     It is for that case that we must store the current (pre-bubbling) cache
    +   *     ID, so that we can compare it with the new (post-bubbling) cache ID
    +   *     when writing to the cache. We store the current cache ID in
    +   *     #cid_pre_bubbling.
    
    @@ -210,6 +231,12 @@ public function renderPlain(&$elements);
    +   *     At the same time, if #cid_pre_bubbling is set, it is compared to the
    +   *     calculated cache ID. If they are different, then a redirecting cache
    

    #cid_pre_bubbling to $cid_pre_bibbling.

  2. +++ b/core/modules/filter/src/FilterProcessResult.php
    @@ -175,6 +183,41 @@ public function setCacheTags(array $cache_tags) {
    +  public function addCacheContexts(array $cache_contexts) {
    +    $this->cacheContexts = Cache::mergeContexts($this->cacheTags, $cache_contexts);
    +    return $this;
    

    Uhm, lets not merge Tags and contexts, please ...

    Missing test coverage?

  3. +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
    @@ -579,20 +600,87 @@ public function testReferencedEntity() {
    +    sort($contexts);
    +    $contexts = \Drupal::service('cache_contexts')->convertTokensToKeys($contexts);
    

    I strongly believe convertTokensToKeys should be doing the sorting.

  4. +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
    @@ -579,20 +600,87 @@ public function testReferencedEntity() {
    +      );
    +      $actual_redirection_cid = implode(':', $cache_keys);
    

    Better to use $this->createCacheId()?

CNW, but besides that and the one follow-up issue to create this is close to RTBC.

Berdir’s picture

cache 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?

  1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
    @@ -22,6 +22,26 @@ class Cache {
    +    $cache_contexts = array_unique($cache_contexts);
    +    sort($cache_contexts);
    +    return $cache_contexts;
    

    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...

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/LanguageFormatter.php
    @@ -30,6 +30,9 @@ class LanguageFormatter extends FormatterBase {
    +    // The 'language' cache context is not necessary, because
    +    // \Drupal\Core\Language\LanguageInterface::getName() always returns the
    +    // human-readable English name.
         foreach ($items as $delta => $item) {
           $elements[$delta] = array('#markup' => $item->language ? String::checkPlain($item->language->getName()) : '');
         }
    

    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.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -31,7 +31,14 @@ public function viewElements(FieldItemListInterface $items) {
         foreach ($items as $delta => $item) {
    -      $elements[$delta] = array('#markup' => format_date($item->value));
    +      $elements[$delta] = [
    +        '#cache' => [
    +          'contexts' => [
    +            'timezone',
    +          ],
    +        ],
    +        '#markup' => format_date($item->value)
    +      ];
         }
     
    

    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...

yched’s picture

Yep, 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 ?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.11 KB
6.01 KB

#50:

  1. Done.
  2. Darn, fixed. Yes, missing test coverage. Added.
  3. Good idea. Also removed the other sort() call (in Renderer).
  4. Yes, this was written before that helper was added. Thanks, fixed :)

#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.

  1. Let's keep this for another issue. Then it's consistent with 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).
  2. If that's true, then it's a violation of the LanguageInterface, which says this:
      /**
       * Gets the name of the language.
       *
       * @return string
       *   The human-readable English name of the language.
       */
      public function getName();
    

    That's what I based that comment on.

  3. See next section.

#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: [meta] Finalize the cache contexts API & DX/usage […]: #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance.

Right now:

  • Look at the cache settings for blocks, all available cache contexts are listed there. That gives you UI labels to grep the code for, to find the corresponding ID in code.
  • Grep YML files for cache_context.* to find them. Command to do that: grep -R --color --include=*.services.yml 'cache_context\..*:$' .. E.g.:
    $ grep -R --color --include=*.services.yml 'cache_context\..*:$' .
    ./core/core.services.yml:  cache_context.url:
    ./core/core.services.yml:  cache_context.pager:
    ./core/core.services.yml:  cache_context.language:
    ./core/core.services.yml:  cache_context.theme:
    ./core/core.services.yml:  cache_context.timezone:
    ./core/core.services.yml:  cache_context.menu.active_trail:
    ./core/modules/book/book.services.yml:  cache_context.book.navigation:
    ./core/modules/node/node.services.yml:  cache_context.node_view_grants:
    ./core/modules/user/user.services.yml:  cache_context.user:
    ./core/modules/user/user.services.yml:  cache_context.user.roles:
    

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:

  1. I'm rendering something. That means I need to think of cacheability!
  2. Is this something that's expensive, and therefore is worth caching? If so: what identifies this thing? Those are the cache keys.
  3. Does the representation of the thing I'm rendering vary per role (needs permissions), per URL (needs something of the request), per language, per… something? Those are the cache contexts. (For every single thing that it varies by, I need to specify the cache context to use, so Drupal can apply smart caching.)
  4. When does the representation of the thing I'm rendering become outdated? I.e. which things does it depend upon, so that when those things change, so should my representation? Those are the cache tags.

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.

effulgentsia’s picture

Only thing that I'm a bit worried about is amount of cache get/set that are happening as part of render caching.

What'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.

Fabianx’s picture

#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.

Fabianx’s picture

Title: Bubble cache contexts » Bubble cache contexts and introduce #cache_redirect
Status: Needs review » Reviewed & tested by the community

I 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!

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
57.08 KB
4.74 KB

I 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:

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -532,7 +551,67 @@ protected function cacheSet(array &$elements) {
+      // Stored cache contexts incomplete: this request causes cache contexts to
+      // be added to the redirecting cache item.
...
+      // Current cache contexts incomplete: this request only uses a subset of
+      // the cache contexts stored in the redirecting cache item. Vary by these
+      // additional (conditional) cache contexts as well, otherwise the
+      // redirecting cache item would be pointing to a cache item that can never
+      // exist.

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.

amateescu’s picture

The doc additions from this patch are awesome, they make it really easy to understand the whole concept :)

  1. +++ b/core/lib/Drupal/Core/Cache/Cache.php
    @@ -22,6 +22,26 @@ class Cache {
    +   * @param string[] …
    

    What's up with those three dots?

  2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -508,13 +525,15 @@ protected function cacheGet(array $elements) {
    +   * @param string|null $pre_bubbling_cid
    ...
    +  protected function cacheSet(array &$elements, $pre_bubbling_cid) {
    
    @@ -532,7 +551,67 @@ protected function cacheSet(array &$elements) {
    +    if (isset($pre_bubbling_cid) && $pre_bubbling_cid !== $cid) {
    

    Do we need to worry about or prevent $pre_bubbling_cid from being an empty string?

  3. +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
    @@ -579,20 +600,85 @@ public function testReferencedEntity() {
    +   * @return null|string
    +   *   The redirected (post-bubbling) CID, if any.
    

    We usually put the concrete data type before null.

effulgentsia’s picture

Re #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.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

as per #56 and #57, docs look good to me

Fabianx’s picture

The 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.

catch’s picture

Assigned: Unassigned » catch

Can't review in depth this morning, but grabbing this one.

Wim Leers’s picture

#54 + #55: Yes, all that too — thanks for adding that :)

#57:

  1. Those doc improvements look great!
  2. Fabianx already wrote an explanation in #61, but I think I can provide a simpler explanation. So… attempting that :)
    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 Finally, and this is key: this works fine for conditional cache contexts.

    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 by language, but a subtree of that is only shown if #access === TRUE, and access is varied by user.role, then that subtree itself (when shown) may itself vary by timezone. 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 the timezone cache context. But then if the next time we do have #access === TRUE, we'll see timezone, 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!

yched’s picture

re @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 ?

dawehner’s picture

The 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:

+      // Two-tier caching: redirect to actual (post-bubbling) cache item.
+      // @see ::doRender()
+      // @see ::cacheSet()
+      if (isset($cached_element['#cache_redirect'])) {
+        return $this->cacheGet($cached_element);
+      }
       // Re

It would be nice to have an explanation WHY you have to redirect in some cases.

Berdir’s picture

We 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?

Wim Leers’s picture

Title: Bubble cache contexts and introduce #cache_redirect » Bubble cache contexts

#64:

#65:

  1. It's only not mentioned because it's meant to be a necessary implementation detail of cache context bubbling, it's not meant to be a first-class API. This is also why it's not mentioned in 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.
  2. The "why" is already documented in RendererInterface (since every renderer implementation should do this), hence the explanation in the implementation is very brief:
    +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
    @@ -210,6 +242,12 @@ public function renderPlain(&$elements);
    +   *     At the same time, if $pre_bubbling_cid is set, it is compared to the
    +   *     calculated cache ID. If they are different, then a redirecting cache
    +   *     item is created, containing the #cache metadata of the current element,
    +   *     and written to cache using the value of $pre_bubbling_cid as the cache
    +   *     ID. This ensures the pre-bubbling ("wrong") cache ID redirects to the
    +   *     post-bubbling ("right") cache ID.
    
Wim Leers’s picture

#66:

What I don't understand yet is why this doesn't fail anywhere in core […]

Because we don't have tests for that use case, probably.

[…] and why it only started to fail recently for us […]

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.

I don't see anything that would have made this work?

Me neither.

a)

Shouldn't render caching always consider the interface language, because there could be t()'s anywhere?

It was the plan to move in that direction anyway, but yes, that is a solid explanation for it to be absolutely essential.

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)

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)

Thinking more generically, are there more things that should be cache contexts, so that they can bubble up?

Absolutely. We add them as we encounter them. Examples:

  1. Absolute links should be varied by the HTTP host: on multisite setups, the same menus exist on both foo.com and bar.com, but in one case, they point to foo.com-relative links, and in the other case, to bar.com-relative links. A "HTTP host" cache context will be added in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
  2. Some things are displayed only for authenticated users. But we currently only have the "per role" (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.

catch’s picture

Haven't done a proper patch review yet but one question on the self-healing.

Let's say a render array varies by language, but a subtree of that is only shown if #access === TRUE, and access is varied by user.role, then that subtree itself (when shown) may itself vary by timezone. 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 the timezone cache context. But then if the next time we do have #access === TRUE, we'll see timezone, 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!

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.

Wim Leers’s picture

I 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 :)

Berdir’s picture

Ok, 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?

catch’s picture

Hmm 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.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

#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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
61.1 KB
8.61 KB

#72:

  1. Test coverage: done. Replaced the existing test rather than adding a new one. Checked with catch, Fabianx and effulgentsia in chat, and they all agreed that that makes more sense.
  2. I think that's within the scope of #2444211: Document cacheability of render arrays, and the considerations to use when generating render arrays.
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, the test example is way cleaner now! Thanks, Wim!

effulgentsia’s picture

Thanks 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.

  • catch committed fe04699 on 8.0.x
    Issue #2429257 by Wim Leers, Fabianx, effulgentsia: Bubble cache...
catch’s picture

Status: Reviewed & tested by the community » Fixed

The 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:

  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -525,14 +544,174 @@ protected function cacheSet(array &$elements) {
    +      if (count(array_diff($merged_cache_contexts, $stored_cache_contexts)) > 0) {
    

    Nit: no need for count().

  2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -525,14 +544,174 @@ protected function cacheSet(array &$elements) {
    +      if (count(array_diff($merged_cache_contexts, $cache_contexts)) > 0) {
    

    And here.

  3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
    @@ -68,6 +71,392 @@ public function testBubblingWithoutPreRender() {
    +          'tags' => ['dee', 'fiddle', 'har', 'rendered', 'yar'],
    

    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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.