Updated: Comment #N

Problem/Motivation

Some cache tags are currently deleted over and over again, especially during test runs.

Additionally, we seem to delete cache tags for each backend, even if they only use the same. There's an issue to address this with a separate service.

See the attached screenshot, 300 calls to Cache::deleteTags() lead to 3000 DatabaseBackend::deleteTags(), each of those executing a merge query.

The biggest offenders are menu name cache tags, cached config storage, entity field definitions.

Proposed resolution

Added a static (drupal_static() just like the existing tags cache for now, real static might mess with tests). Can become a normal property of the cache tag service once we have that.

This cuts down the number of executed merge queries to 25, instead of 3000. Should be about the same for every single web test. Saves almost 2.5 seconds for me, on a fairly well optimized database.

See also the second screenshot how this changes the time per call, you can see that drupal_get_schema() moves to the top, probably because it also sets the cache again and therefore has to delete again. We might be able to optimize that by not adding the tag to the main schema cache item? But that's only the biggest part of the much much smaller total time now.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#11 cache-tag-deletion-2194273-10.patch2.21 KBBerdir
#8 cache-tag-deletion-2194273-8.patch2.9 KBBerdir
#3 great_patch.jpg128.66 KBamateescu
cache_tag_delete_patch.png135.88 KBBerdir
cache_tag_delete_now.png148.02 KBBerdir
cache-tags-deletion.patch1.39 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Parent issue: » #2006434: [meta] Speed up web tests
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Related issues: +#918538: Decouple cache tags from cache bins

Yay, tested locally and it rally speeds-up!

d8t --class '\Drupal\field_ui\Tests\FieldUIRouteTest' changed from 32-31 sec to 25!

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -164,6 +164,13 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
+    // Re

Needs to finish

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
great_patch.jpg128.66 KB

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Avoid repeated cache deletions » Avoid repeated cache tag deletions

Fixing title.

Status: Needs review » Needs work

The last submitted patch, cache-tags-deletion.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Need to look into the fails, but this took 1h30 on testbot #813, other recent core tests there were around 2h20. So the effect there seems to be considerably bigger, I guess that's related to concurrency and that mysql has to handle 8 parallel installations/cache rebuilds.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Priority: Normal » Major

I think this is (at least) major. — Also, I'm super-happy that we're not implanting a test-specific workaround again, but actually attacking the root cause! :)

Thanks @Berdir!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
cache-tag-deletion-2194273-8.patch2.9 KB

Aw, stupid web tests, were messing with the logic again :)

Fixed this exact problem already once, needs a reset in resetVariables(), just like the cache tags cache too.

Also includes the fix #2152825: rename FieldItemBase::getFieldSetting[s]() to getSetting[s]() as HEAD is currently broken, hoping that it will start to run before HEAD comes back :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Related, while debugging this I noticed that configFindByPrefix is deleted *a lot*. I just check with an actual installation that we have that has been running for a while, and the counter of that is now at ~45k. So, it might be very worth it to replace it with a CacheCollector, less cache tag deletion and cache writes, not sure how complicated it would be to implement it.

Status: Needs review » Needs work

The last submitted patch, 8: cache-tag-deletion-2194273-8.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
cache-tag-deletion-2194273-10.patch2.21 KB

Ok, HEAD was fixed before the test was started as it was already postponed, here's an updated one without the fix and also added the comment that I wanted to write above.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

Do we already have an issue to replace those weird preexisting drupal_static()s with class properties or something?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yes, I guess that will happen in #918538: Decouple cache tags from cache bins, which makes cache tags a separate service.

alexpott’s picture

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

Committed af7652d and pushed to 8.x. Thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +D8 cacheability

HOLY SHIT this is an awesome patch!

Berdir++++++++++++++++++++++++++++++++++++++++

Status: Fixed » Closed (fixed)

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