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

Insert cleanup #54

Merged
merged 9 commits into from
Oct 16, 2018
Merged

Insert cleanup #54

merged 9 commits into from
Oct 16, 2018

Conversation

grigi
Copy link
Member

@grigi grigi commented Oct 2, 2018

This branch started with the goal of using prepared statements for inserts, but ended up being a general purpose insert/Client related cleanup.

Work done:

  • _prepare_insert_columns and _prepare_insert_values now do what they are called
  • Added type annotations to the DB Client classes (It is partial, due to some very confusing type usage, especially for "SingleConnectionsWrapper" and "TransactionWrapper" classes)
  • Split out specialised execute_insert() method
  • Decorators to translate exceptions, so we repeat less code.
  • Use macro executors from Added macros for two common cases of chained commands. omnilib/aiosqlite#13 for less synchronization overhead.
  • Generate prepared statements, and use values as a DB driver parameter (Building prepared statements kayak/pypika#163 is stalled, do something slightly hacky in the interm)(We need to try and get this into pypika, as asyncpg needs prepared statements to perform well)
  • Simple, but safe query cache for inserts

Not related to inserts:

  • Pre-generate initial pypika query object per model, instead of in queryset instantiation, as it is immutable (gives 25-50% speedup for small fetch operations)

This should be the last performance-oriented PR from me for now, performance is up over double (and much more for sqlite)

@coveralls
Copy link

coveralls commented Oct 2, 2018

Pull Request Test Coverage Report for Build 240

  • 186 of 187 (99.47%) changed or added relevant lines in 12 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.7%) to 91.466%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tortoise/backends/asyncpg/client.py 42 43 97.67%
Files with Coverage Reduction New Missed Lines %
tortoise/backends/mysql/client.py 1 85.08%
tortoise/backends/asyncpg/client.py 1 84.39%
Totals Coverage Status
Change from base Build 210: 0.7%
Covered Lines: 2048
Relevant Lines: 2181

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 214

  • 157 of 158 (99.37%) changed or added relevant lines in 8 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.6%) to 91.346%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tortoise/backends/asyncpg/client.py 39 40 97.5%
Files with Coverage Reduction New Missed Lines %
tortoise/backends/mysql/client.py 1 84.92%
tortoise/backends/asyncpg/client.py 1 84.12%
Totals Coverage Status
Change from base Build 210: 0.6%
Covered Lines: 2044
Relevant Lines: 2178

💛 - Coveralls

@grigi grigi force-pushed the feature/prepared_statements branch from 20e948f to dbf7646 Compare October 14, 2018 07:08
@grigi grigi changed the title Insert cleanup (WIP) Insert cleanup Oct 14, 2018
@grigi grigi requested a review from abondar October 14, 2018 07:49
@grigi grigi force-pushed the feature/prepared_statements branch from 235554f to c39b21c Compare October 14, 2018 15:18
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.

Wow, great PR! Thank you for hard work, seeing Tortoise speeding up little by little in benchmark really feels good.
Also - do you have account on pypi? I would like to add you as maintainer of package

# Insert should implement returning new id to saved object
# Each db has it's own methods for it, so each implementation should
# go to descendant executors
raise NotImplementedError() # pragma: nocoverage

async def execute_insert(self, instance):
self.connection = await self.db.get_single_connection()
key = '%s:%s' % (self.db.connection_name, self.model._meta.table)
Copy link
Member

Choose a reason for hiding this comment

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

Does this kind of formatting gives any significant speedup?
If not, I think following .format() would be better, cause it feels really much more pythonic way

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, no idea.
I tend to use the % formatting over the .format() out of habit.
I just did a quick search, and, yep. that is the only such code in the codebase. I'll change it.

@grigi
Copy link
Member Author

grigi commented Oct 16, 2018

Yes, people get excited about performance 😁
I have a pypi account. I'll forward you my credentials when I find them. Thanks for the offer 😄

@grigi grigi merged commit d136225 into master Oct 16, 2018
@abondar abondar deleted the feature/prepared_statements branch November 23, 2018 08:22
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