Problem/Motivation

  1. abstract class DrupalSqlBase is the base class for nearly every @MigrateSource plugin.
  2. \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.

CommentFileSizeAuthor
#12 3154156-12.patch1.63 KBwim leers
#12 interdiff.txt685 byteswim leers
#2 3154156-2.patch1.83 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3154156-2.patch1.83 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
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
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Improve migration system performance: » Improve migration system performance: statically cache DrupalSqlBase::$systemData

😅

quietone’s picture

quietone commented
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.

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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
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? 🤓

quietone’s picture

quietone commented
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?

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.

quietone’s picture

quietone commented

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

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented

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?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt685 bytes
new 3154156-12.patch1.63 KB

#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.

Status: Needs review » Needs work

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

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?

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.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
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.

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.

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.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.