Problem/Motivation

See #3497333: Add persistent cache to CachedStorage::findByPrefix

This method causes an uncached query on every page request on translatable sites

Steps to reproduce

Proposed resolution

Add some sort of persistent caching to that method.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3497341

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:

Comments

berdir created an issue. See original summary.

berdir’s picture

Status: Active » Needs review

Seeing two calls to that that aren't statically cached. Once with STATE_CONFIGURABLE, very early, in \Drupal\language\EventSubscriber\LanguageRequestSubscriber::setLanguageOverrides() after/as part of language negotation. And the second one is the EntityConverter param upcasting (through EntityRepository => ContentEntitBase::getLanguages(), which explicitly uses STATE_ALL.

The second cache creates the same information, but mostly hits static caches (CachedStorage and ConfigFactory).

Started with a basic implementation, lets see if this causes any test fails. Also didn't yet add static caching for $languages and no DI either.

berdir’s picture

StatusFileSize
new25.17 KB

To get the tests to pass, this required a single manual cache clear in a test that's explicitly messing with the default language service in the container on the fly. I think that's fair.

This saves an early bootstrap query on every single multilingual site requests that isn't an internal page cache hit. When using redis, that means the amount of queries go down from 6 to 5 on a dynamic page cache hit:

What's strange is that \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache isn't failing, because that's exactly the query that should go away. Ah, I think those tests don't run by default. I also wasn't able to run it manually, go this error:

This job is stuck because of one of the following problems. There are no active runners online, no runners for the protected branch , or no runners that match all of the job's tags: performance-test-runner 

Locally, it 's failing, but not only because that query is gone, also because I'm getting a race condition on automated_cron. Reliably got this fail 3 times:

1) Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     0 => 'SELECT "session" FROM "sessions" WHERE "sid" = "SESSION_ID" LIMIT 0, 1',
     1 => 'SELECT * FROM "users_field_data" "u" WHERE "u"."uid" = "10" AND "u"."default_langcode" = 1',
     2 => 'SELECT "roles_target_id" FROM "user__roles" WHERE "entity_id" = "10"',
-    3 => 'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "language.entity.%" ESCAPE '\\') ORDER BY "collection" ASC, "name" ASC',
+    3 => 'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("cron", "LOCK_ID", "EXPIRE")',
+    4 => 'SELECT "expire", "value" FROM "semaphore" WHERE "name" = "cron"',
+    5 => 'INSERT INTO "watchdog" ("uid", "type", "message", "variables", "severity", "link", "location", "referer", "hostname", "timestamp") VALUES (0, "cron", "Attempting to re-run cron while it is already running.", "WATCHDOG_DATA", 4, "", "LOCATION", "REFERER", "CLIENT_IP", "TIMESTAMP")',
 ]

I think the non-cron-on-installer change really did a number on this test. Others maybe too, didn't try to run them. The umami based test is special because it's first cron run is really slow. There's a lot to index there.

I tried a number of things such as the wait for terminate event (adds another query for the lock and wasn't reliable enough either), explicit lock wait(), sleep(). The one thing that seems to reliably help is explicitly running cron before the first web request. Maybe the performance tests should just run without that module?

catch’s picture

What's strange is that \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache isn't failing, because that's exactly the query that should go away. Ah, I think those tests don't run by default. I also wasn't able to run it manually, go this error:

That should run by default. The performance test job only runs to populate the grafana dashboard at https://gander.tag1.io/

The one thing that seems to reliably help is explicitly running cron before the first web request. Maybe the performance tests should just run without that module?

Yeah that's a good plan, we can always enable it if we want to test it, or run cron manually if we want to test after a cron run.

berdir’s picture

One question is whether we can avoid the cache tag. We have a variable amount of cache keys, but the amount of variations is limited, we might be able to do a bulk delete this. Not sure yet.

catch’s picture

Should we count cache gets that hit the database differently from fast chained? Would be useful to know how many additional db queries we actually have?

It would be possible to separate out chained fast vs. non-chained fast bin gets and aggregate them separately and that might be worth doing. I'm not sure we'd necessarily find more issues that way but it would be closer to reality which is always good. Can add separate getters for these so it's only an API addition.

berdir’s picture

Based on discussion in slack, I've simplified the caching to just cache the listAll(). Then it's a single cache, we can delete that in reset(), no more test changes necessary, no extra cache tags.

Also found #2686079: isMultilingual() loads all languages just to check if there is more than one again, very similar issue, proposed there a different fix, I think this is simpler and good enough.

smustgrave’s picture

Status: Needs review » Needs work

Left 1 small comment on the MR, will keep an eye for this to come back around

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

berdir’s picture

Status: Needs work » Needs review

Rebased, added a comment and updated the new OpenTelemetryNodePagePerformanceTest assertions. Note that it adds two cache gets because the language list gets initialized twice with different flags. I think that's fine for the fast chained bin and doesn't require static caching for this.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 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.

berdir’s picture

Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 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.

berdir’s picture

Status: Needs work » Needs review

Rebased and squashed the commits together to make future rebases easier.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Lets try and get to this before the bot attacks again :)

Actually all feedback appears to be addressed.

catch’s picture

Status: Reviewed & tested by the community » Needs work

One question on the MR.

berdir’s picture

Status: Needs work » Reviewed & tested by the community

Replied.

  • catch committed 7ed9c0c0 on 11.x
    Issue #3497341 by berdir: Add caching to ConfigurableLanguageManager::...
catch’s picture

Status: Reviewed & tested by the community » Fixed

That makes sense, just missed it, sorry for the false alarm.

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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