Problem/Motivation

On cold caches, ContentLanguageManager::loadContentLanguageSettings() gets called multiple times for individual entities/bundles, this is mostly from ContentTranslationHooks::entityExtraFieldInfo() - which calls it 46 times on an umami node page.

If you dump the database queries in the umami front page performance test, it looks like this:

+    23 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "taxonomy.vocabulary.recipe_category", "taxonomy.vocabulary.tags" )',
+    24 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.block_content.banner_block" )',
+    25 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.block_content.basic" )',
+    26 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.block_content.disclaimer_block" )',
+    27 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.block_content.footer_promo_block" )',
+    28 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.media.audio" )',
+    29 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.media.document" )',
+    30 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.media.image" )',
+    31 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.media.remote_video" )',
+    32 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.media.video" )',
+    33 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.menu_link_content.menu_link_content" )',
+    34 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.node.article" )',
+    35 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.node.page" )',
+    36 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.node.recipe" )',
+    37 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.shortcut.default" )',
+    38 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.taxonomy_term.recipe_category" )',
+    39 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.taxonomy_term.tags" )',
+    40 => 'SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "language.content_settings.user.user" )',

We can instead multiple load the config entities, either always in the ContentTranslationManager or just in the entity info hook, to reduce those queries down to one.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3560633-nr-bot_6u051uje.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3560633

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

catch created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

We enabled static caching for language_content_settings, but that only helps with repeatedly loading the same in a single request.

Because there's no guarantee that the configs exist, a pure loadMultiple to preload might not catch them all. I'd probably inline the whole thing into entity field info, then we can do a loadMultiple() and only loop over those that actually exist.

There's also the weird fallback to create on the fly, but that seems pointless to just check something that definitely won't be there then \Drupal\content_translation\ContentTranslationManager::loadContentLanguageSettings. I think we should add a third argument to that method to not create on the fly for the two getters.

catch’s picture

catch
he/him
Primary language English
commented

I'd probably inline the whole thing into entity field info, then we can do a loadMultiple() and only loop over those that actually exist.

Yeah was leaning towards that direction, worth giving a go to see if that fixes the numbers,and then go from there.

catch opened merge request !14011

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work

Needs tidying up but the basics should be there - at least if tests don't blow up.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

OK that works, cache get/set savings on top of the database queries so a fair bit shaved off with this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Posted a review.

catch’s picture

catch
he/him
Primary language English
commented

I also wasn't sure what to do about dependency injection, but went with 'least change' on the basis another issue might do it - either adding dependency injection or moving things around or a combination of the two. There are some hook re-organisation issues landing, but also we have a lot of core modules.

Can try to improve the variable names a bit.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Actual test fails, so something is off I think.

I wasn't sure about renaming $bundles as that then differs from the hook documentation but I think it's the cleanest option for this hook.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

I wasn't sure about renaming $bundles as that then differs from the hook documentation but I think it's the cleanest option for this hook.

Yeah me neither, but same - I think it ends up clearer.

Test failure was not enough variable renaming, should be good now.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Sorry to be that guy, can this be rebased :)

rpayanm made their first commit to this issue’s fork.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

This looks good now I think. My feedback has been addressed, agree with #11 on the variable name. We could open a follow-up to rename the hook docs and other implementations accordingly.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Optimize ContentLanguageManager::loadContentLanguageSettings() » Load content language settings in bulk when rebuilding bundle and extra fields info

Updating title, we aren't actually doing anything to ContentLanguageManager::loadContentLanguageSettings

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3560633-nr-bot_6u051uje.txt91 bytes

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll... conflicting with HEAD

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 658e2bf8816 to main and a38db813bdb to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed a38db813 on 11.x 
    perf: #3560633 Load content language settings in bulk when rebuilding...

  • alexpott committed 658e2bf8 on main 
    perf: #3560633 Load content language settings in bulk when rebuilding...

Status: Fixed » Closed (fixed)

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