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

Split model consructor into from-Python and from-DB paths, leading to 15-25% speedup for large fetch operations. #158

Merged
merged 5 commits into from
Jul 21, 2019

Conversation

grigi
Copy link
Member

@grigi grigi commented Jul 19, 2019

So, this PR started off as me trying to add a post-process hook to model creation, so that we can try and have a single place to do generate pre-compute lookups.

Then I saw all the memoised properties, and thought that pre-generating this would be a good exercised of if the post-process hooks get called correctly.

It actually turns out we have 4 places where these updates can happen:

  1. Model parsed
  2. Model initialised (for pk) → This one feels non-obvious to me.
  3. After relationships created
  4. DB associated (still done in Tortoise.init(), this PR does not change this)

Also split constructor into from-python and from-db so we can unblock #55

This lead to significant perf improvements in the code, especially for large selects.

ALSO:

Manipulating PyPika objects directly, 5-30% speedup for small fetches.
Not too sure about this, as it is touching some private variables. Its safe as we manually copy the PyPika object before manipulating it, but now we ALWAYS copy once, instead of for every transform.

@grigi grigi requested a review from abondar July 19, 2019 12:24
@grigi
Copy link
Member Author

grigi commented Jul 19, 2019

@abondar Please review

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.

Seems legit to me

@@ -517,6 +508,7 @@ def _make_query(self):
value = value.id
else:
db_field = self.model._meta.fields_db_projection[key]
# self.query._updates.append((Field(db_field), ValueWrapper(value))
Copy link
Member

Choose a reason for hiding this comment

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

I think that line should be deleted :)

@grigi grigi merged commit 5f6bd8c into master Jul 21, 2019
@grigi grigi deleted the metadata_perf branch August 24, 2019 05:15
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.

2 participants