[D7] Improve WebTestCase performance by 50%

+++ b/includes/module.inc
@@ -460,6 +460,7 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+      drupal_static_reset('drupal_get_schema_versions');

This should be fixed separately in a child issue.

Here are the benchmarks for one setUp() call with enabling common_test module (if not --cache-modules), but drupal_render() calls it 6 times (as there are 6 test* methods).

drupal_render() 62 passes, 0 fails, 0 exceptions, and 13 debug messages

Without cache:

Test run duration: 1 min 4 sec

With --cache:

Test run duration: 34 sec

And the diff to the version without cache (just setUp):

With --cache-modules:

Test run duration: 15 sec

And the diff to the version without cache (just setUp):

Need to work with Mixologic that we get the test runner --directory patch applied, so this can be tested.

The --cache part did resetAll() twice. So the benchmark was off. Fixed in above comment.

And now we got the 50%.

Fixed the bug, showed that the critical path has not changed (critical-path-unchanged.txt; created with diff -w) and added interdiff.

Very good point!

Removed that and copy speed is thanks to the turn on indexes the same.

#10:

Seems we don't have tests for that except for the tables that had been excluded, but I don't think it was that important.

#15: It was not you, it was DrupalCI having a conflict due to #2551981: Add --directory option to run-tests.sh test discovery. However as DrupalCI does not have --cache for obvious reasons to test the speed-up on the CI, we need to hardcode $test->cache = TRUE inside the patch for now.

Thanks for your contribution!

#18: I do agree and the boost is huge. However as this code will also run for contrib project tests a fixed whitelist is unfortunately not possible.

#19: I resolved the problem of concurrency by using a transaction for the DB copying plus try {} catch. For parallel runs this gives a minimal performance loss of 2s (until the caches are primed), but ensures I don't need any complicated locking. The transaction also ensures that the cache is always consistent.

As for different caches per profile, that is why I use $this->profile as normal cache key and a normalized version of func_get_args() for the module cache key.

I am not sure why your implementation did not work, but as $this->profile is setup before installation, I cannot see where this would fail.

Unfortunately turning the checks off does not make the copy faster for me and the statements itself take 30 ms instead.

I found two flaws however and fixed them:

- Using a cache key suffix was not a good idea as it tried to copy the already cached tables as well. With a cache_key prefix, it works way better.
- Using a transaction won't work as DDL is non transactional in MySQL, so I removed it.

Uploading a new patch, which should pass as there should be no changes.

- Fixed concurrency problem using a different method (re-using semaphore table, but creating a new simpletest_semaphore table, which _only_ exists in the cache copy and is not copied over)
- Allowed cache to be re-created if e.g. the user pressed ctrl-c during storeCache()
- Warning the user if cache copy failed via a debug message, but not fataling

This works like a charm, now testing on d.org again.

Postponed for the bug fix for:

drupal_static_reset('drupal_get_schema_versions');

Turns out the drupal_static_reset() is no longer needed as I cold not reproduce the error anymore and was just a problem due to me forgetting to module_load_all() after loading the cache.

Fixed in interdiff, therefore no longer blocked.

Testing test suite with --cache enabled on d.org again.

Oh, it seems it _is_ a problem after all.

I now could also reproduce it. The problem is that some kind of state is held between test runs even with vanilla Drupal 7:

e.g. if I change:

testUpdateAccess => ttestUpdateAccess, _then_ the test fails, too.

Also the wrong state is stored in cache.

E.g. I apply the fix, but have corrupted state in cache, then it fails, too.

Will open a child issue now.

OMG I know why:

- After the first test has run all the modules install files are already loaded in memory, so function_exists() works obviously.

This means that drupal_get_schema_versions() is called for the module before its .install file has been loaded!

Because we never install some modules in the cached version, the schema is wrong then.

And the reason is that more modules could be loaded already than had install files loaded, which mostly is for core modules.

The reason is purely test based however:

module_list() uses a static, therefore it has the module_list of the host installation.

Therefore the first call to drupal_get_schema_versions() sets all modules that the host has to array(), e.g. block, node, ...

But the module list cannot be refreshed before the system table exists, therefore the test runner cannot refresh it prior to installation, but the installation of system module already leads to the problem then.

The static reset fixes it obviously, but could lead to performance penalty.

Another possibly is to module_load_install() before checking the schema version, but that also runs always.

The best way to fix this is to reset the schema versions when the module list is reset in system_install():

  // Clear out module list and hook implementation statics before calling
  // system_rebuild_theme_data().
  module_list(TRUE);
  module_implements('', FALSE, TRUE);

+  // Ensure the schema versions are not based on the old module list.
+  drupal_static_reset('drupal_get_schema_versions');

And that fixes the problem as well.

Uploading a patch to see if its fixed.

The test fail looks unrelated.

Self-reviewed as if I was reviewing this for commit:

1. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -45,6 +45,20 @@ abstract class DrupalTestCase {
   +   * Whether to cache the setUp.
   ...
   +   * Whether to cache the setUp with modules installed.
   

   nit - These comments could be clearer. Ideas?

2. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +   * @return
   +   *   TRUE when the cache was used, FALSE when cache was not available.
   

   Missing return type.

3. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +    $cache_key = $this->getCacheKey($cache_key_prefix );
   

   nit - trailing "whitespace"

4. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +      $this->copyCache($cache_key, substr($this->databasePrefix, 10));
   

   This needs a try { } catch as well as copyCache() can throw an Exception().

   It might be even better to now put the try { catch } directly into copyCache().

5. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +   *   The cache key to use, by default only based on profile.
   

   nit - $this->profile or on the profile used by the test.

6. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +    return '1cache_' . $cache_key_prefix . $this->profile;
   

   The prefix is already quite long, while 1cache is pretty explicit, it might be better to use:

   1c_

   to allow for longer table names as the 'simpletest' prefix and profile name and modules hash already have quite some length.

   I do agree though that it is good to use the profile to also show which cache this is as this improves debuggability - as if it was just a hash.

   --

   Should also add a comment that 1 is needed as first character to ensure proper cleanup.

7. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +    // Everyone is allowed to pass this step.
   

   This should explain a little more why it is important that everyone can pass this stage.

8. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +   * @param $from
   ...
   +   * @param $to
   

   Missing types.

9. +++ b/modules/simpletest/drupal_web_test_case.php
   @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
   +   *   The prefixId / cacheKey from where to copy.
   ...
   +   *   The prefixId / cacheKey to where to copy.
   

   prefixId / cacheKey => prefix_id / cache_key

10. +++ b/modules/simpletest/drupal_web_test_case.php
    @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
    +   * @param $src
    ...
    +   * @param $dest
    

    Missing types

11. +++ b/modules/simpletest/drupal_web_test_case.php
    @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
    +  protected function recursiveCopy($src, $dst) {
    +    $dir = opendir($src);
    

    We use full descriptive parameter names in Drupal.

12. +++ b/modules/simpletest/drupal_web_test_case.php
    @@ -1424,6 +1438,160 @@ class DrupalWebTestCase extends DrupalTestCase {
    +        if (is_dir($src . '/' . $file)) {
    +          $this->recursiveCopy($src . '/' . $file, $dst . '/' . $file);
    ...
    +          copy($src . '/' . $file, $dst . '/' . $file);
    

    Some helper variables would be nice.

13. +++ b/modules/simpletest/drupal_web_test_case.php
    @@ -1476,57 +1644,108 @@ class DrupalWebTestCase extends DrupalTestCase {
    +      $this->assertTrue(TRUE, t('Using cache: @cache (@key)', array(
    

    nit - can use $this->pass() instead.

14. +++ b/modules/simpletest/drupal_web_test_case.php
    @@ -1476,57 +1644,108 @@ class DrupalWebTestCase extends DrupalTestCase {
    +        '@cache' => ($use_modules_cache ? 'Modules Cache' : 'Normal Cache'),
    

    Normal Cache should be "Installation Cache" as that is what we cache.

15. +++ b/modules/simpletest/drupal_web_test_case.php
    @@ -1476,57 +1644,108 @@ class DrupalWebTestCase extends DrupalTestCase {
    +      $this->resetAll();
    

    In theory this would not be needed, but there is a comment over there that cache tables need this change to properly register the new DB prefix, so we are running this when using --cache-modules.

    Removing this woulds save ~ 1 sec, too.

16. +++ b/modules/simpletest/simpletest.module
    @@ -589,7 +589,7 @@ function simpletest_clean_temporary_directories() {
    +      if (is_dir($path) && (is_numeric($file) || strpos($file, '1cache_') === 0)) {
    

    If we do the 1cache_ change, this should be 1c_ as well.

    Also needs a comment.

17. +++ b/scripts/run-tests.sh
    @@ -152,6 +152,19 @@ All arguments are long options.
    +  --cache     Cache result of setUp per installation profile. This will create
    ...
    +              Cache result of setUp per installation profile and installed
    

    nit - setUp => the test setup

18. +++ b/scripts/run-tests.sh
    @@ -368,6 +385,8 @@ function simpletest_script_run_one_test($test_id, $test_class) {
    +    $test->cache = !empty($args['cache']);
    +    $test->cacheModules = !empty($args['cache-modules']);
    

    We already use a public property for the test results.

    This avoids having to add a setter / getter and also allows tests to opt-out of caching by setting $this->cache = FALSE; in their setUp() method.

As this only affects tests, this does not need to make 7.50 to go in, but can go in afterwards as experimental option.

Here is the cleanup with interdiff, etc.

Can still be reviewed if someone wants to.

No longer blocked on the other issue, but needs to be rerolled now.

Just a re-roll.

This needs some good reviews now.

This is blocked on reviews now.

#42: The thing is opt-in. You will need to put --cache for the test runner for that to work.

I hardcoded $this->cache = TRUE for the test run in #29. I can easily hard-code it again - but I think we tested performance on test bot enough now.

#45: I have tried those, but as we disable / enable the keys already, this did not bring any further speedup for me.

Nice, that change works for me :).

3 minute test takes 21 seconds on subsequent run.

With --cache or with --cache-modules? (just curious)

Testing speed increase on test runners again (hardcoded --cache to TRUE and --cache-modules to true in other patch), DO NOT USE.

• Without: 12 min
• --cache: 5 min 9 sec
• --cache --cache-modules: 4 min 36 sec

The problem is that we need to get the Drupal 8 patch in first, before we can profit from it.

The other problem is that the Drupal 8 patch does not profit from raw installation caching as much as the Drupal 7 patch, so it is not secure to get in ...

#59: Good catch!

- #51 broke it, by removing the underscore.

- As we call it after an installation there should be tables to copy ...

- simpletest_semaphore is just an internal table to ensure not two processes try to gain the 'lock' to create the setup cache first. It is not part of the actual Drupal installation.

Leaving at CNW to revert parts of #51.

I am providing now a justification, why this can go into Drupal 7 even though it never entered Drupal 9:

- For Drupal 9 it's less useful, because we have minimal profile there so the (--cache) saves almost nothing
- --cache-modules would still be useful, but a large part of rebuilds is in the container, which varies a lot and is hard to cache (I tried really really hard but without further restrictions this just cannot be done efficiently)

So for Drupal 9 this is not really useful, but for Drupal 7 it saves a ton of time.

And even if we don't use it for testbot, anyone that still needs to run tests locally on D7 can profit from this.

So let's get this finally in and push it over the finish line.