Optimize issuable updates (!58114) · Merge requests · GitLab.org / GitLab

Markus Koller requested to merge 21068-optimize-issueable-updates into master Mar 31, 2021

What does this MR do?

Reduce N+1 queries when bulk-updating
Avoid looking up blank milestones in the hierarchy
Use .size to avoid a redundant .count query

Related to #21068

Query count differences in spec/services/issuable/bulk_update_service_spec.rb

--- before.txt	2021-04-08 16:29:44.209679331 +0200
+++ after.txt	2021-04-08 16:31:39.262232001 +0200
@@ -5,113 +5,113 @@
 Issuable::BulkUpdateService
   with issuables at a project level
     with unpermitted attributes
-DB queries: 33
+DB queries: 25
       does not update the issues
     close issues
-DB queries: 102
+DB queries: 81
       succeeds and returns the correct number of issues updated
-DB queries: 102
+DB queries: 81
       closes all the issues passed
     reopen issues
-DB queries: 86
+DB queries: 65
       succeeds and returns the correct number of issues updated
-DB queries: 86
+DB queries: 65
       reopens all the issues passed
     updating merge request assignee
       when the new assignee ID is a valid user
-DB queries: 108
+DB queries: 110
         succeeds
-DB queries: 107
+DB queries: 109
         updates the assignee to the user ID passed
       when the new assignee ID is 0
-DB queries: 73
+DB queries: 75
         unassigns the issues
       when the new assignee ID is not present
-DB queries: 24
+DB queries: 26
         does not unassign
     updating issue assignee
       when the new assignee ID is a valid user
-DB queries: 78
+DB queries: 72
         succeeds
-DB queries: 78
+DB queries: 72
         updates the assignee to the user ID passed
       when the new assignee ID is 0
-DB queries: 53
+DB queries: 47
         unassigns the issues
       when the new assignee ID is not present
-DB queries: 17
+DB queries: 19
         does not unassign
     updating milestones
       behaves like updates milestones
-DB queries: 42
+DB queries: 37
         succeeds
-DB queries: 42
+DB queries: 37
         updates the issuables milestone
     updating labels
       behaves like updating labels
         when add_label_ids are passed
-DB queries: 114
+DB queries: 85
           adds those label IDs to all issues passed
-DB queries: 114
+DB queries: 85
           does not update issues not passed in
         when remove_label_ids are passed
-DB queries: 106
+DB queries: 78
           removes those label IDs from all issues passed
-DB queries: 106
+DB queries: 78
           does not update issues not passed in
         when add_label_ids and remove_label_ids are passed
-DB queries: 134
+DB queries: 103
           adds the label IDs to all issues passed
-DB queries: 134
+DB queries: 103
           removes the label IDs from all issues passed
-DB queries: 134
+DB queries: 103
           does not update issues not passed in
     subscribe to issues
-DB queries: 47
+DB queries: 39
       subscribes the given user
     unsubscribe from issues
-DB queries: 47
+DB queries: 39
       unsubscribes the given user
     updating issues from external project
-DB queries: 77
+DB queries: 70
       updates only issues that belong to the parent project
   with issuables at a group level
     updating milestones
       when issues
         behaves like updates milestones
-DB queries: 99
+DB queries: 78
           succeeds
-DB queries: 99
+DB queries: 78
           updates the issuables milestone
       when merge requests
         behaves like updates milestones
-DB queries: 135
+DB queries: 118
           succeeds
-DB queries: 135
+DB queries: 118
           updates the issuables milestone
     updating labels
       behaves like updating labels
         when add_label_ids are passed
-DB queries: 130
+DB queries: 101
           adds those label IDs to all issues passed
-DB queries: 130
+DB queries: 101
           does not update issues not passed in
         when remove_label_ids are passed
-DB queries: 122
+DB queries: 98
           removes those label IDs from all issues passed
-DB queries: 122
+DB queries: 98
           does not update issues not passed in
         when add_label_ids and remove_label_ids are passed
-DB queries: 152
+DB queries: 121
           adds the label IDs to all issues passed
-DB queries: 152
+DB queries: 121
           removes the label IDs from all issues passed
-DB queries: 152
+DB queries: 121
           does not update issues not passed in
     with issues from external group
-DB queries: 103
+DB queries: 87
       updates issues that belong to the parent group or descendants
 
-Finished in 46.89 seconds (files took 9.15 seconds to load)
+Finished in 42.11 seconds (files took 15.99 seconds to load)
 37 examples, 0 failures

Query count differences in ee/app/services/ee/issuable/bulk_update_service.rb

--- before_ee.txt	2021-04-12 18:28:25.602398912 +0200
+++ after_ee.txt	2021-04-12 18:31:09.266918261 +0200
@@ -1,57 +1,57 @@
 Run options: include {:focus=>true}
 
 All examples were filtered out; ignoring {:focus=>true}
 
 Issuable::BulkUpdateService
   with issues
     updating health status and epic
       when features are enabled
 DB queries: 160
         succeeds and returns the correct number of issuables updated
         when params value is '0'
 DB queries: 80
           succeeds and remove values
         when epic param is incorrect
           returns error
       when feature issuable_health_status is disabled
         behaves like does not update issuables attribute
           does not update attribute
       when user can not update issues
         behaves like does not update issuables attribute
           does not update attribute
         behaves like does not update issuables attribute
           does not update attribute
       when user can not admin epic
         behaves like does not update issuables attribute
           does not update attribute
     updating iterations
       at group level
         when issues
           behaves like updates iterations
 DB queries: 58
             succeeds
 DB queries: 59
             updates the issuables iteration
       at project level
         behaves like updates iterations
 DB queries: 35
           succeeds
 DB queries: 36
           updates the issuables iteration
   with epics
     updating labels
       when epics are enabled
         behaves like updates issuables attribute
-DB queries: 59
+DB queries: 58
           succeeds and returns the correct number of issuables updated
       when epics are disabled
         behaves like does not update issuables attribute
-DB queries: 7
+DB queries: 9
           does not update attribute
       when issuable_ids contain external epics
-DB queries: 59
+DB queries: 58
         updates epics that belong to the parent group or descendants
 
-Finished in 18.77 seconds (files took 2.73 seconds to load)
+Finished in 18.16 seconds (files took 7.29 seconds to load)
 14 examples, 0 failures

Debugging code to generate the above

diff --git i/app/services/issuable/bulk_update_service.rb w/app/services/issuable/bulk_update_service.rb
index 13e289716ef..9e226235bf8 100644
--- i/app/services/issuable/bulk_update_service.rb
+++ w/app/services/issuable/bulk_update_service.rb
@@ -13,7 +13,13 @@ def initialize(parent, user = nil, params = {})
     def execute(type)
       ids = params.delete(:issuable_ids).split(",")
       set_update_params(type)
-      items = update_issuables(type, ids)
+
+      items = nil
+      queries = ActiveRecord::QueryRecorder.new do
+        items = update_issuables(type, ids)
+      end
+
+      $stdout.puts "DB queries: #{queries.count}"
 
       response_success(payload: { count: items.size })
     rescue ArgumentError => e

Does this MR meet the acceptance criteria?

Conformity

📋 Does this MR need a changelog?
- I have included a changelog entry.
[-] Documentation (if required)
Code review guidelines
Merge request performance guidelines
Style guides
Database guides
[-] Separation of EE specific content

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
[-] Tested in all supported browsers
[-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

[-] Label as security and @ mention @gitlab-com/gl-security/appsec
[-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
[-] Security reports checked/validated by a reviewer from the AppSec team

Edited Apr 12, 2021 by Markus Koller

Optimize issuable updates

What does this MR do?

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Merge request reports