The core cache bins have tended to be tied to specific features rather than types of stuff, 'cache' is the exception rather than the rule.

I think we should refactor this so that they reflect the type of data being used rather than which module they might be related to.

There are the following categories of things we cache:

Registries and configuration - arrays and objects that aren't tied to content. cache + cache_bootstrap are probable fine for this as is.

Rendered HTML that is tied to arbitrary combinations of content - cache_block + cache_page, entity render cache. Strings...

Caches related to multiple content items, variable cache keys: (cache_path and cache_menu, views results) - not rendered HTML. Usually cached by path but not always.

Caches attached to specific entities - entity/field cache.

If we try to distil that into cache bin naming, maybe something like this:

{cache_bootstrap} - same as now - reserved for things you may want to put in a 'local' cache like APC. If bootstrap gets seriously slimmed down by the CMI and WSCCI initiatives we might be able to merge this back into {cache}, but it's still useful at the moment.

{cache_default} - all 'global' registries and configuration - some configuration/registries in other cache bins should move here, anything content-related should move out.

{cache_render} - all cached HTML strings go here - page + block (possibly including the check_markup() cache in core). This can get very big, the items are usually expensive to build, and don't last long.

{cache_path} - all per-path (but not HTML string) caches. Similar to cache_render but for lower level stuff. This can also get very big, but the items tend to last longer (since they're lower level) than rendered stuff.

{cache_entity} - renamed field cache bin, but could potentially be used by entity caching too if that gets added to D8.

Advantages of this:

- With MySQL, items requested very often (config and registries) will be in smaller bins with less frequent updates - so less likely to accumulate overhead with MyISAM, more efficient use of the query cache (the query cache is maintained at a per-table level for expiry.

- With Memcache (and anything else that is LRU or has limited space), it'd be easier to allocate memory to instances based on usage - we'd document that cache and cache_bootstrap are expected to have a finite number of items, and that cache_render, cache_path and cache_entity are more likely to get filled up.

There may be better divisions than this, but posting up for comment.

CommentFileSizeAuthor
#106 1194136-followup-106.patch1.51 KBclaudiu.cristea
#98 interdiff-1194136-98.txt724 bytesdamiankloip
#98 1194136-98.patch71.7 KBdamiankloip
#96 1194136-96-interdiff.txt6.34 KBBerdir
#96 1194136-96.patch71.33 KBBerdir
#94 1194136-94-interdiff.txt1.03 KBBerdir
#94 1194136-94.patch65.42 KBBerdir
#92 1194136-92-interdiff.txt7.71 KBBerdir
#92 1194136-92.patch64.55 KBBerdir
#89 1194136-89-interdiff.txt2.18 KBBerdir
#89 1194136-89.patch65.2 KBBerdir
#87 1194136-87-interdiff.txt1.73 KBBerdir
#87 1194136-87.patch55.17 KBBerdir
#84 interdiff.txt937 bytesWim Leers
#84 1194136-84.patch53.62 KBWim Leers
#82 interdiff.txt9.6 KBWim Leers
#82 1194136-82.patch53.66 KBWim Leers
#77 1194136-77.patch50.19 KBdamiankloip
#76 interdiff-1194136-76.txt812 bytesdamiankloip
#76 1194136-76.patch50.17 KBdamiankloip
#74 1194136-74.patch49.38 KBdamiankloip
#72 1194136-72.patch50.96 KBdamiankloip
#70 interdiff-1194136-70.txt754 bytesdamiankloip
#70 1194136-70.patch49.78 KBdamiankloip
#62 interdiff.txt751 bytesWim Leers
#62 1194136-62.patch50.97 KBWim Leers
#58 interdiff-1194136-56.txt363 bytesdamiankloip
#58 1194136-56.patch50.97 KBdamiankloip
#54 interdiff-1194136-54.txt5.26 KBdamiankloip
#54 1194136-54.patch50.62 KBdamiankloip
#51 interdiff-1194136-51.txt3.43 KBdamiankloip
#51 1194136-51.patch50.74 KBdamiankloip
#49 interdiff-1194136-49.txt22.81 KBdamiankloip
#49 1194136-49.patch49.92 KBdamiankloip
#48 1194136-48.patch39.67 KBdamiankloip
#44 interdiff-1194136-44.txt3.11 KBdamiankloip
#44 1194136-44.patch40.28 KBdamiankloip
#42 1194136-42.patch37.16 KBdamiankloip
#39 interdiff-1194136-39.txt644 bytesdamiankloip
#39 1194136-39.patch38.54 KBdamiankloip
#36 unify-cache-bins-1194136-36.patch38.35 KBBerdir
#33 unify-cache-bins-1194136-33.patch48.76 KBBerdir
#33 unify-cache-bins-1194136-33-interdiff.txt624 bytesBerdir
#31 unify-cache-bins-1194136-31.patch48.15 KBBerdir
#31 unify-cache-bins-1194136-31-interdiff.txt3.35 KBBerdir
#29 unify-cache-bins-1194136-29-interdiff.txt2.86 KBBerdir
#29 unify-cache-bins-1194136-29.patch45.18 KBBerdir
#27 unify-cache-bins-1194136-27.patch42.32 KBBerdir
#22 unify-cache-bins-1194136-22.patch43.93 KBBerdir
#22 unify-cache-bins-1194136-22-interdiff.txt5.83 KBBerdir
#19 unify-cache-bins-1194136-19.patch38.49 KBBerdir
#17 unify-cache-bins-1194136-17.patch38.67 KBBerdir
#14 unify-cache-bins-1194136-14.patch39.56 KBBerdir
#14 unify-cache-bins-1194136-14-interdiff.txt438 bytesBerdir
#12 unify-cache-bins-1194136-12.patch39.56 KBBerdir
#12 unify-cache-bins-1194136-12-interdiff.txt1.02 KBBerdir
#10 unify-cache-bins-1194136-10.patch39.23 KBBerdir
#8 unify-cache-bins-1194136-8.patch32.26 KBBerdir
#6 unify-cache-bins-1194136-6.patch32.25 KBBerdir
#4 unify-cache-bins-1194136-4.patch30.92 KBBerdir
#4 unify-cache-bins-1194136-4-interdiff.txt805 bytesBerdir
#3 unify-cache-bins-1194136-3.patch30.39 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

It is important that we keep the cache_form data separate because there are several issues that arise when the form cache is cleared:

#500646: An unrecoverable error occurred. Form missing from server cache
#795004: An unrecoverable error occurred. This form was missing from the server cache. Try reloading the page and submitting again.
#959200: Add ability to exclude bins (tables) from memcache
#722322: This form was missing from the server cache. Try reloading the page and submitting again.
#1309928: This form was missing from the server cache
...

I would be in favor of renaming that table something that doesn't include the word cache at all (form_storage? form_states?) but most importantly I just want to make sure it is kept completely separate

R.Muilwijk’s picture

Atleast content cache should be splitted from the configuration/registries caches because they take a different cache strategy.

Berdir’s picture

Status: Active » Needs review
FileSize
30.39 KB

Getting this started.

- cache_image is gone.
- There's now cache_config. I guess this will replace large parts of cache_bootstrap, now that the system table is on it's way out. We can possibly merge the remaining stuff in cache_bootstrap with cache once that has progressed a bit more.
- Introduced the html, path, entity bins as suggested in the initial post.
- cache_form currently moved into cache_path, the form_state part will move to KV and then it's jut a normal cache, as catch described in the initial post.

No upgrade path yet.

Berdir’s picture

Forgot the #cache definition in block.module. Also removed a new duplicated cache clear. There might be more of those.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
32.25 KB

Forgot to update hook_cache_flush(), let's see how this goes.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
32.26 KB

Messed up the naming, changed 'html' to render.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
39.23 KB

Fixed some remaining manual queries, unit tests and so on. Upgrade path is a bit tricky, quite a mess currently but seems to pass locally.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-10.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
39.56 KB

Ok, found more incorrect render/html cache bins. The dblog test errors were interesting. The invalid cache flush bins resulted in exceptions in cron runs and the only place this showed up were those tests because the number of watchdog entries didn't match.

This might pass but there are still two things to figure out:

- Complete pgrade path, hopefully one that doesn't require the additional try/catch statements. Maybe we need to add some of these bins in the early fix function.
- This is blocked on #512026: Move $form_state storage from cache to new state/key-value system which in turn is blocked on the cache DIC issue. Which could benefit from this issue, because it would get the number of cache bins down.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
438 bytes
39.56 KB

Well, it would help if I wouldn't comment the cache clear calls, no wonder that test fails then ;)

Berdir’s picture

Issue tags: -Performance, -scalability

#14: unify-cache-bins-1194136-14.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +scalability

The last submitted patch, unify-cache-bins-1194136-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.67 KB

Here is a re-roll.

Haven't touche the views bins yet, should we incorporate them into this too?

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-17.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.49 KB

Upsie. Missed one merge conflict.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-19.patch, failed testing.

damiankloip’s picture

I'm not sure how views would fit into this?

(sorry if I'm telling people what they already know).

So we have two cache tables currently:

  • cache_views_data: caches everything generate from /hook_views_data(_alter)?/ hooks.
  • cache_views_results: This stores an entry for both the rendered output of a cached view, and the raw results or a cached view. So each cached view would have 2 rows in this table.

So I guess the new cache_render table could house the cached markup side of a view that's currently in cache_views_data? But then we would still want the results too, so could this live somewhere else? and could the current cache_views_data stuff just live in the general cache table? There could be quite a few entries in here if you had a site with alot of different languages, but if not, you would have one row for everything, and then one row containing the broken up data for each base table.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.83 KB
43.93 KB

Ok, this should fix the test failures in the cache tests and also removes the views_results cache bin.

I'm not sure about views_info yet either, but as suggested in the views_fetch_data() -> service issue, We might want to look into only storing the tables separately that are actually requested. That would mean that we initially have to do a few loads of the whole cache but that should stop quickly and it would save us possibly hundreds of useless cache_set()'s for sites with a few hundred fields/views tables. After that, I think it should be fine to merge views_info into the default cache bin?

The last submitted patch, unify-cache-bins-1194136-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#22: unify-cache-bins-1194136-22.patch queued for re-testing.

Berdir’s picture

#22: unify-cache-bins-1194136-22.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +scalability

The last submitted patch, unify-cache-bins-1194136-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
42.32 KB

All the issues this depended on finally went in I think, here's an initial re-roll.

This will very likely fail all over the place as a lot of changes happened since the last patch.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-27.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
45.18 KB
2.86 KB

This should fix the installer.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.35 KB
48.15 KB

This should fix most failures, not sure sure about book but too lazy to install again just to run that locally ;)

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-31.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
624 bytes
48.76 KB

This should fix the last test fail.

Berdir’s picture

Issue tags: -Performance, -scalability

#33: unify-cache-bins-1194136-33.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +scalability

The last submitted patch, unify-cache-bins-1194136-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.35 KB

Re-roll, patch got a lot easier due to the removal of the cache table schema definitions just a bit ugly to re-roll :)

Updated the update function to remove the cache tables, will need to do the same for filter, block and views_info cache bins.

jibran’s picture

Issue tags: -Performance, -scalability

#36: unify-cache-bins-1194136-36.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, unify-cache-bins-1194136-36.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
38.54 KB
644 bytes

Sorry.

damiankloip’s picture

Status: Needs review » Needs work

Sorry, totally wrong patch. IGNORE

The last submitted patch, 39: 1194136-39.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
37.16 KB

Let's try and start this again. It's been such a long time, I didn't bother with a reroll, just started again. Too much stuff has changed in the mean time.

Status: Needs review » Needs work

The last submitted patch, 42: 1194136-42.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
40.28 KB
3.11 KB
catch’s picture

Issue summary: View changes
catch’s picture

Issue summary: View changes
catch’s picture

Looking at the list of bins, we're missing a place for the Views results cache.

This most closely maps to the things in cache_path - menu links, path alias pre-load lists etc. are collections of entities by some kind of criteria, which later get loaded and rendered, which is similar to views results. Difference is that views results aren't per-page.

Might just need renaming - cache_listing or cache_collection could be OK. Not much will use this.

damiankloip’s picture

FileSize
39.67 KB

Just a reroll for now. Will work on this a bit more today.

damiankloip’s picture

FileSize
49.92 KB
22.81 KB

Ok, made a few more changes after speaking to catch:

- Renamed path cache bin to data
- Removed views_info cache bin, this stuff now lives in data
- Views result cache now also uses data bin

Status: Needs review » Needs work

The last submitted patch, 49: 1194136-49.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
50.74 KB
3.43 KB

Missed a couple. This should pass now.

Berdir’s picture

  1. +++ b/core/includes/menu.inc
    @@ -579,8 +579,8 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
    +      // If the static variable doesn't have the data, check {cache_path}.
    +      $cache = \Drupal::cache('data')->get($cid);
    

    Stale comment...

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
    @@ -134,7 +134,7 @@ protected function loadFieldItems(array $entities) {
             $cids[] = "field:{$this->entityTypeId}:$id";
           }
    -      $cache = \Drupal::cache('field')->getMultiple($cids);
    +      $cache = \Drupal::cache('entity')->getMultiple($cids);
    

    This is in line with what I'm doing in #597236: Add entity caching to core.

  3. +++ b/core/modules/field/field.services.yml
    @@ -1,5 +1,5 @@
       field.info:
         class: Drupal\field\FieldInfo
    -    arguments: ['@cache.field', '@config.factory', '@module_handler', '@plugin.manager.field.field_type', '@language_manager']
    +    arguments: ['@cache.entity', '@config.factory', '@module_handler', '@plugin.manager.field.field_type', '@language_manager']
    

    FieldInfo is about field metadata, not sure if this should be in the entity bin, would be nice to only use that for the entity storage.

    Looking at what it stores, it's up to 3 small caches (fields/instances/extra fields) per entity_type/bundle.

    Not sure if we want it in cache.cache?

  4. +++ b/core/modules/views/views.module
    @@ -492,24 +492,30 @@ function views_language_list($field = 'name', $flags = Language::STATE_ALL) {
    +  \Drupal::cache('data')->deleteAll();
    +  // @todo: Is this necessary?
    +  \Drupal::cache('data')->deleteAll();
    +  \Drupal::cache('render')->deleteAll();
    

    Well, at least it shouldn't be necessary to delete it twice :)

Applying the patch, manually dropping all cache tables in the database, core/rebuild.php => all well. Pretty cool :)

Some notes based on that:
- The list of cache bins looks really nice, way better than right now.
- What about cache_toolbar? Was not mentioned in here before, it creates a cache per user, so on a site with many users that have access to the toolbar, that could be quite a list, but not sure if that deserves it's on cache bin for that?
- cache contains per template cache entries that contain nothing more than a timestamp if I understand that correctly to compare the file changed timestamp. Should probably live in a different bin, also sounds like a job for a cache collector (not something to worry about here). Not that problematic, as it's only used if auto_refresh is turned on, which you want to disable on production...
- cache_bootstrap is down to 3 entries, hook_info, module_implements, and system_list. Seems pretty useless :)

Trying to understand a bit more what's going on, so added the following line to DatabaseBackend::getMultiple():

print_r($this->bin . ': ' . implode(', ', $cids) . "\n");

Gives:

cache_config: system.module
cache_config: system.performance
cache_bootstrap: module_implements
cache_config: system.file
cache_config: system.filter
cache_config: system.date
cache_config: devel.settings
cache_config: system.site
cache_bootstrap: system_list
cache_config: system.theme
cache: entity_type:en
cache_config: node.settings
cache_config: views.view.frontpage
cache_config: user.settings
cache_data: views:display_plugins:en
cache_config: views.settings
cache_data: views:unpack_options:558eede105bba71476073d6f42fba854d68ae52ae53e9ee86aa54be404bee11b:en
cache_data: views:unpack_options:3707432e1e53263798611c2f6e975bfa4b1758b0500363530a07575468c53776:en
cache_data: views_data:node_field_data:en
cache_data: views:sort_plugins
cache_data: views:filter_plugins
cache_data: views_data:views:en
cache_data: views:area_plugins
cache_data: views_data:node:en
cache_data: views:pager_plugins:en
cache_data: views:query_plugins:en
cache_data: views:join_plugins
cache_data: views:style_plugins:en
cache_data: views:row_plugins:en
cache_data: views:unpack_options:2644cffbe051b3708c113121b23bb3cfd1d21422bd56475c60c725110e193059:en
cache: path_alias_whitelist
cache: theme_registry:runtime:bartik
cache_data: views:cache_plugins:en
cache: schema
cache: entity_base_field_definitions:node:en
cache: entity_bundle_field_definitions:node:article:en
cache_config: find:field.field.
cache_config: field.field.comment.comment_body, field.field.custom_block.body, field.field.node.body, field.field.node.comment, field.field.node.field_image, field.field.node.field_number, field.field.node.field_tags, field.field.user.user_picture
cache: field_types_plugins:en
cache: typed_data_types_plugins:en
cache_entity: field:node:1
cache_entity: field_info:field_map
cache_data: views:exposed_form_plugins:en
cache_config: entity.view_display.node.article.teaser, entity.view_display.node.article.default
cache_entity: field_info:bundle_extra:en:node:article
cache: field_formatter_types_plugins:en
cache: entity_base_field_definitions:user:en
cache: entity_bundle_field_definitions:user:user:en
cache_entity: field:user:1
cache_config: rdf.mapping.node.article
cache_config: filter.format.basic_html
cache: entity_base_field_definitions:file:en
cache: entity_bundle_field_definitions:file:file:en
cache: entity_base_field_definitions:taxonomy_term:en
cache: entity_bundle_field_definitions:taxonomy_term:tags:en
cache_entity: field:taxonomy_term:1, field:taxonomy_term:2, field:taxonomy_term:3, field:taxonomy_term:4, field:taxonomy_term:5, field:taxonomy_term:6
cache: entity_view_mode_info:en
cache_bootstrap: hook_info
cache: entity_view:node:1:teaser:bartik:r.authenticated,administrator
cache_entity: field_info:bundle:fields:node:article
cache_entity: field_info:bundle:instances:node:article
cache: library:info:views
cache_config: find:block.block.
cache_config: block.block.bartik_breadcrumbs, block.block.bartik_content, block.block.bartik_footer, block.block.bartik_help, block.block.bartik_login, block.block.bartik_powered, block.block.bartik_search, block.block.bartik_tools, block.block.seven_breadcrumbs, block.block.seven_content, block.block.seven_help, block.block.seven_login
cache_render: block_plugins:en
cache_data: links:tools:page:node:en:1:0
cache_data: links:tools:tree-data:en:9bd1605e2280833450478f9083b7f8714c2fa28f1012455e2744e5af1a13eec5
cache_config: find:node.type.
cache_config: node.type.article, node.type.page
cache_config: search.page.node_search
cache: search_plugins:en
cache_config: search.settings
cache_data: links:footer:page:node:en:1:0
cache_data: links:footer:tree-data:en:9bd1605e2280833450478f9083b7f8714c2fa28f1012455e2744e5af1a13eec5
cache_config: system.theme.global
cache_config: bartik.settings
cache_config: menu.settings
cache_data: links:main:page:node:en:1:1
cache_data: links:main:tree-data:en:9ec01ec58bf82a695e4acd636af283e0585fe8cd8a6e54eb140188a3e284ab1c
cache_data: links:account:page:node:en:1:1
cache_data: links:account:tree-data:en:9ec01ec58bf82a695e4acd636af283e0585fe8cd8a6e54eb140188a3e284ab1c
cache_config: breakpoint.breakpoint_group.module.toolbar.toolbar
cache_config: breakpoint.breakpoint.module.toolbar.narrow
cache_config: breakpoint.breakpoint.module.toolbar.standard
cache_config: breakpoint.breakpoint.module.toolbar.wide
cache_config: shortcut.set.default
cache: entity_base_field_definitions:shortcut:en
cache: entity_bundle_field_definitions:shortcut:default:en
cache_data: links:admin:tree-data:en:1fc7d31ce109d9a9ded6a75e0609246fb57912778604f63949beb6bc7bccfbaf
cache_toolbar: toolbar_1:en
cache: library:info:toolbar
cache: library:info:core
cache: library:info:shortcut
cache: library:info:contextual
cache: library:info:tour
cache: library:info:user
cache: local_task_plugins:en:view.frontpage.page_1
cache: local_action_plugins:en
cache_config: color.bartik
cache_config: find:tour.tour.
cache_config: tour.tour.views-ui
cache: library:info:edit
cache_bootstrap: system_list
cache: library:info:filter
cache: library:info:system

(Why are we caching libraries by module?!)

tim.plunkett’s picture

The cached block annotations are not "render" information, IMO.

damiankloip’s picture

FileSize
50.62 KB
5.26 KB

Thanks for the reviews.

The cached block annotations are not "render" information, IMO.

Totally, this was in error. Was cache.block > C&P > cache.render :) We want this in either cache.cache or cache.data.

(Why are we caching libraries by module?!)

afaik, drupal_get_library() was always per module, but the conversion to yaml added the caching layer too for this. Sun has an issue to consolidate this. We have identified that this is not cool :)

I am not sure about cache_toolbar right now. This table could easily get waaaay big. So I feel like we should leave it as its own bin for now? and maybe discuss any alternatives once we have this in? Just to try and keep the scope of this issue in a bit.

One other question, I currently have cached info data, whether this is info, or cache plugin definitions, split between cache.cache and cache.data. I feel like maybe I should just move all of these to cache.cache for consistency? thoughts on that?

Patch contains those changes from above.

Status: Needs review » Needs work

The last submitted patch, 54: 1194136-54.patch, failed testing.

Wim Leers’s picture

RE: cache_toolbar: I think the only reason the Toolbar module has its own cache, and its own (peculiar) caching technique, is because the underlying caching problem in the menu system hasn't been solved yet: #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees. Once that is solved, Toolbar pretty much can get rid of its own cache. Again, AFAIK/IIRC.

Sadly, #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees hasn't gone anywhere in the past half year. Should it be a critical instead of a major? Should it be a beta blocker? I don't know.

Berdir’s picture

I am not sure about cache_toolbar right now. This table could easily get waaaay big. So I feel like we should leave it as its own bin for now? and maybe discuss any alternatives once we have this in? Just to try and keep the scope of this issue in a bit.

That's what I thought at first too. (waaay big). But cache_entity is going to have a row for every single loaded entity, now cache_toolbar seems pretty small :)

Re huge cache_entity, That's something I'm wondering about in #597236: Add entity caching to core, if we should use a bin per entity instead, but that's currently problematic, while we could easily get it dynamically from the cache factory, we currently can not clean it up like we can the others? maybe do that in entity_module_preuninstall() ?

Anyway, I'm fine leaving that, especially if it's hopefully only temporary/a workaround.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
50.97 KB
363 bytes
catch’s picture

entitycache in contrib has a bin-per-entity exactly to avoid them getting out of hand. That's worth doing but could definitely be its own issue.

toolbar is somewhere between cache_render and cache_data, but yes it's only there because of the other issue. I'd be fine with making that critical.

damiankloip’s picture

ok, so we should leave this as cache entity for now then? We can then have a follow up when the entity cache issue and this is in?

Toolbar can be left for now, and fixed in said issue above.

This just leaves the question of info, should we make this more iniform and put say all of them in cache.cache?

catch’s picture

ok, so we should leave this as cache entity for now then? We can then have a follow up when the entity cache issue and this is in?

Yep. Also it's no worse than the field cache in 7.x which was for all entities.

Info - most of those should really be in cache_bootstrap based on the intention of that bin - that was supposed to be for things that are a requirement for any full page request and so they can live somewhere that gets less polluted than the default bin.

Wim Leers’s picture

FileSize
50.97 KB
751 bytes

Reviewed for potential RTBC; found one mistake (rendered blocks are now stored in cache_render, but the wrong service was being injected: @cache.cache instead of @cache.render.

And found these:

+++ b/core/modules/views/views.module
@@ -492,24 +492,27 @@ function views_language_list($field = 'name', $flags = Language::STATE_ALL) {
+  // @todo: Is this necessary?
...
+  // @todo: Is this necessary?
...
+  // @todo: Is this necessary?

Which still needs to be addressed, or get a linked follow-up issue.

Wim Leers’s picture

damiankloip’s picture

Which still needs to be addressed, or get a linked follow-up issue.

Yes, these were added in by this patch. I get the feeling these will need to utilise cache tags. As clearing out whole bins like this is not that helpful. when a field is changed or something.

catch’s picture

Bumped #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

On those views clears, there shouldn't be any need to clear views results just because a field instance was created. I can see views info caches needing clearing but that's very different.

damiankloip’s picture

Well, before (currently) both result and info cache gets cleared out. Or are you saying we definitely don't need this for the created case in particular?

catch’s picture

I don't see why we'd clear out result - how can new field instances invalidate existing views?

damiankloip’s picture

That's what I'm saying; you mean just ditch this from views_field_instance_config_create()? :)

catch’s picture

Yep :)

damiankloip’s picture

FileSize
49.78 KB
754 bytes

rerolled and removed the create hook implementation.

Status: Needs review » Needs work

The last submitted patch, 70: 1194136-70.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
50.96 KB

Sorry, rebase/branch fail.

Berdir’s picture

I added a @todo to #2216543: Fill in topic/@defgroup docs for Cache overview to add documentation about common cache bins and for what they should be used, I hope that will land asap (reviews welcome). The cache API really deserves some accurate and up to date documentation :)

damiankloip’s picture

FileSize
49.38 KB

Needs another reroll after the cache tags testing patch.

Status: Needs review » Needs work

The last submitted patch, 74: 1194136-74.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
50.17 KB
812 bytes

Forgot to change the cache bin in the new cache tags base test class.

damiankloip’s picture

FileSize
50.19 KB

Another reroll...

catch’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready.

One extra thing we could do is rename the 'cache' bin to 'cache_default', would mean cache('default') instead of cache('cache') but separate issue I think.

Berdir’s picture

+1 to all of that. RTBC, cache_default and doing that in a separate issue :) Defaulting to 'default' is way less confusing than 'cache' and we don't need to special case 'cache' anymore.

catch’s picture

Wim Leers’s picture

Thanks for this. I love the clean-up :)

My only concern is that many comments haven't been updated and are therefore now confusing/misleading. E.g.: The menu cache object for this controller. in BookAdminEditForm, The page cache object. in PerformanceForm, etc.

Wim Leers’s picture

FileSize
53.66 KB
9.6 KB

Comments updated!

I wanted to rename the comment in BookAdminEditForm but I noticed that all of that cache clearing was in fact unnecessary: BookManager::saveBookLink() and Node::save() already invalidated the affected cache tags. And the static reset for book_menu_subtree_data was entirely ludicrous, since that function does not exist anymore!


While updating comments, I noticed something. This patch highlights in how many places we're still doing "let's clear all the things in the bin" instead of using cache tags to only clear affected cache entries. With the move towards fewer cache bins in this patch, that problem only grows in importance.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 82: 1194136-82.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
53.62 KB
937 bytes

#2216543: Fill in topic/@defgroup docs for Cache overview broke this. Cache docs have moved to core.api.php, hence the interdiff.

Berdir’s picture

Yep, sorry :)

Any chance that we can resolve the @todo in there that points to this issue about providing some documentation on the common cache bins and what they should be used for? I'm also OK with doing that on d.o if it's considered to be too much in-depth, but then we should create a stub page on d.o below Cache API and link to it.

Status: Needs review » Needs work

The last submitted patch, 84: 1194136-84.patch, failed testing.

Berdir’s picture

Can't reproduce the installation fail, updated documentation a bit and fixed a @todo in EntityViewBuilder pointing to this issue. This might introduce some new fails that expect the cache.cache bin to be used.

Status: Needs review » Needs work

The last submitted patch, 87: 1194136-87.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
65.2 KB
2.18 KB

Looks like only Wim's new cache tag tests care about which bin this is stored in ;)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +D8 cacheability

Thanks for the rerolls! Nice to see the @todo that was blocked on this issue resolved in #87.

Back to RTBC per #78.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: 1194136-89.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
64.55 KB
7.71 KB

Re-roll after #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags, less conflicts than I feared (just one). Let's see if I found all the new cache usages.

Status: Needs review » Needs work

The last submitted patch, 92: 1194136-92.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
65.42 KB
1.03 KB

Ah, I assumed that I missed at least one reference to the block cache but couldn't find it.

Status: Needs review » Needs work

The last submitted patch, 94: 1194136-94.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
71.33 KB
6.34 KB

Ah, defaulting the render cache to use 'render' was not yet done here, but makes a lot of sense I think. Fixed the second place where it defaulted to 'cache' and using 'render' in the tests.

Status: Needs review » Needs work

The last submitted patch, 96: 1194136-96.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
71.7 KB
724 bytes

Needed reroll, and fixed test fail.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, missed one more. Thanks, I think this can go back to RTBC now.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit bb0f5fc on 8.x by catch:
    Issue #1194136 by Berdir, damiankloip, Wim Leers: Re-organise core cache...
Wim Leers’s picture

Awesome! :)

Status: Fixed » Closed (fixed)

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

claudiu.cristea’s picture

The only problem here is that core provides also a non-bin cache table: cache_tags. And that has the same name pattern as the bins. If you manually truncate the DB cache you may accidentally truncate also cache_tags (I did it!). So, IMO, cache bins should have a different name pattern, like cache_bin_*.

Wim Leers’s picture

cache_tags is an implementation detail of the database-powered cache back-end. But I agree with the rationale in #104; right now it's relatively confusing.

claudiu.cristea’s picture

Priority: Normal » Minor
Status: Closed (fixed) » Needs review
FileSize
1.51 KB

Then, here's a follow-up patch making DB cache bin naming more clear. Changed also the priority to 'Minor'.

Berdir’s picture

Can you open a new issue for this? There's no reason to re-open this long-fixed issue for this, which is really a new task, and not a bugfix or so.

claudiu.cristea’s picture

Priority: Minor » Normal
Status: Needs review » Closed (fixed)

@Berdir

Yes, sure. Sorry.

It's here as a followup: #2302843: {cache_tags} conflicts with cache bin database table names