-
Notifications
You must be signed in to change notification settings - Fork 17
perf: select related objects in container queries #333
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: select related objects in container queries #333
Conversation
|
Thanks for the pull request, @pomegranited! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
In c971295 I have some further work on this branch that takes the queries of the main test in edx-platform down a bit further, from 225 to 196. But I'm not sure the added complexity is worth the cost. Many of these queries are just loading a single object, which are always going to be very fast queries anyways. We really want to focus on select_related where it's loading many rows of objects at once. This PR does include some of that though. |
|
In c971295 I have some further work on this branch that takes the queries of the main test in edx-platform down a bit further, from 225 to 196. But I'm not sure the added complexity is worth the cost. Yeah, I wasn't sure about muddying the API either.. I chose to continue adding things to the hard-coded |
8d37a0a to
ddc65e9
Compare
to improve performance when fetching lots of children / parents
improves performance a little bit more.
6e14bbf to
666ae68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. I haven't tested it though.
| select_related_version: An optional optimization; specify a relationship | ||
| on ContainerVersion, like `componentversion` or `containerversion__x` | ||
| to preload via select_related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think a single relationship is going to be sufficient here, or will we have some use cases where we need to specify multiple relationships to select, like ["containerversion__subsectionversion", "containerversion__unitversion"] ?
I'm guessing that since this is only loading a single level at a time, a single string like you have is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that since this is only loading a single level at a time, a single string like you have is sufficient.
I think that's the case for now. We can update it later (or move to something like what is discussed here: openedx/edx-platform#36813 (comment)).
|
Hi @ormsbee! Sorry for the ping. |
Description
Improves performance when fetching lots of container children / parents.
Supporting information
Part of:
Blocks:
Private-ref: FAL-4178
Testing instructions
See openedx/frontend-app-authoring#2186