Problem/Motivation

#3032869: \Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects() caused test runs to double in duration is tracking increased 8.7.x test run times after adding \Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects() - we thought we had fixed it but we only improved it.

That method relies on system.theme.data being broken and then fixing it by doing $container->get('theme_handler')->refreshInfo();. In 8.7.x this call doesn't fix $container->get('state')->get('system.theme.data', []); because that is not used anymore and not kept up to date. Therefore the caches are being cleared on every update request causing performance issues.

Proposed resolution

In 8.7.x only remove system.theme.data in \Drupal\Core\Update\UpdateKernel::fixSerializedExtensionObjects().

Also an update function to remove it because it is possible that the key was fixed in 8.6.x but it still needs removing as it will get stale because it is no longer maintained.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#14 3037823-14.patch3.21 KBalexpott
#14 12-14-interdiff.txt1.73 KBalexpott
#12 3037823-12.patch2.79 KBalexpott
#12 8-12-interdiff.txt1.74 KBalexpott
#8 3037823-8.patch2.65 KBalexpott
#8 2-8-interdiff.txt907 bytesalexpott
#2 3037823-2.patch2.68 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Active » Needs review
FileSize
3037823-2.patch2.68 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented 
+++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
@@ -236,6 +236,8 @@ public static function fixSerializedExtensionObjects(ContainerInterface $contain
+      // The system.theme.data key is no longer used in Drupal 8.7.x.
+      $container->get('state')->delete('system.theme.data');

+++ b/core/modules/system/system.install
@@ -2239,3 +2239,11 @@ function system_update_8601() {
+/**
+ * Remove the unused 'system.theme.data' from state.
+ */
+function system_update_8701() {
+  // The system.theme.data key is no longer used in Drupal 8.7.x.
+  \Drupal::state()->delete('system.theme.data');
+}

I've added this in both places because it is possible that the key was fixed on 8.6 but it still needs removing on 8.7.x because it is no longer used and will not be kept up-to-date.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Title: system.theme.data is not removed even though it is no longer used » The system.theme.data key remains corrupted in state causing performance issues and is not used in >= 8.7
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Priority: Major » Critical

So the Functional tests in #2:
PHP 7.2 - Test run duration: 33 min 26 sec
PHP 7.2-dev - Test run duration: 32 min 15 sec

And HEAD
PHP 7.2 - Test run duration: 46 min 26 sec
PHP 7.2-dev - Test run duration: 44 min 33 sec

I think a 10+ minute performance increase in tests probably makes this a critical.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 
+++ b/core/modules/system/tests/src/Functional/Update/WarmCacheUpdateFrom8dot6Test.php
@@ -28,6 +28,13 @@ protected function setDatabaseDumpFiles() {
+    $connection = Database::getConnection();

The $connection variable doesn't seem to be used more than once, so perhaps we could inline the call?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
2-8-interdiff.txt907 bytes
3037823-8.patch2.65 KB

Thanks @amateescu

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/core/modules/system/tests/src/Functional/Update/WarmCacheUpdateFrom8dot6Test.php
@@ -38,6 +44,7 @@ public function testUpdatedSite() {
     $this->assertSame('Australia/Sydney', $this->config('system.date')->get('timezone.default'));
+    $this->assertSame('0', $count_query->execute()->fetchField());
   }

can we either give the query a better variable name, add a comment, or re-create it (I'm not if we actually support executing the same query object more than once. We certainly don't with entity queries).

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks great now :)

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Reviewed & tested by the community » Needs review

Oops, cross-posted with #9.

I remember that we do support executing the same db query more than once, but maybe it's safer to clone it for the second call.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
8-12-interdiff.txt1.74 KB
3037823-12.patch2.79 KB

Slightly new approach in the test. Not using the quest twice anymore because after updates we can use the API.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Yes, now it looks great.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
12-14-interdiff.txt1.73 KB
3037823-14.patch3.21 KB

Just a few more updates to the test. Improving the comments and assertNull without a message is not that helpful. Also the resetAll() is totally superfluous now #2975081: UpdatePathTestBase fails to re-initialize the test site (rebuild container, clear caches) after running the database updates has landed.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Updates look fine to, assuming it passes, which I guess you tried locally.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I think a 10+ minute performance increase in tests probably makes this a critical.

🤯

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented
Related issues: +#3033037: DrupalCI should track and display job run times

In order to make regressions like this easier to catch #3033037: DrupalCI should track and display job run times

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented

Yup, this completely restores pre Feb 7 time baseline, and explains why only 8.7,x was affected :).

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Related issues: +#3031626: The state key system.theme.data is no longer used

This issue contains the change from #3031626: The state key system.theme.data is no longer used. This related issue can be closed as a duplicate on commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Lol yeah I thought I had opened this issue already :)

Also drupal search was not my friend - https://www.drupal.org/project/issues/drupal?text=system.theme.data&stat...

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 4f15f2d and pushed to 8.7.x. Thanks!

  • catch committed 4f15f2d on 8.7.x 
    Issue #3037823 by alexpott, Berdir, amateescu: The system.theme.data key...

Status: Fixed » Closed (fixed)

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