-
-
Notifications
You must be signed in to change notification settings - Fork 860
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
Optimise N+1 end points #676
Comments
I think the cause of the I just realized we can get some insight into some of the |
Yea, I've been playing with https://github.com/djsutho/django-debug-toolbar-request-history as a means to inspect both POSTS and the AJAX requests which has been working well.
The problems with SavePanel seem to come from the act of returning a new serialised debate means having to recalculate the feedback score for adjs and the liveness score for teams. It should be possible to more tactically construct the new serialisation so that it pulls those values from the object it receives.
…On 5 Jul 2018, 10:27 AM +1000, Chuan-Zheng Lee ***@***.***>, wrote:
I think the cause of the TournamentAdminHomeView and TournamentAssistantHomeView is grabbing the content object in the action logs, which I think can't be done easily and actually just indicates that our schema for action logs is poorly designed.
I just realized we can get some insight into some of the POST requests by intercepting redirects with the debug toolbar, so might try that some time.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Re serialisation, you might remember this discussion from here: 3545827#commitcomment-28040144 I had the feeling our serialisation framework causes a lot of duplicate and redundant queries, I think we need to think more thoroughly through how to get it to fetch everything we want, and only want we want, efficiently? |
Yea definitely. Although it might be worth delaying until it is pushing changes back in an async fashion (although this would probably use a similar serialisation method so maybe not). When done to produce a serialised draw bulk I think the current methods aren't that inefficient (maybe I'm wrong here) but when done to update just a single debate it seems inefficient. Either the fetch/re-serialise query could be combined into a singular query and/or we could pre-populated 'expensive' attributes like adj scores/liveness from the JSON provided which would remove the need to query them (although at the cost of information not being completely live). |
Hmm, I think it's the other way round. You generally can't make updating a single debate any more efficient, because (a) for the debate itself, there's no concept of an N+1 problem if you're only dealing with one row and (b) for things the debate has many of, like teams, speaker scores and team scores, in general (If you have an N+1 problem with an update, there are ways in SQL to make it more efficient, but as far as I can tell you can't collapse it into a single statement, I think you have to use I'm not fully confident about this assessment, but last time I looked at it, I felt like serialising large amounts of data had a bunch of N+1 issues (3545827). I think we just want to revise the framework—having |
While I think information could be definitely prefetched better in whatever method is used to produce serialisations, for all intents and purposes hitting the Any performance problems with the mass-serialisation (i.e. on initial page load) are probably less impactful as they aren't being accessed constantly. Or at least they don't have the N+1 issues flagged by scout. |
Do you remember what the N+1 queries for I get a little nervous about bulk updates, or even bulk creates, because they bypass the |
Um I don't have a copy saved, but looking at the queries in debug toolbar (using the Request History addon linked above) the duplicated queries are:
|
Whoa okay, the duplication's not really bothersome but that's a lot of different tables for a view that's just meant to save a debate panel. |
Just added
Called 372 times (presumably once per speaker), I presume from:
I guess there is some optimisation possible where the updates are issued in bulk rather than through looping. |
|
Update: I have constructed gotten the query part into a single |
|
Also btw @tienne-B or @tfpk if you are handy with optimising SQL query usage (I suspect most cases just need a strategic |
I'll try some (will improve my Django/queries skills). Already have `AdjudicatorFeedback` views fixed.
|
Improve queries in adjudicator feedback views as part of #676
(Reposted this comment to tag @czlee) For whenever you had time to look at this:
|
|
Regarding atomic transactions, just posting a link to the comment thread at 7a67893#r29956843. TL;DR: Atomic transactions won't reduce round trips to the database and we shouldn't use them for performance purposes. More generally, I sort of think we shouldn't try to optimise N+1 issues involving
Regarding the debug line, I definitely agree that debug lines shouldn't necessitate prefetches, I'll have a look at it and consider removing or simplifying the debug line. The |
Details for a few recent additions (tournament rounds = 6; rooms = 9):
|
Currently get_side_names and record_links on record pages lookup objects per team/adj. Given this is always going to be for the tournament that the page is attached to it seems better to pass that value
Re Despite this proof of concept, as explained above, I'd prefer that for database writes ( |
Yea, for both create draw and auto-allocate they aren't exactly commonly-hit functions so there isn't a general need to speed their performance at the cost of reliability. The only concern is if they run up against the dyno timeout but that is probably better handled by shifting their calculation to Celery. |
This is an ongoing tracker for identifying/discussing these views, although scout doesn't actually identify many over the 150ms threshold so they could definitely all be ticked off. None are egregious in terms of execution time (except for
/database/participants/adjudicator/
) but it would be nice to improve them:results.views.PublicResultsForRoundView
participants.views.AdjudicatorRecordView
+participants.views.PublicTeamRecordView
tournaments.views.TournamentAdminHomeView
+tournaments.views.TournamentAssistantHomeView
adjallocation.views.CreateAutoAllocation
draw.views.CreateDrawView
results.views.AdminNewBallotSetView
+results.views.AdminEditBallotSetView
adjallocation.views.SaveDebatePanel
django.contrib.admin.options.changelist_view
for/database/participants/adjudicator/
participants.views.UpdateEligibilityEditView
/django.contrib.admin.options.change_view
aka/database/adjfeedback/adjudicatorfeedback/740/change/
venues.views.AutoAllocateVenuesView
adjfeedback.views.FeedbackByTargetView
adjfeedback.views.FeedbackBySourceView
printing.views.AdminPrintScoresheetsView
adjfeedback.views.PublicAddFeedbackByRandomisedUrlView
The text was updated successfully, but these errors were encountered: