-
Notifications
You must be signed in to change notification settings - Fork 27
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
[#5077] Improve performance in loading logic rules in admin #5078
[#5077] Improve performance in loading logic rules in admin #5078
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5078 +/- ##
=======================================
Coverage 96.73% 96.73%
=======================================
Files 771 771
Lines 26597 26619 +22
Branches 3460 3463 +3
=======================================
+ Hits 25729 25751 +22
Misses 606 606
Partials 262 262 ☔ View full report in Codecov by Sentry. |
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 think the variables bulk/list endpoint may have similar issues, iirc that one also showed up in APM. You can give it the same treatment (notably the FormVariable.form
and FormVariable.form_definition
foreign keys may cause n+1 queries here)
src/openforms/forms/api/viewsets.py
Outdated
# See github issue https://github.com/open-formulieren/open-forms/issues/5077 | ||
logic_rules = form.formlogic_set.select_related( | ||
"form", "trigger_from_step__form" | ||
) |
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.
have you tried prefetch_related
too instead of select_related
?
Select related performs an inner join and results in more data going over the wire between database and application, especially here because the joined form will always be same as the form we already retrieved (meaning that 55 logic rules result in 55 times the same form being joined).
For the trigger_from_step, I expect a similar situation - it's more likely that multiple different logic rules have the same trigger_from_step, so joining can be slower than doing a prefetch. And of course, the related form
of the trigger_from_step
has the same problem as the form
FK of the logic rule.
I think that all three of these fields (form, trigger_from_step and trigger_from_step__form) might benefit from a prefetch - it will result in a couple more queries, but it will be a fixed amount and the data going over the wire could be a lot less, so I'm interested in the performance profile of that approach.
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 didn't notice any difference concerning the update endpoints for both logic-rules and variables (tried with both joins and prefetch)
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.
You specifically mention the update endpoints, but I'm interested in the list endpoints in particular :P
However, looking at your screenshots, prefetch_related
seems to be a clear winner!
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 think the variables bulk/list endpoint may have similar issues, iirc that one also showed up in APM. You can give it the same treatment (notably the
FormVariable.form
andFormVariable.form_definition
foreign keys may cause n+1 queries here)
This is why I mentioned it.I understood that we wanted to see what's going on with these as well. But clearly wrong place to write this, it should be as a reply to the above.
45b71ff
to
46103a7
Compare
Had to apply prefetch here as too many queries (n+1) were taking place for the variables and logic-rules list endpoints and affected the performance.
46103a7
to
7a56bce
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.
⚡ LETSGO
Closes #5077
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh
./bin/compilemessages_js.sh
Dockerfile/scripts
./bin
folderCommit hygiene