Problem/Motivation

This is a follow-up from #2952210: Improve list usage page

The logic for deciding what results should appear on the UI when visiting the usage page is quite expensive. On a site with lots of relationships, this could have a significant impact if we need to re-calculate that every time we show the page.

Proposed resolution

We should be able to cache the visible rows for a given page (target type/id) and just re-use that. Every time the API records a modification in usage for that same target type/id we should invalidate this cache.

Remaining tasks

  • Figuring out to invalidate cache by target's cache tags

User interface changes

API changes

Data model changes

Issue fork entity_usage-2985265

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

marcoscano created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +D8 cacheability

Sounds sensible!

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

For visibility, we might end up marking this as won't-fix in the 2.x branch, since in a theoretical 3.x branch being cooked in #3060802: Refactor module architecture in a simpler, opinionated and more performant approach this wouldn't be an issue anymore.

rp7’s picture

rp7 commented

Could you elaborate a little more on how #3060802: Refactor module architecture in a simpler, opinionated and more performant approach makes this no longer necessary? Is it because rendering the page itself will be less expensive?

If that's the case - IMO this page could still benefit from caching, but 3.x makes this less of a priority?

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

>Is it because rendering the page itself will be less expensive?

Yes! The idea on that branch is that the page is just a single DB query away, since we would be displaying all results, without the need of post-processing them (which is what makes it slow in some cases). I agree it won't hurt to cache that if we want, but IMHO it might not be worth the effort since the benefit would be minimal as compared to querying the DB again.

el7cosmos’s picture

el7cosmos
Primary language Indonesian
Location 🇮🇩 GMT+7
commented

On our site, the list got cached in the dynamic page cache, and we implemented custom invalidation. But we need a proper cache tag in the first place and I will submit MR for that.

el7cosmos opened merge request !26

el7cosmos’s picture

el7cosmos
Primary language Indonesian
Location 🇮🇩 GMT+7
commented
Issue summary: View changes
Status: Active » Needs work

MR still needs invalidation

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

Thank you for contributing!
I'm a bit confused about the scope of your MR though. In #6 you seem to mention that you just wanted to add the tag, but in #8 I got the idea that you'd expect this MR to implement invalidation too, so now I'm not sure. If it's just the tag, maybe it would be better in a separate issue, since this one is to actually implement a full caching solution? (Although, if you think the custom invalidation you have in your project could be shareable, we would certainly welcome it being discussed here!)

Also, secondarily, I'm seeing we are improving a few unrelated things in the same PR, in a slightly non-BC way. I agree the changes are an improvement, but that would indeed break custom code that extended our controller class, for example.

Thanks!

acbramley’s picture

acbramley commented

@marcoscano adding cacheable metadata is good practice for any rendering layer in Drupal. There is no need for custom invalidation as entity tags get automatically invalidated when the entity is updated or deleted.

In this case, we are viewing the usage page for an entity, so it makes perfect sense to add the entity as a cacheable dependency.

We can use drupal's gitlab instance to search for contrib modules extending this controller

We can see that it is just the delivery module which does not have a stable release so this is a good time to update and notify the maintainers there of the changes required.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

Thank you @acbramley for your reply!

I agree that adding cacheability to render arrays is usually good, and I'm onboard to adding it here.
However, I might be missing how just adding the tags for the node we are checking usages for is enough...

For example, if NodeA references NodeB in an entity_reference field, when we look at the usage list page for NodeB, we'll be adding NodeB cache tags, which will automatically get invalidated when NodeB gets changed/deleted.

However, if now NodeC references NodeB in an entity_reference field, we need to invalidate the cached list we built above, and NodeB's cache tag won't help with that. That's why I believe this issue is tricky to solve, because potentially any piece of content could create a new reference to the entity we are interested in, at any point. Maybe creating a custom tag and invalidating it on every (content) entity save could be a way, but I'm not 100% convinced it's worth the effort... I might be missing something here too, so apologies if that's the case.

Regarding BC on the controller, there could be custom code relying on this as well, and it would be suboptimal to silently break those implementations on a minor release bump. I agree this probability is low, and I'm actually OK leaving a big warning in the release notes if we absolutely have to do it, but wanted to make sure it's a justified change. Unless I'm missing something here, the goal on this PR could be achieved equally without the BC-breaking changes? If so, how about we open a separate ticket to optimize/improve existing code, that would only get merged into a new major, so that we can list all BC-breaking changes?

Thanks!

acbramley’s picture

acbramley commented
Status: Needs review » Needs work

@marcoscano yeah that's a fair point, I suppose a further improvement would be to invalidate a target's entity tags when usage is registered, that way an entity's usage page would always be up to date.

I like the idea of keeping this issue simple without BC changes, and moving them to a separate issue although I imagine that might just be postponed on the 3.x branch (which is fine).

NW based on that. Let's just load the entity in the controller callback with
$entity = $this->entityTypeManager->getStorage($entity_type)->load($entity_id);

This shouldn't add much overhead as it'll be statically cached.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

I suppose a further improvement would be to invalidate a target's entity tags when usage is registered, that way an entity's usage page would always be up to date.

Yep, that would work too. I think I would have a slight preference for a custom tag in this case, since we don't really need to invalidate rendered versions of node:12 every time a new piece of content references it... On the other hand, I do see the value of :

    $metadata->addCacheableDependency($entity);
    $metadata->applyTo($build);

so I'm not really sure here...