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

Improve region performance #1022

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Conversation

david-venhoff
Copy link
Member

Short description

This pr removes some database queries related to regions.
Related issue: #943

Proposed changes

  • For region list view: pass a list of regions to the paginator, not a query. This prevents an unnecessary SELECT Count(*) ... query. Is it worth doing this for every model list view?
  • Use select_related on the Region.default_language property. this reduces the number of queries on every view by one for every region which has Include administrative division into name checked. It would be possible to go even further and include the prefix as part of the name so that no additional queries are required at all, but the situation where the root language of a region gets changed would have to be handled correctly.

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! 👍

I had some major suggestions which I couldn't add via GitHub's UI, so I directly pushed a commit, I hope that's ok. The most significant change is a custom model manager for regions which always prefetches the default language of a region. I think this makes sense because nearly every time a region object is fetched from the database, sooner or later region.full_name is called and it comes in handy if the necessary information doesn't need to be fetched from the database.
We can't include the prefix as part of the name, because the app needs the name without prefix for their sorting in the region list (so e.g. "Stadt Augsburg" is listed under "A" instead of "S")... we only could store the prefix in its own charfield which doesn't need to be translated in the first place, but for now I would keep the database structure as it is and see how much performance improvement we can achieve by just adapting the queries.

Could you have a look at my changes and check whether you see any flaws or room for improvement? Thanks!

@david-venhoff
Copy link
Member Author

These changes look good to me. I think that one problem with this approach is that this could lead to unnecessary prefetch calls in the future if the language tree nodes are not required for some database calls (?), but I cannot think of a practical better approach.

- Add custom RegionManager to always prefetch the region's default language
- Update sphinxcontrib_django2 to version 1.3
- Monkeypatch RegionManager during documentation build to fix mptt errors

Co-authored-by: Timo Ludwig <ludwig@integreat-app.de>
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

I think that one problem with this approach is that this could lead to unnecessary prefetch calls in the future if the language tree nodes are not required for some database calls (?), but I cannot think of a practical better approach.

Yes you're right, this might become a problem. But we can always define a second model manager without the prefetching and use that one if needed. At the moment, I think it's important to focus on potential bottlenecks when multiple regions are loaded (e.g. in the region list, the region API or in the model choice fields where regions can be selected) and in all of these cases, the full_name is used afterwards. If there are views where only a single region is requested (without a call to full_name), it's no big deal when one single useless query is performed... it only becomes a problem, if the same thing happens for n regions etc... but yeah, let's keep an eye on this.

For now, I'd suggest to merge this PR, put a check mark behind "regions" and move on to the next model 😉

@timobrembeck timobrembeck merged commit e8ba3c9 into develop Nov 23, 2021
@timobrembeck timobrembeck deleted the feature/improve_region_queries branch November 23, 2021 00:38
@david-venhoff david-venhoff mentioned this pull request Jan 4, 2022
16 tasks
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.

None yet

2 participants