Improve cache dependencies in ContentTranslationDeleteAccess

Created on 4 January 2025, 3 months ago

Problem/Motivation

It loads all workflow only to add their cache tag, as any workflow might influence the access check.

That is slow (causes an extra config findByPrefix() query) and loads all those config entities, and it's also incorrect, as a new workflow might be added.

Steps to reproduce

Proposed resolution

Replace with a workflow_list cache tag. That said, this alone won't help, since the implementation then again loads all workflow entities.

However, we already have an API for this that doesn't require looping over them:

\Drupal\content_moderation\ModerationInformation::canModerateEntitiesOfEntityType and \Drupal\content_moderation\ModerationInformation::shouldModerateEntitiesOfBundle.

That relies on cached entity type and bundle information.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.1 πŸ”₯

Component

content_translation.module

Created by

πŸ‡¨πŸ‡­Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Unsure what to do about the cache tags. We can depend on entity entity_bundles cache tag, because they get invalidated on any workflow change. But that implies that we know about the implementation of the content moderation API that we're using.

  • Pipeline finished with Failed
    3 months ago
    Total: 90s
    #385772
  • Pipeline finished with Failed
    3 months ago
    Total: 1213s
    #385917
  • Pipeline finished with Failed
    3 months ago
    Total: 90s
    #385935
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Ok, this has a problem because it already needs this information in hook_entity_bundle_info_alter(). And we can't run after content_moderation, because the alter hook in content_moderation in turn depends on the translatable flag to be set.

    Added the direct check using workflow config entities for this case.

  • Pipeline finished with Failed
    3 months ago
    Total: 573s
    #386538
  • Pipeline finished with Failed
    3 months ago
    Total: 660s
    #386563
  • Pipeline finished with Failed
    3 months ago
    Total: 308s
    #387721
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    Replace with a workflow_list cache tag. That said, this alone won't help, since the implementation then again loads all workflow entities.

    Why would adding a list cache tag load all entities?

    In case you're worried there are too many workflows that are not related to content moderation, we could introduce a few custom list cache tags in workflows so you can have something like: workflow_list:type:content_moderation

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    It used to, but the new implementation no longer needs to load.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Is there any performance measurement that may help with the review?

  • Pipeline finished with Failed
    about 2 months ago
    Total: 433s
    #427906
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    This (authenticated user + content moderation + content translation) is currently not a scenario that has explicit enough test coverage to cover this. The test methods in \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest hit this scenario and there are issues to expand explicit assertions on them. I specifically see the query that this removes in πŸ“Œ Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active (search for workflows in the query list) and others might have hat it too of that meta, one that has a warm cache is likely more interesting as it has fewer queries in total.

    But those issues have been stuck for a while and aren't trivial to get in due to their risk of random fails.

    And it doesn't help that much to add explicit test coverage here, because those things are only really useful if they are already in HEAD and you can see the difference they make in a diff.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    I did rebase the linked issue (and reopened the closed MR), and then merged in this MR, the resulting diff is:

    $ git diff
    diff --git a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php
    index 903c4fba006..9aaef449198 100644
    --- a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php
    +++ b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php
    @@ -260,7 +260,6 @@ protected function testNodePageWarmCache(): void {
           'SELECT "t".* FROM "node_revision__field_summary" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
           'SELECT "t".* FROM "node_revision__field_tags" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
           'SELECT "t".* FROM "node_revision__layout_builder__layout" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
    -      'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "workflows.workflow.%" ESCAPE \'\\\\\') ORDER BY "collection" ASC, "name" ASC',
           'SELECT "revision"."vid" AS "vid", "revision"."langcode" AS "langcode", "revision"."revision_uid" AS "revision_uid", "revision"."revision_timestamp" AS "revision_timestamp", "revision"."revision_log" AS "revision_log", "revision"."revision_default" AS "revision_default", "base"."nid" AS "nid", "base"."type" AS "type", "base"."uuid" AS "uuid", CASE "base"."vid" WHEN "revision"."vid" THEN 1 ELSE 0 END AS "isDefaultRevision" FROM "node" "base" INNER JOIN "node_revision" "revision" ON "revision"."nid" = "base"."nid" AND "revision"."vid" IN (75)',
           'SELECT "revision".* FROM "node_field_revision" "revision" WHERE ("revision"."vid" IN (75)) AND ("revision"."vid" IN ("75")) ORDER BY "revision"."vid" ASC',
           'SELECT "t".* FROM "node_revision__field_cooking_time" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
    @@ -286,7 +285,7 @@ protected function testNodePageWarmCache(): void {
         $this->assertSame($expected_queries, $recorded_queries);
    
         $expected = [
    -      'QueryCount' => 172,
    +      'QueryCount' => 171,
           'CacheGetCount' => 247,
           'CacheGetCountByBin' => [
             'page' => 1,
    
  • Status changed to Needs review 21 days ago
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    That issue landed, so rebased, so now we can assert the improvement. It's not much, not compared with the amount of queries going on in that scenario, but every query saved helps.

  • Pipeline finished with Success
    21 days ago
    Total: 534s
    #448733
  • Pipeline finished with Success
    17 days ago
    Total: 364s
    #451674
  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Rebased.

  • Pipeline finished with Success
    8 days ago
    Total: 465s
    #459456
  • Pipeline finished with Success
    4 days ago
    Total: 553s
    #461901
  • Pipeline finished with Success
    4 days ago
    Total: 574s
    #462648
Production build 0.71.5 2024