Make parallel testing db faster #33479

twitnithegirl · 2018-07-31T01:52:41Z

Summary

Currently TestDatabases.create_and_migrate for parallel tests uses migrate. For a large number of workers this makes the tests run pretty slowly. The PR changes this to load_schema which shows an increase of speed.

Benchmarks below are for a sample app with 6 tables, 10 workers, and generated tests.

Before:
After:

/cc @eileencodes

rails-bot · 2018-07-31T01:52:44Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

twitnithegirl · 2018-07-31T03:08:04Z

I'm just dropping the CI failures here so I know what to look at tomorrow:

Ruby 2.4.4, 2.5.1 & Head GEM-Railties:

.F
Failure:
ApplicationTests::TestTest#test_hooks_for_plugins [test/application/test_test.rb:319]:
Expected "before: false\n-- create_table(:users)\n   -> 0.0036s\nafter: true\nRun options: --seed 33770\n\n# Running:\n\n.\n\nFinished in 0.010207s, 97.9722 runs/s, 0.0000 assertions/s.\n1 runs, 0 assertions, 0 failures, 0 errors, 0 skips\n" to include "before: false\nafter: true".
rails test test/application/test_test.rb:281
.F
Failure:
ApplicationTests::TestTest#test_ruby_schema_migrations [test/application/test_test.rb:143]:
`Expected "-- create_table(:users)\n   -> 0.0054s\nRun options: --seed 8880\n\n# Running:\n\n.\n\nFinished in 0.016965s, 58.9435 runs/s, 0.0000 assertions/s.\n1 runs, 0 assertions, 0 failures, 0 errors, 0 skips\n" to not include "create_table(:users)".

rafaelfranca · 2018-07-31T03:07:47Z

activerecord/lib/active_record/test_databases.rb

    ensure
      ActiveRecord::Base.establish_connection(ActiveRecord::Base.configurations[Rails.env])
      ENV["VERBOSE"] = old
    end

-    def self.drop(i, spec_name:)
+    def self.drop(spec_name:)


Should not the i be used here?

It took me a couple of looks to see why this is okay: create_and_migrate modifies the original connection spec hash, so the rest of the (forked) app, including this method, sees that already-suffixed DB name.

Yeah, that is confusing.

I had put the i in when I wrote this and while pairing with Britni on fixing this realized that it isn't used so we should delete it.

rafaelfranca · 2018-07-31T03:08:24Z

activerecord/lib/active_record/tasks/database_tasks.rb

@@ -247,6 +247,7 @@ def structure_load(*arguments)

      def load_schema(configuration, format = ActiveRecord::Base.schema_format, file = nil, environment = env, spec_name = "primary") # :nodoc:
        file ||= dump_filename(spec_name, format)
+        verbose_was, Migration.verbose = Migration.verbose, verbose?


Is Migration.verbose thread-safe?

Is any of this? It runs in a fork, so I think it should be fine.

But doesn't the parallel test also have the threaded mode? I guess this don't run on those threads but before it in a fork.

Yes it does have a threaded mode, but we don't make new databases when we're using the threaded parallelizer because it uses new connections instead.

Why do we need these changes for #load_schema? Is this method supposed to work with migrations?
If remove these change, tests in railties/test/application/test_test.rb succeed. See #33479 (comment)

If we don't do this running your tests in parallel will display schema output for every single process which is noisy and unnecessary.

kaspth · 2018-07-31T07:56:18Z

Cool! What happens if I create a migration that adds some columns, forget to migrate the test database, and then run my tests. Do the tests fail in a sufficiently obvious manner that I can know I need to run the migration?

eileencodes · 2018-07-31T12:24:05Z

@kaspth this doesn't change how your tests would run if you didn't run a migration. You'd still get blocked by pending migrations error because the create_and_migrate part runs after the tests boot. This just speeds up setting up the databases for test-database-0 and test-database-1 and doesn't affect test-database.

eileencodes · 2018-07-31T12:24:55Z

activerecord/lib/active_record/test_databases.rb

-    ActiveSupport::Testing::Parallelization.run_cleanup_hook do |i|
-      drop(i, spec_name: Rails.env)
+    ActiveSupport::Testing::Parallelization.run_cleanup_hook do |_|
+      drop(spec_name: Rails.env)
    end

    def self.create_and_migrate(i, spec_name:)


Let's also rename this method to create_and_load since we're no longer migrating.

Got it! Thanks.

rafaelfranca · 2018-07-31T14:56:28Z

Thank you @eileencodes for the clarifications, now I understand better the parallel test implementation.

Nice improvement @twitnithegirl 👏.

twitnithegirl · 2018-07-31T17:57:25Z

@rafael @eileencodes @kaspth I just pushed a change that I think will fix CI for railties/application/test_test.rb. But I feel like we're making some strange assumptions. It seems that we assume that migrations are verbose by default but that schema loading is silent by default. If you don't set ENV['VERBOSE'] then verbose? evaluates to true because anything but "false" evaluates to true. The tests, at least in railties, assume that not setting verbose will result in shema_loads being silent. Granted, before we weren't even bothering with Migration.verbose at all in the schema_load but I think that verbose? itself is misleading, or at least has some disjointed logic in practice.

eileencodes · 2018-07-31T22:24:16Z

@twitnithegirl can you rebase and squash your commits into one?

twitnithegirl · 2018-08-01T01:20:41Z

@eileencodes sure, should I update the changelog too?

twitnithegirl · 2018-08-01T01:26:56Z

All squashed up

eileencodes · 2018-08-01T12:13:12Z

Thanks @twitnithegirl - no need for a changelog since this is a performance improvement and not actually in a version of Rails yet.

rails#33479 changed `#load_schema` to prevent displaying schema load on running tests in parallel. We should test this in order to prevent any regression in the future. Context rails#33479 (comment)

- Fix formatting - Don't repeat "Active Record automatically handles creating and migrating a new database for each worker to use." - Tell that AR loads the schema to a database for each process(Related to rails#33479) - Clarify that `parallelize_teardown` is executed for each process

rails-bot assigned kaspth Jul 31, 2018

rafaelfranca reviewed Jul 31, 2018

View reviewed changes

eileencodes reviewed Jul 31, 2018

View reviewed changes

use load_schema instead of migrate for parallel testing

e6173c9

twitnithegirl force-pushed the make_parallel_testing_db_faster branch from 6ac0e99 to e6173c9 Compare August 1, 2018 01:26

eileencodes merged commit 78992d6 into rails:master Aug 1, 2018

bogdanvlviv mentioned this pull request Aug 8, 2018

Ensure that running tests in parallel doesn't display schema load output #33553

Merged

bogdanvlviv mentioned this pull request Dec 2, 2018

Improve parallel testing guide [ci skip] #34596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make parallel testing db faster #33479

Make parallel testing db faster #33479

twitnithegirl commented Jul 31, 2018 •

edited

rails-bot commented Jul 31, 2018

twitnithegirl commented Jul 31, 2018 •

edited

rafaelfranca Jul 31, 2018

matthewd Jul 31, 2018

rafaelfranca Jul 31, 2018

eileencodes Jul 31, 2018

rafaelfranca Jul 31, 2018

matthewd Jul 31, 2018

rafaelfranca Jul 31, 2018

eileencodes Jul 31, 2018

bogdanvlviv Aug 1, 2018

eileencodes Aug 1, 2018

kaspth commented Jul 31, 2018

eileencodes commented Jul 31, 2018

eileencodes Jul 31, 2018

twitnithegirl Jul 31, 2018

rafaelfranca commented Jul 31, 2018

twitnithegirl commented Jul 31, 2018 •

edited

eileencodes commented Jul 31, 2018

twitnithegirl commented Aug 1, 2018

twitnithegirl commented Aug 1, 2018

eileencodes commented Aug 1, 2018

Make parallel testing db faster #33479

Make parallel testing db faster #33479

Conversation

twitnithegirl commented Jul 31, 2018 • edited

Summary

rails-bot commented Jul 31, 2018

twitnithegirl commented Jul 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth commented Jul 31, 2018

eileencodes commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented Jul 31, 2018

twitnithegirl commented Jul 31, 2018 • edited

eileencodes commented Jul 31, 2018

twitnithegirl commented Aug 1, 2018

twitnithegirl commented Aug 1, 2018

eileencodes commented Aug 1, 2018

twitnithegirl commented Jul 31, 2018 •

edited

twitnithegirl commented Jul 31, 2018 •

edited

twitnithegirl commented Jul 31, 2018 •

edited