Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: squash all migrations #2598

Merged
merged 6 commits into from
Aug 20, 2024

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Aug 9, 2024

A prep step for upgrading to Laravel 11. (ref)

When modifying a column, you must now explicitly include all the modifiers you want to keep on the column definition after it is changed. Any missing attributes will be dropped ... In Laravel 10, this migration would retain the unsigned, default, and comment attributes on the column. However, in Laravel 11, the migration must now also include all of the attributes that were previously defined on the column. Otherwise, they will be dropped.

If you do not want to update all of the existing "change" migrations in your application to retain the column's existing attributes, you may simply squash your migrations.

I've confirmed that migrating a fresh database still works. All migrations in platform, community and the migrations root folders have been deleted. I left upcoming alone. In the platform and community folders, I added .gitkeep files so the empty folders wouldn't get blown away by git.

By squashing migrations, we also manage to bring down non-parallel test execution time by roughly 33%. On my machine it decreases from 180s to 120s. Parallel execution time decreases from 30s to 21s.

Additionally, I've added Pest as a dependency. Pest seems to magically take over test execution when installed -- I found during local benchmarks that this also made a ~5% improvement in test execution time.

Performance improvements are noticeable in CI as well.

Once we are on Laravel 11, we should get another significant improvement in sqlite testing performance.

Before
Screenshot 2024-08-09 at 10 24 57 AM

After
Screenshot 2024-08-09 at 10 25 27 AM

@ioslife
Copy link
Contributor

ioslife commented Aug 9, 2024

Let's gooo. That time improvement is huge

@wescopeland
Copy link
Member Author

@Jamiras One thing I'm curious about when you review this PR: are you able to run tests in parallel mode?

composer test -- --parallel

I believe Pest uses a different parallel testing engine than PHPUnit does. I am not sure how a Windows machine will react when parallel testing mode is used, but if things are fine, we may be able to switch the pre-push hook to use parallel testing.

@Jamiras
Copy link
Member

Jamiras commented Aug 17, 2024

I am not sure how a Windows machine will react when parallel testing mode is used...

I still run RAWeb in a Ubuntu VM within Windows, so I doubt I can provide the information you're looking for.

Copy link
Member

@Jamiras Jamiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I did see a significant performance increase using the squashed migrations, I didn't see much of a difference moving to pest.

php artisan test (master): 157.34s
php artisan test (after): 102.13s
vendor/bin/pest: 100.75s

php artisan test --parallel (master): 43.47s
php artisan test --parallel (after): 29.85s
vendor/bin/pest --parallel: 27.49s

I've also confirmed that sail migrated:fresh --seed generates a workable site.

database/schema/sqlite-schema.sql Outdated Show resolved Hide resolved
database/schema/sqlite-schema.sql Outdated Show resolved Hide resolved
database/schema/mysql-schema.sql Show resolved Hide resolved
@wescopeland
Copy link
Member Author

Sorry @Jamiras, I think I managed to confuse myself very much here.

I think the issues should be addressed now though. My intention was to also squash 2024_08_07_000000_update_emailconfirmations_table.php. I've confirmed migrating a fresh database & tests are now all working again.

@wescopeland wescopeland merged commit 10087ca into RetroAchievements:master Aug 20, 2024
5 checks passed
@wescopeland wescopeland deleted the squash-migrations branch August 20, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants