Updated: Comment 0

Problem/Motivation

Proposed resolution

Note: We don't leverage cache tags on the actual executing level, so views still executes the view.

Remaining tasks

User interface changes

API changes

Original report by @username

I suspect a bunch of cache handling in views is broken. There have been a few changes in core that have not yet been included in views. Especially cache tag stuff.

Attached is a patch that at least fixes a hook call :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Committed this part, because it is simply fine. Thanks!

Regarding the cache: views already uses cache($bin)->get(), i guess this is fine.
Regarding tags, it would maybe make sense to port the dynamic creation of cache keys to replace with tags.
As far as i understand tags it does simply the same kind of stuff but it would be easier.

aspilicious’s picture

Issue tags: +VDC

adding tag

aspilicious’s picture

After grepping views I found one problematic file:

views\handlers\views_handler_relationship_groupwise_max.inc

That file uses cache_clear_all, cache_get and cache_set. These should get converted with cache_tags (so we can replace cche_clear_all with cache_invalidate(...).

Not 100% sure how to do this so leaving to others.

aspilicious’s picture

This should do it I think

aspilicious’s picture

wrong patch

dawehner’s picture

Title: Cache handling has changed a lot in drupal 8 » How to leverage cache tags in d8
Status: Needs review » Active

This part is fine, thanks!
lets figure out whether/how to use cache tags

damiankloip’s picture

Are cache tags going to be that useful here, with a relationship plugin that can be used for lots of different things?

moshe weitzman’s picture

Interesting issue! In general, the cache tags that should be written relate to the ids that are returned by the query. So we want to return something like array('node' => array(8,12,16,...) as cache tags. Maybe we return a maximum of 500 ids? I'll ping catch to chime in here.

Some related nice-to-have issues:

  1. #1748164: Use #attached to add all CSS and JS. I'd love for Views to return cache tags as part of a larger render array that implements #cache, including cache tags.
  2. [#1605290: Enable entity render caching with cache tag support. Core issue for adding cache_tag support to render cache processing
catch’s picture

For cache tags and views, I think there's two bits:

1. Cache entity IDs that appear in the cached, rendered HTML (so if there's a pager showing ten items per page, the ten entity IDs on that page). This should more or less replace views content cache - at least in the sense that if one of those entities is updated or deleted, the cache will be invalidated.

The only thing required then is getting the entity IDs out of the view and into the cache tags. If we have a new nested rendering model with blocks/layouts, then ideally the individual entity view mode cache tags would filter up to the rendered output of the whole view, and Views wouldn't actually need to do any of this (assuming it's using standard rendering). Until that's in place though, Views could do a simple implementation.

2. (much harder) tag the view itself with cache tags that affect the query results. So if a view lists nodes of type article, have a cache tag that is "$entity_type-$bundle" and tag the view with it, invalidating that tag when a matching entity to the tag definition is saved. This requires conventions about how tags are named/invalidated which we don't have in core yet. Figuring out which possible cache tags might affect a view is definitely Views' job, defining which tags get invalidated when entities are saved I'm not sure exactly where the responsibilities lie for that yet.

dawehner’s picture

Regarding the first point, this kind of solved already, because we have methods which return the used entities on the view.

The second one is indeed much harder, for example if you have a bundle filter, you should only add the cache tag if the bundle filter is not exposed for users, as there might be to granular caching. I kind of feel that this is not possible in a satisfying way,
so in the end people have to write custom code to get the full caching power.

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
DamienMcKenna’s picture

I'd recommend also tracking the bundle so that the cache for all objects of that type can be flushed when the bundle is modified.

moshe weitzman’s picture

Title: How to leverage cache tags in d8 » How to leverage cache tags in Views
Issue tags: +D8 cacheability

#1748164: Use #attached to add all CSS and JS is in so this is not blocked, AFAIK. Views still does a drupal_render() internally so it isn't perfect but I think we should proceed with adding tags even if the tags don't bubble all the way up the page.

dawehner’s picture

2. (much harder) tag the view itself with cache tags that affect the query results. So if a view lists nodes of type article, have a cache tag that is "$entity_type-$bundle" and tag the view with it, invalidating that tag when a matching entity to the tag definition is saved. This requires conventions about how tags are named/invalidated which we don't have in core yet. Figuring out which possible cache tags might affect a view is definitely Views' job, defining which tags get invalidated when entities are saved I'm not sure exactly where the responsibilities lie for that yet.

It seems to be that we really have to make this bit swappable, because even we try it hard, there seems to be now way to do it right for all usecases?

ordermind’s picture

I remember back in the D6 days that using the WHERE part of the views query as a basis for determining which entities are involved in the view worked pretty well. For example a view would list nodes of a certain type, and then when a node of that type was created, updated or deleted, the view cache would be invalidated.

damiankloip’s picture

Status: Active » Needs review
FileSize
5.08 KB

This still needs work, but I think this might be the right direction (**think).

Status: Needs review » Needs work

The last submitted patch, 1712456-16.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
amateescu’s picture

Discussed in Prague that we could also add a tag with the entity bundle so we don't clear every view by entity type when an entity is changed, and here's an interdiff that adds support for invalidating the bundle tag in EntityRenderController.

damiankloip’s picture

FileSize
3.02 KB
10.62 KB

Merged in amateescu's patch, added a quick test for the getEntityTypes() method.

Also added a Tag cache plugin, so cache items will only be invalidated based on tags, and persist until that happens.

Still needs some more work, and tests.

dawehner’s picture

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/Tag.php
    @@ -0,0 +1,37 @@
    + *
    + * @ViewsCache(
    + *   id = "tag",
    + *   title = @Translation("Tag based"),
    + *   help = @Translation("Tag based caching of data. Caches will persist until any related cache tags are invalidated.")
    + * )
    + */
    

    I am wondering how this would work with a listing which has different results depending on new content ... can we somehow support the usecase?

  2. +++ b/core/modules/views/lib/Drupal/views/ViewExecutable.php
    @@ -2225,4 +2225,34 @@ public function buildThemeFunctions($hook) {
    +    // Initialize handlers so we will have any relationships
    +    $this->initHandlers();
    +    // Start with the base table.
    +    $entity_types = array();
    +    $views_data = Views::viewsData();
    +
    +    // Add the base table.
    +    $base_table_data = $views_data->get($this->storage->get('base_table'));
    +    if (isset($base_table_data['table']['entity type'])) {
    +      $entity_types[] = $base_table_data['table']['entity type'];
    +    }
    +
    +    // Include all relationships.
    +    foreach ($this->relationship as $relationship) {
    +      $table_data = $views_data->get($relationship->definition['base']);
    +      if (isset($table_data['table']['entity type'])) {
    +        $entity_types[] = $table_data['table']['entity type'];
    +      }
    +    }
    +
    +    return array_values(array_unique($entity_types));
    

    We could simplify the code a lot by using Query->getEntityTables() ... maybe we should try to reorganize some of the code.

damiankloip’s picture

FileSize
6.79 KB
14.13 KB

So, if we have an ENTITY_TYPE_list_BUNDLE tag that gets added to views cache entries, this can get invalidated by the entity render controller resetCache() method, which will get invoked during postSave().

After our discussion, I've changed getEntityTables() to getEntityInformation() and refactored it a bit.

So I guess some questions:
- Do we make it so that the tags are only generated when a view uses fields? or certain tags anyway? A in theory, rendered entities should generate their own tags that would bubble up, so we wouldn't need to generate these twice.
- Cache tags like this only really work with entities so how do we deal with this in the UI? Just some clear descriptions that this will only invalidate stuff based on entities? Conditionally showing this plugin is probably a no go too, as you could have a base table with no entity support that creates relationships to entities etc.. this is pretty dynamic.

Status: Needs review » Needs work

The last submitted patch, 1712456-22.patch, failed testing.

Wim Leers’s picture

in theory, rendered entities should generate their own tags that would bubble up, so we wouldn't need to generate these twice.

This is only true when using Views that use the Entity/Field API render pipeline, isn't it? I.e. when using table views, Views would have to generate cache tags.

Cache tags like this only really work with entities so how do we deal with this in the UI?

AFAICT Views in core only offers two caching options: "none" and "time-based". I think/fear you're right, which means that we'd need to present entity-based cache invalidation not as a third option (a third radio button in the UI), but something entirely separate?
OTOH, do we even need to provide a UI option for this? Why wouldn't we always want to enable this? Which means the UI could maybe have some text along the lines of "Note that Views' caching will always automatically react to entity changes."

Also, does Views have enough metadata to know whether *all* its contents are entity-based? If so, we can make smart decisions and enable entity-based caching by default.

Finally: note that individual entities can disable render caching, so Views probably needs to deal with that too.

damiankloip’s picture

This is only true when using Views that use the Entity/Field API render pipeline, isn't it? I.e. when using table views, Views would have to generate cache tags.

Yes, see previous comment: "Do we make it so that the tags are only generated when a view uses fields? " - So we can tell easily when a view is using field based rows or not. Our results will also contain the loaded entities too, so it would only use these anyway.

AFAICT Views in core only offers two caching options: "none" and "time-based".

Correct, this patch currently adds a third for tag based caching only (as well as for time based as it would still need to invalidate in that case) but as you point out, this could lead to issues with particular views that do not use any entities atm.

Also, does Views have enough metadata to know whether *all* its contents are entity-based? If so, we can make smart decisions and enable entity-based caching by default.

Yes, the current patch already takes care of that, we can use views' data to determine all the various entities used in the view, from the results (entities and relationship_entities).

Finally: note that individual entities can disable render caching, so Views probably needs to deal with that too.

This currently is not taken care of in the patch, but neither is any checking for field based views, so tags would be generated all the time currently. We could check this in the entity info aswell though, that shouldn't be a problem. I guess we could get into a few issues with some entities having render caching and some not..

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/Tag.php
@@ -0,0 +1,37 @@
+class Tag extends CachePluginBase {
...
+  protected function cacheExpire($type) {
+    return FALSE;
+  }

So does that mean that we basically can't support query caching at all? This makes it kind of pointless for many many usecases. Should we maybe allow to combine time for query and tags for actual rendering?

DamienMcKenna’s picture

Please excuse me for not knowing all of the APIs so I may be unaware of something, but won't getCacheTags() lead to the same item being added multiple times to the $tags list? Shouldn't $tags[$type] be filtered for duplicates?

damiankloip’s picture

Do you mean from the relationship entities?

DamienMcKenna’s picture

@damienkloip: Yes. Never mind occasions those crazy views where resultant objects reference each other.

chrisjlee’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.phpundefined
@@ -327,6 +325,44 @@ public function generateOutputKey() {
+      $tags[$this->view->storage->id()] = TRUE;
+      // Add an ENTIY_TYPE_view tag for each entity type used by this view.

#25 - @damiankloip, I think entity is misspelled unless i'm missing something here (forgive me i'm just lurking; trying to get up to speed.)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
14.59 KB

Made those changes.

Daniel, not sure what to do about the query caching atm, as we don't support that at all really. So that should be tackled if/when we add that?

Status: Needs review » Needs work

The last submitted patch, 1712456-31.patch, failed testing.

xjm’s picture

Issue tags: +Prague Hard Problems
dawehner’s picture

I think that a cache plugin without a query cache is basically dump.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.45 KB
17.52 KB

This should get tests passing again, now the entities are loaded again...

I spoke to Daniel on IRC, he was getting query cache and results cache mixed up, this patch still supports results cache like before. Adding query caching is another issue altogether :)

Status: Needs review » Needs work

The last submitted patch, 1712456-35.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
16.82 KB

We don't have the ViewExecutable::getEntityTypes() method anymore...

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/Tag.php
@@ -0,0 +1,37 @@
+class Tag extends CachePluginBase {
...
+   */
+  protected function cacheExpire($type) {
+    return FALSE;
+  }

With this we have an infitive time until the caches of the result change. What about writing the class in a way that it either extends the time one, or accepts an existing entry and just wraps it with the cache expire logic needed here?

damiankloip’s picture

FileSize
9.33 KB
24.52 KB

Here is a new patch with some quick web tests, and a couple of other changes/fixes that were needed.

Status: Needs review » Needs work
Issue tags: -VDC, -D8 cacheability, -Prague Hard Problems

The last submitted patch, 1712456-38.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

39: 1712456-38.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 39: 1712456-38.patch, failed testing.

damiankloip’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
24.48 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 43: 1712456-43.patch, failed testing.

damiankloip’s picture

This needs a bit of fixing. Leave it with me.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
25.58 KB
3.24 KB

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 46: 1712456-46.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
28.19 KB
3.3 KB

Reroll and a couple of fixes.

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -264,11 +264,14 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +        $tags[$this->entityType . '_view_' . $entity->bundle()] = TRUE;
    +        $tags[$this->entityType . '_list_' . $entity->bundle()] = TRUE;
    

    The *_view_$bundle tag is not set anywhere and I commented on *_list_$bundle below as I don't think it brings any benefit.

    Also, it's weird for the generic view builder to clear tags that it doesn't know about (since they are set by Views).

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
    @@ -79,10 +79,9 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    -  public function resetCache(array $ids = NULL);
    +  public function resetCache(array $entities = NULL);
    

    It's not clear from the patch why this change is necessary..

  3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
    @@ -326,6 +325,50 @@ public function generateOutputKey() {
    +    $tags['view'][$id] = $id;
    

    A 'view' tag seems to be too generic, how about 'views_view' instead?

  4. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
    @@ -326,6 +325,50 @@ public function generateOutputKey() {
    +          $tags[$type . '_list_' . $result->_entity->bundle()] = TRUE;
    

    I'm not sure this tag brings any benefits, we don't have code that clears render cache by entity bundle..

damiankloip’s picture

The *_view_$bundle tag is not set anywhere and I commented on *_list_$bundle below as I don't think it brings any benefit.

Yeah, this isn;t implemented yet, it seems we should implement bundles though? Say you save settings for one node type, it would invalidate caches for every node on the system?! So I guess nowhere does this now but I think we should start to.

Also, it's weird for the generic view builder to clear tags that it doesn't know about (since they are set by Views).

We need a way like this to invalidate 'lists' containing this entity type. This and 'view' could be similar though I guess.

It's not clear from the patch why this change is necessary..

Erm, see #19 for that ;)

A 'view' tag seems to be too generic, how about 'views_view' instead?

Don't all other tags just go with entity_type => array(id => id) too?

damiankloip’s picture

FileSize
31.69 KB
12.22 KB

Ok, thanks amateescu. I think I've implemented your points; getEntityInformation is now called getEntityTableInfo (much better imo!), and I've removed the '_list_' tags.

Also, added a couple more test cases, as cacheFlush() was actually broken, as well as the filter on the view (the new test assertions cover this too, by using an article type).

amateescu’s picture

Awesome! I'll wait for the testbot before rtbc-ing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -371,16 +371,16 @@ public function referencedEntities() {
    -      $referenced_entity_ids[$referenced_entity->entityType()][$referenced_entity->id()] = TRUE;
    +      $referenced_entity_ids[$referenced_entity->entityType()][$referenced_entity->id()] = $referenced_entity;
    ...
    -        \Drupal::entityManager()->getViewBuilder($entity_type)->resetCache(array_keys($entity_ids));
    +        \Drupal::entityManager()->getViewBuilder($entity_type)->resetCache($entities);
    

    I am confused. So we do name the variable entity_ids, but we store full entities (according to referencedEntities().

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
    @@ -79,10 +79,9 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +   * @param array|null $entities
    +   *   (optional) If specified, the cache is reset for the given entities  only.
    

    What about using \Drupal\Core\Entity\EntityInterface[] instead?

  3. +++ b/core/modules/views/lib/Drupal/views/Entity/View.php
    @@ -278,6 +279,8 @@ public function getExportProperties() {
    +    $id = $this->id();
    +    Cache::deleteTags(array('view' => array($id => $id)));
    

    I wonder whether we should call it on postDelete as well. Additional I ask myself whether the entity system should clear tags for entity types automatically.

  4. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/CacheTagTest.php
    @@ -0,0 +1,203 @@
    +   * @var \Drupal\node\NodeViewBuilder
    ...
    +   * @var \Drupal\Core\Entity\EntityViewBuilder
    ...
    +   * @var \Drupal\node\Entity\Node[]
    ...
    +   * @var \Drupal\node\Entity\Node
    

    Do we typehint on concrete entity classes or just on the interfaces?

  5. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.php
    @@ -2,11 +2,12 @@
    - * Contains \Drupal\views\Tests\ViewExecutableUnitTest.
    + * Contains \Drupal\views\Tests\ViewExecutableTest.
    ...
    +use Drupal\views\Views;
    
    @@ -35,7 +36,7 @@ class ViewExecutableTest extends ViewUnitTestBase {
    -  public static $testViews = array('test_destroy', 'test_executable_displays');
    +  public static $testViews = array('test_destroy', 'test_executable_displays', 'test_view');
    

    I don't get why this change is needed, sorry.

Status: Needs review » Needs work

The last submitted patch, 51: 1712456-51.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
32.11 KB
4.51 KB

Thanks! I've made those changes. You're right, the postDelete needs to be added too.

I also added a todo, that we could remove that code if views ever implements view builders.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this :)

dawehner’s picture

Issue summary: View changes
webchick’s picture

Assigned: Unassigned » catch

This is most likely best for catch to review.

Dries’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. This could make a big difference on performance. Hard to measure right now though.

dawehner’s picture

+ *   help = @Translation("Tag based caching of data. Caches will persist until any related cache tags are invalidated.")

Please tell people that this is horrible for any view which has changing entries (feels like every view), as they will just not expire at all.

dawehner’s picture

Status: Fixed » Needs work
Issue tags: +VDC

.

damiankloip’s picture

Not sure why you're bringing this up at this point.

Let's catch up about this at some point, either way I think this should be a followup. Not re opening this one With the front of a string change... :)

dawehner’s picture

The actual problem is that the committer was probably not aware how limited this feature actually is.

damiankloip’s picture

I would like to think we can iterate on this now in some ways and improve this feature as we start using it more. If we are clever about our invalidation strategies, this will be very useful.

catch’s picture

I was aware of this limitation, not sure if Dries was. Just to make it clear what the limitation is:

Solved:

Edits or deletes to items rendered as part of a view listing will invalidate the cache entry for the view. This is a cache tags version of views_content_cache

Not solved:

Newly listed items in the view (whether new entities, or entities edited where the edit would result in them appearing in the view).

There's two ways to fix this problem that I know of:

1. Add a TTL to the cache entry - this ensures no stale items/content are left in the view, then it's up to the person configuring the view to decide what the maximum acceptable time is for new items to appear in it.

2. Try to add extra cache tags to the view (and EFQ listings) that will be invalidated when items are created or updated with certain properties.

So for example a newly create article that's promoted to front page and published should invalidate the front page view, but a newly created article that's unpublished shouldn't. Adding a taxonomy term to an existing article could invalidate the taxonomy/term/n cache for that term, but not for all the other terms.

amateescu’s picture

Not solved:

Newly listed items in the view (whether new entities, or entities edited where the edit would result in them appearing in the view).

This is not really true. The patch introduced a "bundle" tag that is set by the view and cleared by the entity view builder, so any new or edited entity which is of a bundle that is already part of a view will clear the render cache for that view.

What's not covered is a new entity (not edit, since we assume we can't change bundles) of a bundle which *not* already part of the view.

Edit: Given the above, I've no idea why Daniel is so concerned over this change, the use case which is not covered is quite limited, as I don't think there are many views with no bundle restriction.

effulgentsia’s picture

Status: Needs work » Fixed
olli’s picture

Is the tag based plugin same as time based with expire times set to 0? If so, do we need this separate plugin? It is not obvious in the UI that time based uses tags too.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture