Fix IssuesController#show degradation including project on loaded notes
What does this MR do?
Resolve N+1 problem with notes and project on IssuesController#show action as detected here
On an issue with 8 notes, using sherlock we make 50 database queries instead 71
Are there points in the code the reviewer needs to double check?
Why was this MR needed?
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
CHANGELOG entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if you do - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Merge request reports
Activity
@yorickpeterse one thing I notice lately is that we create a lot of instances of the same projects during the same requests. For example on if we do the following
project = Project.find_with_namespace("gitlab-org/gitlab-ce") # a project instance is created issue = project.issues.find_by(iid: 124) # an issue instance is created issue.project # returns the previous project instance thanks to inverse_of notes = issue.notes.includes(:project) notes.first.project # is a different project instance (the same for all the notes because we use includes)
I'm not sure if this is a problem, but anytime we use a project instance we instantiate its
project_feature
association and probably try to get its group. So if we manage different instance there are a lot of queries we do per each of those instances. And I don't know if there is something we can do along the codebase to avoid this thing.@pacoguzman I don't think there's a way around that either unless we start hacking ActiveRecord.
@pacoguzman This MR looks good but there's a merge conflict.
Milestone changed to %8.13
Milestone changed to %8.12
Added 15 commits:
-
921b1b95...3820ca58 - 14 commits from branch
master
- 256dfa13 - Fix IssuesController#show degradation including project on loaded notes
-
921b1b95...3820ca58 - 14 commits from branch
@yorickpeterse rebase against master it seems can be merged automatically now
Edited by Paco GuzmanThanks for handling this so quickly, @pacoguzman!
You're welcome @stanhu this was an easy one
@pacoguzman Out of curiosity, any ideas how this started happening?
Marked the task CHANGELOG entry added as completed
Marked the task Documentation created/updated as completed
Marked the task Conform by the merge request performance guides as completed
Marked the task Conform by the style guides as completed
Marked the task Squashed related commits together as completed
Mentioned in commit 52711b53
Mentioned in issue #22556 (closed)
Mentioned in commit 712bd173
Mentioned in issue #23342 (closed)