After #2094241: Cache tag the page cache we can remove the 'content' cache tag. That was a hack to preserve the functionality of CACHE_TEMPORARY from previous versions, and we won't see the benefits of cache tags for page caching until it's gone.

In terms of real-life performance this is critical, but it's likely something that could be done after release, so putting at 'major' for now.

Any removal of a content cache tag that does not trigger a test failure, should receive additional test coverage, to ensure we make no regressions.

Note: this is also important for much better/smarter/more efficient reverse proxy caching too, because it'll allow us to essentially cache all pages forever, and then just purge those that match the appropriate cache tags. Without crazy/unreliable contrib modules that have to hack that in.

Missing/broken cache tags blockers

Cacheable blocks

  1. Blocks: #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags Assigned to: catch
  2. Optimize menu & book blocks: #2224861: Cache SystemMenuBlock and BookNavigationBlock per active trail (currently cached per URL, this breaks on very long URLs) Assigned to: Wim Leers
  3. Cacheable form blocks: #2221391: Cacheable form blocks: Remove #token from the user login form
  4. Cacheable language switcher block: #2232375: Make language switcher block cacheable
  5. Cacheable syndicate block: #2232385: Cacheable "Syndicate" block: never cache it, because it's cheaper to always render it Assigned to: Wim Leers
  6. Cacheable breadcrumbs block: #2232609: Cacheable breadcrumbs block, and fix breadcrumb builders
  7. Cacheable branding block: #2185617: Each page cache entry should have a theme cache tag Assigned to: Wim Leers
  8. TODO: optional core modules' blocks: AggregatorFeedBlock, ActiveTopicsBlock, NewTopicsBlock, ShortcutsBlock, StatisticsPopularBlock, ViewsBlock and ViewsExposedFilterBlock. The last two are blocked on #2183017: Views doesn't bubble up rendered entities' cache tags.

Test coverage

General test procedure

  1. Install Drupal 8 in the Standard profile.
  2. Enable page caching (go to admin/config/development/performance, check the "Use internal page cache" setting and set the "Page cache maximum age" setting to e.g. 3 minutes).
  3. Create content (e.g. create one node (with a title, a taxonomy term or two and something in the body field), that is visible on the front page)
  4. Visit the content as the anonymous user (e.g. the front page)
  5. Verify that all the expected page cache tags exist in the database (look at the cache_page table, find the cache entry, look at the cache_tags column)
  6. Edit any of the content that appears on that page
  7. Verify that the page cache entry was deleted
    1. If it works and it's worth testing, then add it to the PageCacheTagsIntegrationTest.
    2. If it doesn't work, open an issue for it, definitely add a test to the PageCacheTagsIntegrationTest, and post that patch. Until it's green, we have work to do :)
  8. Repeat until we can no longer find any problems related to cache tags!
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Postponed
Issue tags: +Spark, +sprint

I started working on this.

Unfortunately, page cache tag is currently broken, see #2167039-50: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page().

It's clear already that we won't be able to simply remove the content cache tag, because cache tags for e.g. menus (not menu links) are missing. And that's probably only the tip of the ice berg.

Wim Leers’s picture

#2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache now has a patch that completely fixes the menu part of the problem. Part of this patch will include getting rid of _menu_clear_page_cache(). This patch should also expand the \Drupal\system\Tests\Cache\PageCacheIntegrationTest test introduced there, if that turns out to be useful.

Another huge problem that I encountered: the front page View prevents bubbling up of cache tags provided by the entities that it contains, despite them being rendered using the Entity/Field API render pipeline. Investigating that is probably going to be the next step.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
nielsvm’s picture

This would be massive for the Acquia Purge module I've written, as it would basically take away all the unreliability that expire gives experience wise. In my opinion this could warrant a hook (or similar fired event) that simply receives that passed in paths as stored in the cache tags.

Having these working in views would be massively great, as that's the biggest gap for Acquia Purge customers: where we advise to detect changes using custom rules :/.

I'll make myself available for help on this.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Wim Leers’s picture

Title: Remove the 'content' cache tag » Ensure everything on a page has corresponding cache tags, so we can remove the 'content' cache tag

This is a more accurate title. I wonder if this shouldn't in fact be critical…

Wim Leers’s picture

Issue summary: View changes
catch’s picture

Priority: Major » Critical

Yeah I think this should be critical. Some of the child issues are requiring API changes, and if we get stuck with the content cache tag for the duration of 8.x it'll seriously affect our ability to make up for all the performance regressions introduced elsewhere.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
catch’s picture

Title: Ensure everything on a page has corresponding cache tags, so we can remove the 'content' cache tag » [meta] Ensure everything on a page has corresponding cache tags, so we can remove the 'content' cache tag
Status: Postponed » Active

Image styles potentially need to tag the resulting cached HTML - in case the dimensions of the image style changes when it's updated.

See #2204159-3: (Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing.

Bumping this to a meta and un-postponing.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
29.23 KB

If we remove all occurrences of the content cache tag, then that would currently look like the attached patch. There should be many failures. As we improve cache tag usage everywhere, the number of failures should go down. Any removal of a content cache tag that does not trigger a test failure, should receive additional test coverage, to ensure we make no regressions.

(This serves as a progress indicator for this issue.)

catch’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 16: remove_content_cache_tag-2124957-16.patch, failed testing.

Wim Leers’s picture

I'm honestly scared by how few test failures this causes — it means our test coverage in this area is very lacking…

sun’s picture

In almost all cases, tests are working around stale cache problems by manually clearing caches, but in fact, I'm not aware of any test that would specifically test cache existence and clearing of cached data.

In light of the cache tag architecture, I would actually say that asserting this in individual tests is rather pointless/questionable. The nature of cache tags is that the cache invalidation layer is moved into abstracted application layer logic that is, strictly speaking, not part of your own functional code anymore. Unlike a discrete untagged cache item, the invalidation of a tagged cache item vastly depends on what exact other code is running in parallel to your code, because a possible intersection of tags causes your cache items to get invalidated, too.

I think we need a proper plan for testing tagged cache items.

I'd rather not want to see lots of individual new tests whose only purpose is to assert that some specific combinations of enabled modules causes specific cache items to be invalidated in specific scenarios that are tightly coupled to aforementioned combination of enabled modules. We should have a better plan than that.

Wim Leers’s picture

Issue summary: View changes

Progress!

#2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags landed. It provides integration test coverage for the most visible content entity types.

Next up: #2217749: Entity base class should provide standardized cache tags and built-in invalidation, which will provide the assurance that for each entity type, a proper cache tag exists and that if you inherit from the recommended base classes, cache tag invalidation already works.

Together with #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags, we can have peace of mind that Drupal 8's entity cache tags are comprehensive.

The only potential addition that might be needed is to have >=1 simple integration test per entity type to ensure that the proper cache tags are also effectively being set in the render process. Most config entities are not rendered directly, but are used as part of another rendering process, e.g. the image_style config entity is used in the image field's formatter.


@sun: #2217749: Entity base class should provide standardized cache tags and built-in invalidation would be the generic coverage you were looking for. #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags does not test "with some specific combination of enabled modules". It tests the most crucial content entity types in very complete integration tests. Because solely #2217749: Entity base class should provide standardized cache tags and built-in invalidation would not give sufficient guarantees.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

Let's make all other blocks in core cacheable out of the box as well!

Wim Leers’s picture

Issue summary: View changes

#2217749: Entity base class should provide standardized cache tags and built-in invalidation has finally landed!

Now I can work on #2204159: (Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing, #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags and other, yet to be created issues — which are the LAST SET OF ISSUES to work on for this meta issue to be completed, all of which can be found below:

  1. Last content entity types that end up on the page, and therefore affecting render caches:
    1. Aggregator Feed/Item: #2241229: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags
    2. Shortcut: #2241235: Shortcut/ShortcutSet entity types should use cache tags

    (contact.module's Message type doesn't qualify: it's never rendered!)

  2. Last config entity types that end up on the page, and therefore affecting render caches:
    1. search.module's SearchPage: #2241249: First step in making search results pages cacheable: add the associated SearchPage's cache tag
    2. ShortcutSet: merged with #2241235: Shortcut/ShortcutSet entity types should use cache tags
    3. Tour: #2241267: Make tours set cache tags
    4. DateFormat: #2241275: DateFormat cache tag: don't set the cache tag, but invalidate the entire render cache?

    (rdf.module's RdfMapping doesn't qualify because it's done in the theme layer, which is too late to set cache tags.)

In creating these issues, I also discovered a recently added regression: #2241291: Regression: menu link-specific cache tags.

And finally, once these have landed, we can start analyzing which cache tags work well, which should be optimized (like the Menu/Menu Link one), and so on.

catch’s picture

Wim Leers’s picture

#25: thanks for that :)

catch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: remove_content_cache_tag-2124957-16.patch, failed testing.

Wim Leers’s picture

I need to reroll the patch here. Will do so today.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
31.47 KB
4.52 KB

Reroll of #16, started again from scratch because so much has changed since. Beyond simply deleting the content cache tag, also:
renamed _menu_clear_page_cache() to _menu_update_expanded_menus() because that's all it does now.

The interdiff contains the most noteworthy changes.

Status: Needs review » Needs work

The last submitted patch, 30: remove_content_cache_tag-2124957-30.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
32.49 KB

This just needed two block tests updating that were looking for content cache tags that are no longer there.

catch’s picture

Assuming this comes back green, then we have the same (better) correctness and test coverage as we had for cache_clear_all(), so I think we could go ahead and commit the patch, and continue with the other (major and critical) issues as we find them.

Status: Needs review » Needs work

The last submitted patch, 32: remove_content_cache_tag-2124957-32.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
39.98 KB
10.97 KB

#32: HAHAHAH how stupid of me!

#33: oh wow, I didn't think that would be acceptable, but that'd make my life a lot easier. It'll also show the problems more explicitly. And most (fundamental) things will work just fine thanks to the work we've done so far. So yay! :)

Interdiff against #30.

Status: Needs review » Needs work

The last submitted patch, 35: remove_content_cache_tag-2124957-35.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
40 KB

Straight reroll.

catch’s picture

Title: [meta] Ensure everything on a page has corresponding cache tags, so we can remove the 'content' cache tag » [meta] Ensure everything on a page has corresponding cache tags, remove the 'content' cache tag
Status: Needs review » Reviewed & tested by the community

OK marking this RTBC. The issue summary is useful once this goes in, so we could keep it open as a (major) meta after commit.

The situation once this patch is in:

1. Some modules clear cache_render (previously cache_block/cache_page) manually instead of using cache tags- this is leftover code from Drupal 7. They need fixing but were never affected by the replacement of CACHE_TEMPORARY with the content cache tag because it was not using cache_clear_all() with no arguments in the first place.

2. Some modules don't clear caches on content change at all, relying on TTLs etc. Those also aren't affected by CACHE_TEMPORARY removal.

This issue has resulting in finding lots of those, but they already have major/critical issues open, and removing the content cache tag now isn't a regression (in fact it's a major performance improvement).

sun’s picture

I guess this could use a change notice?

catch’s picture

It'll be an update to https://drupal.org/node/1534648 (which is very out of date) or https://drupal.org/node/1272696

This isn't really an API change - if you're adding this tag, nothing stops you. If you're invalidating it, all the stuff you were invalidating should be handling it's own cache coherency anyway.

Berdir’s picture

Hm.

So we have #2241377: [meta] Profile/rationalise cache tags to review cache tags usage but what exactly are we going to do if we decide that cache tag X (let's say, image style cache tags) aren't worth it, remove them and... what will we replace them with? Will we be able to do *clear all render caches* without a global cache tag? Can we require that all render caches must be in the render bin? Or will we end up with render cache related cache entries in the default or data bins, for example that we'd need to clear as well?

If we mandate that clearing the render cache is enough, great, if there's a chance that it's not, my suggestion would be to keep the content cache tag and just remove *invalidations* here. Having one more cache tag to check for a page request isn't going to make much of a difference. That would make it easier to re-introduce the cache invalidation for that for the very rare cases?

Also, this task is currently a meta issue to keep track of different issues that still need to be done, should we move it to active again after committing this or move the patch to a separate more focused issue?

From the issue summary:

Any removal of a content cache tag that does not trigger a test failure, should receive additional test coverage, to ensure we make no regressions.

Based on that, we should have separate patches to find out which cache invalidations have test coverage and which ones don't? That or somehow move this to follow-up issues but I think this will be hard to verify once it's gone? So we should at least create the issues for the different use cases that have no test coverage before we commit this?

Also, based on a discussion that I had with @znerol today, I was wondering if we could use the render cache for check_markup, and introduce a #check_markup or something element type that can replace a manual check_plain() call? Because we could then possibly

a) remove the custom check_markup() cache *and*
b) we might even be able to go as far as dropping the whole field prepare cache API because we can cache that efficiently enough with the render cache, especially with #2099131: Use #pre_render pattern for entity render caching? That would remove a huge amount of complexity from the persistent entity cache topic, as we wouldn't have to support use cases like clearing the entity storage cache when changing text formats anymore?

Not sure if there's a better place to discuss that?

Berdir’s picture

I guess the part about regression testing in case invalidations don't result in test fails is already too late anyway as we fixed the test fails we knew about already?

catch’s picture

Status: Reviewed & tested by the community » Needs review

Hmm I'd not thought of leaving the tag in but removing the invalidations, that's definitely worth thinking about. I'd been assuming we'd do direct cache_render clears, but it seems cleaner to still use the tag, and the pattern of rarely invalidated/oft-requested cache tag -> content seems pretty easy.

The only place you'd run into issues is a contrib module invalidating the content cache tag unnecessarily, but that's just as much possibly with {cache_render} direct clears too.

Back to CNR at least.

For check_markup(), I opened #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags which is similar. I definitely think once the #pre_render patch is in we can get rid of the entity caching of text fields, that was only ever an optimization for completely uncached field renders.

Based on that, we should have separate patches to find out which cache invalidations have test coverage and which ones don't? That or somehow move this to follow-up issues but I think this will be hard to verify once it's gone? So we should at least create the issues for the different use cases that have no test coverage before we commit this?

I think we already have those issues, most linked from the summary.

Wim Leers’s picture

Great feedback, Berdir — thanks!

I agree that we need a way to clear "all render caches", because some modules or sites may choose to use a different cache bin for rendered things. In fact, we kind of already have three cache bins for "rendered things": cache_render, cache_page and cache_toolbar.
I think we have two ways to achieve this:

  1. If we want to continue to have a cache tag, then I suggest we use array('rendered' => TRUE) or something like that, and make drupal_render_cache_set() set it.
  2. Alternatively, we could have a way to tag cache bins containing rendered items with a cache.bin.rendered tag, with a corresponding compiler pass and a drupal_render_cache_clear() function that simply calls deleteAll() on each cache bin.

I think the latter is closest in spirit to the original intent of this issue, but I don't have a strong preference.

catch’s picture

Status: Needs review » Needs work

If we want to continue to have a cache tag, then I suggest we use array('rendered' => TRUE) or something like that, and make drupal_render_cache_set() set it.

This sounds good to me. Will make fixing #2241377: [meta] Profile/rationalise cache tags much easier too - just don't bother setting tags and clear the 'rendered' cache tag if we decide certain ones are too granular/rarely invalidated.

Berdir’s picture

+1, sounds good to me :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
45.22 KB
13.66 KB

Alrighty! :)

Implemented #44.1, because of the +1s in #45/#46.


#2241275: DateFormat cache tag: don't set the cache tag, but invalidate the entire render cache? is postponed on this.

catch’s picture

Looks good to me.

catch’s picture

Title: [meta] Ensure everything on a page has corresponding cache tags, remove the 'content' cache tag » Replace 'content' cache tag with 'rendered' and use it sparingly

Status: Needs review » Needs work

The last submitted patch, 47: remove_content_cache_tag-2124957-47.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
47.9 KB
3.51 KB

Finally found time to reroll this sucker again.

First, this had to be rerolled; HEAD has changed quite a bit in the past month. The interdiff shows the changes to fix the test failures in #47.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Since this was RTBC earlier, and was pseudo-re-RTBC'd by catch in #49 for trivial changes, and #51 only contains super trivial changes, I'm re-RTBC'ing this.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/src/Tests/BlockViewBuilderTest.php
@@ -222,7 +222,7 @@ public function testBlockViewBuilderAlter() {
     $this->setBlockCacheConfig(array(
@@ -237,8 +237,8 @@ public function testBlockViewBuilderAlter() {

@@ -237,8 +237,8 @@ public function testBlockViewBuilderAlter() {
     $this->assertIdentical(drupal_render($build), '');
     $cache_entry = $this->container->get('cache.render')->get($cid);
     $this->assertTrue($cache_entry, 'The block render element has been cached with the expected cache ID.');
-    $expected_flattened_tags = array('content:1', 'block_view:1', 'block:test_block', 'theme:stark', 'block_plugin:test_cache');
-    $this->assertIdentical($cache_entry->tags, $expected_flattened_tags); //, 'The block render element has been cached with the expected cache tags.');
+    $expected_flattened_tags = array('block_view:1', 'block:test_block', 'theme:stark', 'block_plugin:test_cache', 'rendered:1');
+    $this->assertIdentical($cache_entry->tags, $expected_flattened_tags, 'The block render element has been cached with the expected cache tags.');
     $this->container->get('cache.render')->delete($cid);
 
     // Advanced: cached block, but an alter hook adds an additional cache tag.
@@ -246,12 +246,12 @@ public function testBlockViewBuilderAlter() {

@@ -246,12 +246,12 @@ public function testBlockViewBuilderAlter() {
     \Drupal::state()->set('block_test_view_alter_cache_tag', $alter_add_tag);
     $expected_tags = NestedArray::mergeDeep($default_tags, array($alter_add_tag => TRUE));
     $build = $this->getBlockRenderArray();
-    $this->assertIdentical($expected_tags, $build['#cache']['tags'], 'An altered cacheable block has the expected cache tags.');
+    $this->assertIdentical($expected_tags, $build['#cache']['tags']); //, 'An altered cacheable block has the expected cache tags.');
     $cid = drupal_render_cid_create(array('#cache' => array('keys' => $expected_keys)));
     $this->assertIdentical(drupal_render($build), '');
     $cache_entry = $this->container->get('cache.render')->get($cid);
     $this->assertTrue($cache_entry, 'The block render element has been cached with the expected cache ID.');
-    $expected_flattened_tags = array('content:1', 'block_view:1', 'block:test_block', 'theme:stark', 'block_plugin:test_cache', $alter_add_tag . ':1');
+    $expected_flattened_tags = array('block_view:1', 'block:test_block', 'theme:stark', 'block_plugin:test_cache', $alter_add_tag . ':1', 'rendered:1');
     $this->assertIdentical($cache_entry->tags, $expected_flattened_tags); //, 'The block render element has been cached with the expected cache tags.');
     $this->container->get('cache.render')->delete($cid);
 

Minor, but the commented out assertion methods should just be removed.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
47.56 KB
1.82 KB

They weren't commented, only their message was commented. Restored.

catch’s picture

Sorry I mis-typed. s/methods/messages/...

As long as they're properly in or out that's fine though, thanks.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 6f33b60 on 8.x by catch:
    Issue #2124957 by Wim Leers, catch: Replace 'content' cache tag with '...
Wim Leers’s picture

Issue tags: -sprint

Lovely — and at last, I'd say :)

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture