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

Optimize field conversion to database format #1840

Merged
merged 7 commits into from
Jan 10, 2025

Conversation

henadzit
Copy link
Contributor

@henadzit henadzit commented Jan 4, 2025

Description

  • Remove TO_DB_OVERRIDE that is no longer needed after parametrization changes. The only exception is SQLite for which sqlite3.register_adapter was added - this is the same approach used by Django.
  • Get rid of BaseExecutor.column_map. It was only required because of TO_DB_OVERRIDE.
  • Cache timezone.get_use_tz, timezone.get_timezone and timezone.get_default_timezone. Since these are called for each field with auto_now or auto_now_add, it provides a significant speedup.

Motivation and Context

This PR improves the performance of field value conversion to the DB format (see the detailed report in the comments):

  • 52% improvement in the test_bulk_create_many_fields benchmark
  • 13% improvement in test_bulk_create_few_fields
  • 9% improvement in test_create_many_fields

The speedup is primarily achieved by the removal of self.model._meta.db from the Field. to_db_value code. The db property gets the current connection from ContextVar and it was a relatively slow operation to do it for each field for each object in bulk_create.

How Has This Been Tested?

make ci

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link

codspeed-hq bot commented Jan 4, 2025

CodSpeed Performance Report

Merging #1840 will improve performances by 52.82%

Comparing henadzit:chore/to_db_value_unused-code (6562ce4) with develop (8b5ac14)

Summary

⚡ 2 improvements
✅ 6 untouched benchmarks

Benchmarks breakdown

Benchmark develop henadzit:chore/to_db_value_unused-code Change
test_bulk_create_few_fields 3 ms 2.6 ms +13.12%
test_bulk_create_many_fields 17.3 ms 11.3 ms +52.82%

@coveralls
Copy link

coveralls commented Jan 5, 2025

Pull Request Test Coverage Report for Build 12631809925

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 6 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 90.252%

Files with Coverage Reduction New Missed Lines %
tortoise/backends/mssql/executor.py 1 83.33%
Totals Coverage Status
Change from base Build 12617761088: 0.1%
Covered Lines: 6427
Relevant Lines: 7005

💛 - Coveralls

@henadzit henadzit force-pushed the chore/to_db_value_unused-code branch 2 times, most recently from 201554f to 002513f Compare January 5, 2025 21:26
@henadzit henadzit force-pushed the chore/to_db_value_unused-code branch from 002513f to bafb6f7 Compare January 5, 2025 21:38
@henadzit henadzit force-pushed the chore/to_db_value_unused-code branch from 0535db7 to 80684cc Compare January 5, 2025 23:08
@henadzit henadzit changed the title WIP Remove unnecessary code from to_db_value Optimize field conversion to database format Jan 6, 2025
@henadzit henadzit marked this pull request as ready for review January 6, 2025 11:37
@henadzit henadzit requested a review from abondar January 6, 2025 11:52
@henadzit
Copy link
Contributor Author

@abondar any chance you can have a look at this? Thanks!

Copy link
Member

@abondar abondar left a comment

Choose a reason for hiding this comment

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

Good job!

@henadzit henadzit merged commit 46e3aef into tortoise:develop Jan 10, 2025
10 checks passed
@henadzit henadzit mentioned this pull request Jan 22, 2025
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