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
- Menus: #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache Assigned to: Wim Leers
- Menus regression: #2241291: Regression: menu link-specific cache tags Assigned to: Wim Leers
- Views: #2183017: Views doesn't bubble up rendered entities' cache tags
- Blocks: #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags Assigned to: catch
- Themes: #2185617: Each page cache entry should have a theme cache tag Assigned to: Wim Leers
- Image styles: #2204159: (Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing Assigned to: Wim Leers
- Filters referencing cache tagged things: #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags Assigned to: Wim Leers
- Aggregator
Feed
/Item
: #2241229: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags Assigned to: catch Shortcut
/ShortcutSet
: #2241235: Shortcut/ShortcutSet entity types should use cache tags Assigned to: Wim LeersSearchPage
: #2241249: First step in making search results pages cacheable: add the associated SearchPage's cache tag Assigned to: Wim LeersTour
: #2241267: Make tours set cache tagsDateFormat
: #2241275: DateFormat cache tag: don't set the cache tag, but invalidate the entire render cache?
Cacheable blocks
- Blocks: #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags Assigned to: catch
- 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
- Cacheable form blocks: #2221391: Cacheable form blocks: Remove #token from the user login form
- Cacheable language switcher block: #2232375: Make language switcher block cacheable
- Cacheable syndicate block: #2232385: Cacheable "Syndicate" block: never cache it, because it's cheaper to always render it Assigned to: Wim Leers
- Cacheable breadcrumbs block: #2232609: Cacheable breadcrumbs block, and fix breadcrumb builders
- Cacheable branding block: #2185617: Each page cache entry should have a theme cache tag Assigned to: Wim Leers
- TODO: optional core modules' blocks:
AggregatorFeedBlock
,ActiveTopicsBlock
,NewTopicsBlock
,ShortcutsBlock
,StatisticsPopularBlock
,ViewsBlock
andViewsExposedFilterBlock
. The last two are blocked on #2183017: Views doesn't bubble up rendered entities' cache tags.
Test coverage
- PHPUnit tests for entity base classes: #2217749: Entity base class should provide standardized cache tags and built-in invalidation
- Integration tests for Comment, Custom Block, Node, Taxonomy, User: #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags Assigned to: Wim Leers
General test procedure
- Install Drupal 8 in the Standard profile.
- 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). - 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)
- Visit the content as the anonymous user (e.g. the front page)
- Verify that all the expected page cache tags exist in the database (look at the
cache_page
table, find the cache entry, look at thecache_tags
column) - Edit any of the content that appears on that page
- Verify that the page cache entry was deleted
-
- If it works and it's worth testing, then add it to the
PageCacheTagsIntegrationTest
. - 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 :)
- If it works and it's worth testing, then add it to the
- Repeat until we can no longer find any problems related to cache tags!
Comment | File | Size | Author |
---|---|---|---|
#54 | remove_content_cache_tag-2124957-54.patch | 47.56 KB | Wim Leers |
Comments
Comment #1
Wim LeersI 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.Comment #2
Wim Leers#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.
Comment #3
Wim LeersComment #4
Wim LeersI've debugged the problems in Views, see #2183017: Views doesn't bubble up rendered entities' cache tags. Consequently, I also found other cache tags + Views problems: #2183039: Views' cache tag-based caching only sets cache tags for related entities based on Views relationships, should also do so for entity reference fields.
Comment #5
Wim LeersComment #6
Wim LeersComment #7
nielsvm CreditAttribution: nielsvm commentedThis 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.
Comment #8
Wim LeersComment #9
Wim Leers#2167039: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page() is in, so now we can finally push the issues forward that fix broken/add missing cache tags! :)
Comment #10
Wim LeersThis is a more accurate title. I wonder if this shouldn't in fact be critical…
Comment #11
Wim LeersComment #12
catchYeah 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.
Comment #13
Wim LeersComment #14
Wim LeersComment #15
catchImage 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.
Comment #16
Wim LeersIf 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 acontent
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.)
Comment #17
catchComment #19
Wim LeersI'm honestly scared by how few test failures this causes — it means our test coverage in this area is very lacking…
Comment #20
sunIn 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.
Comment #21
Wim LeersProgress!
#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.
Comment #22
Wim LeersComment #23
Wim LeersLet's make all other blocks in core cacheable out of the box as well!
Comment #24
Wim Leers#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:
Feed
/Item
: #2241229: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tagsShortcut
: #2241235: Shortcut/ShortcutSet entity types should use cache tags(contact.module's
Message
type doesn't qualify: it's never rendered!)SearchPage
: #2241249: First step in making search results pages cacheable: add the associated SearchPage's cache tagShortcutSet
: merged with #2241235: Shortcut/ShortcutSet entity types should use cache tagsTour
: #2241267: Make tours set cache tagsDateFormat
: #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.Comment #25
catchOpened #2241377: [meta] Profile/rationalise cache tags.
Comment #26
Wim Leers#25: thanks for that :)
Comment #27
catch16: remove_content_cache_tag-2124957-16.patch queued for re-testing.
Comment #29
Wim LeersI need to reroll the patch here. Will do so today.
Comment #30
Wim LeersReroll 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.
Comment #32
catchThis just needed two block tests updating that were looking for content cache tags that are no longer there.
Comment #33
catchAssuming 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.
Comment #35
Wim Leers#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.
Comment #37
Wim LeersStraight reroll.
Comment #38
catchOK 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).
Comment #39
sunI guess this could use a change notice?
Comment #40
catchIt'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.
Comment #41
BerdirHm.
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:
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?
Comment #42
BerdirI 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?
Comment #43
catchHmm 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.
I think we already have those issues, most linked from the summary.
Comment #44
Wim LeersGreat 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
andcache_toolbar
.I think we have two ways to achieve this:
array('rendered' => TRUE)
or something like that, and makedrupal_render_cache_set()
set it.cache.bin.rendered
tag, with a corresponding compiler pass and adrupal_render_cache_clear()
function that simply callsdeleteAll()
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.
Comment #45
catchThis 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.
Comment #46
Berdir+1, sounds good to me :)
Comment #47
Wim LeersAlrighty! :)
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.
Comment #48
catchLooks good to me.
Comment #49
catchComment #51
Wim LeersFinally 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.
Comment #52
Wim LeersSince 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.
Comment #53
catchMinor, but the commented out assertion methods should just be removed.
Comment #54
Wim LeersThey weren't commented, only their message was commented. Restored.
Comment #55
catchSorry I mis-typed. s/methods/messages/...
As long as they're properly in or out that's fine though, thanks.
Comment #56
catchCommitted/pushed to 8.x, thanks!
Comment #58
Wim LeersLovely — and at last, I'd say :)
Comment #60
Wim LeersRelated: #2342651: Cache tags for *all* config entities.