Improve migration system performance: statically cache DrupalSqlBase::$systemData [#3154156]

Problem/Motivation

abstract class DrupalSqlBase is the base class for nearly every @MigrateSource plugin.
\Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::checkRequirements() calls \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::moduleExists(), which calls \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::getSystemData(), which does a DB query to collect all data in the Drupal 6/7 system DB table and statically caches the result

The consequence is that the system table is usually queried over a hundred times. Even though the system table of the source site of course always stays the same, regardless of the source plugin used.

Proposed resolution

Reduce this to a single query without breaking BC, to improve performance.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Comment	File	Size	Author
#12	3154156-12.patch	1.63 KB	Wim Leers
#12	9.3.x: PHP 8.0 & MySQL 5.7 29,229 pass, 2 fail 9.4.x: PHP 8.0 & MySQL 5.7 29,345 pass, 2 fail 10.0.x: PHP 8.0 & MySQL 5.7 29,300 pass, 2 fail
#12	interdiff.txt	685 bytes	Wim Leers
#2	3154156-2.patch	1.83 KB	Wim Leers
#2	9.0.x: PHP 7.3 & MySQL 5.7 27,335 pass 8.9.x: PHP 7.3 & MySQL 5.7 28,533 pass 9.1.x: PHP 7.3 & MySQL 5.7 27,374 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

23 June 2020 at 12:01

Wim Leers created an issue. See original summary.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 23 June 2020 at 12:02

Status:

Active

» Needs review

File	Size
3154156-2.patch	1.83 KB
9.0.x: PHP 7.3 & MySQL 5.7 27,335 pass 8.9.x: PHP 7.3 & MySQL 5.7 28,533 pass 9.1.x: PHP 7.3 & MySQL 5.7 27,374 pass

File

Size

3154156-2.patch

1.83 KB

9.0.x:
PHP 7.3 & MySQL 5.7 27,335 pass

8.9.x:
PHP 7.3 & MySQL 5.7 28,533 pass

9.1.x:
PHP 7.3 & MySQL 5.7 27,374 pass

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 23 June 2020 at 12:08

Issue tags:

+Performance

Calling checkRequirements() on every (derived) migration plugin instances with \Drupal\migrate_drupal\MigrationPluginManager's cache primed (meaning they've already been discovered), on the concrete project I'm working on:

HEAD: 564–701 milliseconds
With #2 applied: 345–357 milliseconds

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 23 June 2020 at 12:10

Title:

Improve migration system performance:

» Improve migration system performance: statically cache DrupalSqlBase::$systemData

😅

Comment #5

quietone CreditAttribution: quietone as a volunteer commented 28 June 2020 at 01:45

Status:

Needs review

» Needs work

Nice improvement that will help most migrations.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -35,9 +35,21 @@ abstract class DrupalSqlBase extends SqlBase implements ContainerFactoryPluginIn
+   * This is safe to share across all source plugin instances because the source
+   * database must be the same for all of them.

Ah, but source plugins that extend SqlBase can set a database connection. There is no guarantee that all source plugins will use the same db.

Comment #6

28 June 2020 at 01:45

Version:

9.0.x-dev

» 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 28 September 2021 at 15:06

Status:

Needs work

» Needs review

#5: But … this is specifically DrupalSqlBase, not SqlBase. So only Drupal-DB-targeting source plugins will be affected by this. Which also means only they would get this performance improvement, and hence your concern in #5 would not apply?

Perhaps you want to change the wording of the comment from "all source plugins" to something more specific? 🤓

Comment #8

quietone CreditAttribution: quietone as a volunteer commented 26 October 2021 at 08:45

Status:

Needs review

» Needs work

Yes, a change to the comment would be good. Unfortunately, I am not coming up with anything (a bit late here). Also, the use of 'cross-instance' muddles things. I keep wondering if what I am thinking as 'cross-instance' is the same as yours. Perhaps the comments would be clearer without that.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -35,9 +35,21 @@ abstract class DrupalSqlBase extends SqlBase implements ContainerFactoryPluginIn
+   * @deprecated

Why deprecate this?

Comment #9

26 October 2021 at 08:45

Version:

9.1.x-dev

» 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #10

quietone CreditAttribution: quietone as a volunteer commented 19 January 2022 at 09:15

Took another look and this will need proper deprecation of the property, https://www.drupal.org/node/2856615#how-param.

Comment #11

RoSk0

he/him

Russian

Wellington

CreditAttribution: RoSk0 at Catalyst IT commented 20 January 2022 at 06:29

Tested patch from #2 on 9.2.9. Call to drush migrate:status --tag=tag -vvv

Before: [1.31 sec, 350.57 MB]
After: [0.62 sec, 41.01 MB]

Speaks for itself.

With regards to $systemData property: maybe we should mark it internal instead?

Comment #12

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 24 January 2022 at 14:14

Status:

Needs work

» Needs review

File	Size
interdiff.txt	685 bytes
3154156-12.patch	1.63 KB
9.3.x: PHP 8.0 & MySQL 5.7 29,229 pass, 2 fail 9.4.x: PHP 8.0 & MySQL 5.7 29,345 pass, 2 fail 10.0.x: PHP 8.0 & MySQL 5.7 29,300 pass, 2 fail

File

Size

interdiff.txt

685 bytes

3154156-12.patch

1.63 KB

9.3.x:
PHP 8.0 & MySQL 5.7 29,229 pass, 2 fail

9.4.x:
PHP 8.0 & MySQL 5.7 29,345 pass, 2 fail

10.0.x:
PHP 8.0 & MySQL 5.7 29,300 pass, 2 fail

#11: Thanks for the test! 🥳

#10: The docs you linked to only work for method parameters, not for class properties. I don't think we can trigger a deprecation error for properties… I'm also fine with not deprecating this; there's no real cost in maintaining it anyway. Did that in this reroll.

Comment #13

24 January 2022 at 15:13

Status:

Needs review

» Needs work

The last submitted patch, 12: 3154156-12.patch, failed testing. View results

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 24 January 2022 at 15:46

This failed because \Drupal\Tests\migrate_drupal\Unit\source\DrupalSqlBaseTest::testMinimumVersion() was newly introduced in #3151732: DrupalSqlBase::checkRequirements should test version with $minimum_version and is the only code violating the assumptions this patch introduces: it pretends there are different databases for each migration plugin definition.

This is not reproducible in real-world scenarios. Therefore we should change that test.

Do you agree, @quietone?

Comment #15

24 January 2022 at 15:46

Version:

9.3.x-dev

» 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #16

Anybody

German

Porta Westfalica

CreditAttribution: Anybody at DROWL.de commented 4 October 2022 at 08:51

Version:

9.4.x-dev

» 9.5.x-dev

@quietone here's an open question from @Wim Leers in #14, which might be relevant for the next steps here.

Still relevant for migration performance, updating version.

Comment #17

4 October 2022 at 08:51

Version:

9.5.x-dev

» 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #18

4 October 2022 at 08:51

Version:

10.1.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.