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
Comment | File | Size | Author |
---|---|---|---|
#14 | 3037823-14.patch | 3.21 KB | alexpott |
#14 | 12-14-interdiff.txt | 1.73 KB | alexpott |
#12 | 3037823-12.patch | 2.79 KB | alexpott |
#12 | 8-12-interdiff.txt | 1.74 KB | alexpott |
#8 | 3037823-8.patch | 2.65 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottI'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.
Comment #4
alexpottComment #5
alexpottComment #6
alexpottSo 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.
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe
$connection
variable doesn't seem to be used more than once, so perhaps we could inline the call?Comment #8
alexpottThanks @amateescu
Comment #9
Berdircan 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).
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks great now :)
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOops, 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.
Comment #12
alexpottSlightly new approach in the test. Not using the quest twice anymore because after updates we can use the API.
Comment #13
BerdirYes, now it looks great.
Comment #14
alexpottJust 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.
Comment #15
BerdirUpdates look fine to, assuming it passes, which I guess you tried locally.
Comment #16
Wim Leers🤯
Comment #17
LendudeIn order to make regressions like this easier to catch #3033037: DrupalCI should track and display job run times
Comment #18
tacituseu CreditAttribution: tacituseu commentedYup, this completely restores pre Feb 7 time baseline, and explains why only
8.7,x
was affected :).Comment #19
idebr CreditAttribution: idebr at ezCompany commentedThis 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.
Comment #20
alexpottLol 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...
Comment #21
catchCommitted 4f15f2d and pushed to 8.7.x. Thanks!