Move code owner group loading to db layer
What does this MR do and why?
Simplifies the group loading logic for code owners.
Issues in old implementation:
-
applicable_ancestorscalledActiveRecord::Relation#selectwith a block to match values which behaves like Array#select causing the logic to iterate over the whole collection. - Called
applicable_ancestorstwice without memoizing method. - Converting group_list to array and then calling uniq before returning.
Changes:
- Now we use
Group.from_unionwhich automatically removes duplicates, removing the need for us to iterate overgroup_listandapplicable_ancestors -
applicable_ancestorsmethod completely removed, less debt and also no need to memoize. - Added
feature_category: :source_code_managementto all code owner specs
Benchmark locally
Benchmarked locally with the following code.
N = 1000
module Before
module_function
def load_groups(project, extractor)
return Group.none if extractor.names.empty?
groups = project.invited_groups.where_full_path_in(extractor.names);
group_list = groups.with_route.with_users.to_a;
if project.group
# If the project.group's ancestor group(s) are listed as owners, add
# them to group_list
#
if applicable_ancestors(project, extractor.names).any?
group_list.concat(applicable_ancestors(project, extractor.names));
end
end
group_list.uniq;
end
def applicable_ancestors(project, extractor_names)
ancestor_groups = project.group.self_and_ancestors.with_route.with_users;
ancestor_groups.select { |group| extractor_names.include?(group.full_path) };
end
end
module After
module_function
def load_groups(project, extractor)
return Group.none if extractor.names.empty?
relations = [
project.invited_groups.where_full_path_in(extractor.names, use_includes: false)
];
# Include the projects ancestor group(s) if they are listed as owners
if project.group
relations << project.group.self_and_ancestors.where_full_path_in(extractor.names, use_includes: false);
end
Group.from_union(relations).with_route.with_users;
end
end
Benchmark.bm do |bm|
project = Project.last
extractor = OpenStruct.new(names: ['gitlab-org', 'toolbox', 'Commit451'])
bm.report('load_groups before: ') { N.times { Before.load_groups(project, extractor) } }
bm.report('load_groups after: ') { N.times { After.load_groups(project, extractor) } }
end user system total real
load_groups before: 8.112644 0.148106 8.260750 ( 10.382684)
load_groups after: 0.666265 0.004052 0.670317 ( 0.671757)Query Diff
Before
project.invited_groups.where_full_path_in(extractor.names).with_route.with_users
SELECT
"namespaces"."id" AS t0_r0,
"namespaces"."name" AS t0_r1,
"namespaces"."path" AS t0_r2,
"namespaces"."owner_id" AS t0_r3,
"namespaces"."created_at" AS t0_r4,
"namespaces"."updated_at" AS t0_r5,
"namespaces"."type" AS t0_r6,
"namespaces"."description" AS t0_r7,
"namespaces"."avatar" AS t0_r8,
"namespaces"."membership_lock" AS t0_r9,
"namespaces"."share_with_group_lock" AS t0_r10,
"namespaces"."visibility_level" AS t0_r11,
"namespaces"."request_access_enabled" AS t0_r12,
"namespaces"."ldap_sync_status" AS t0_r13,
"namespaces"."ldap_sync_error" AS t0_r14,
"namespaces"."ldap_sync_last_update_at" AS t0_r15,
"namespaces"."ldap_sync_last_successful_update_at" AS t0_r16,
"namespaces"."ldap_sync_last_sync_at" AS t0_r17,
"namespaces"."description_html" AS t0_r18,
"namespaces"."lfs_enabled" AS t0_r19,
"namespaces"."parent_id" AS t0_r20,
"namespaces"."shared_runners_minutes_limit" AS t0_r21,
"namespaces"."repository_size_limit" AS t0_r22,
"namespaces"."require_two_factor_authentication" AS t0_r23,
"namespaces"."two_factor_grace_period" AS t0_r24,
"namespaces"."cached_markdown_version" AS t0_r25,
"namespaces"."project_creation_level" AS t0_r26,
"namespaces"."runners_token" AS t0_r27,
"namespaces"."file_template_project_id" AS t0_r28,
"namespaces"."saml_discovery_token" AS t0_r29,
"namespaces"."runners_token_encrypted" AS t0_r30,
"namespaces"."custom_project_templates_group_id" AS t0_r31,
"namespaces"."auto_devops_enabled" AS t0_r32,
"namespaces"."extra_shared_runners_minutes_limit" AS t0_r33,
"namespaces"."last_ci_minutes_notification_at" AS t0_r34,
"namespaces"."last_ci_minutes_usage_notification_level" AS t0_r35,
"namespaces"."subgroup_creation_level" AS t0_r36,
"namespaces"."emails_disabled" AS t0_r37,
"namespaces"."max_pages_size" AS t0_r38,
"namespaces"."max_artifacts_size" AS t0_r39,
"namespaces"."mentions_disabled" AS t0_r40,
"namespaces"."default_branch_protection" AS t0_r41,
"namespaces"."unlock_membership_to_ldap" AS t0_r42,
"namespaces"."max_personal_access_token_lifetime" AS t0_r43,
"namespaces"."push_rule_id" AS t0_r44,
"namespaces"."shared_runners_enabled" AS t0_r45,
"namespaces"."allow_descendants_override_disabled_shared_runners" AS t0_r46,
"namespaces"."traversal_ids" AS t0_r47,
"routes"."id" AS t1_r0,
"routes"."source_id" AS t1_r1,
"routes"."source_type" AS t1_r2,
"routes"."path" AS t1_r3,
"routes"."created_at" AS t1_r4,
"routes"."updated_at" AS t1_r5,
"routes"."name" AS t1_r6,
"routes"."namespace_id" AS t1_r7,
"users"."id" AS t2_r0,
"users"."email" AS t2_r1,
"users"."encrypted_password" AS t2_r2,
"users"."reset_password_token" AS t2_r3,
"users"."reset_password_sent_at" AS t2_r4,
"users"."remember_created_at" AS t2_r5,
"users"."sign_in_count" AS t2_r6,
"users"."current_sign_in_at" AS t2_r7,
"users"."last_sign_in_at" AS t2_r8,
"users"."current_sign_in_ip" AS t2_r9,
"users"."last_sign_in_ip" AS t2_r10,
"users"."created_at" AS t2_r11,
"users"."updated_at" AS t2_r12,
"users"."name" AS t2_r13,
"users"."admin" AS t2_r14,
"users"."projects_limit" AS t2_r15,
"users"."failed_attempts" AS t2_r16,
"users"."locked_at" AS t2_r17,
"users"."username" AS t2_r18,
"users"."can_create_group" AS t2_r19,
"users"."can_create_team" AS t2_r20,
"users"."state" AS t2_r21,
"users"."color_scheme_id" AS t2_r22,
"users"."password_expires_at" AS t2_r23,
"users"."created_by_id" AS t2_r24,
"users"."last_credential_check_at" AS t2_r25,
"users"."avatar" AS t2_r26,
"users"."confirmation_token" AS t2_r27,
"users"."confirmed_at" AS t2_r28,
"users"."confirmation_sent_at" AS t2_r29,
"users"."unconfirmed_email" AS t2_r30,
"users"."hide_no_ssh_key" AS t2_r31,
"users"."admin_email_unsubscribed_at" AS t2_r32,
"users"."notification_email" AS t2_r33,
"users"."hide_no_password" AS t2_r34,
"users"."password_automatically_set" AS t2_r35,
"users"."encrypted_otp_secret" AS t2_r36,
"users"."encrypted_otp_secret_iv" AS t2_r37,
"users"."encrypted_otp_secret_salt" AS t2_r38,
"users"."otp_required_for_login" AS t2_r39,
"users"."otp_backup_codes" AS t2_r40,
"users"."public_email" AS t2_r41,
"users"."dashboard" AS t2_r42,
"users"."project_view" AS t2_r43,
"users"."consumed_timestep" AS t2_r44,
"users"."layout" AS t2_r45,
"users"."hide_project_limit" AS t2_r46,
"users"."note" AS t2_r47,
"users"."unlock_token" AS t2_r48,
"users"."otp_grace_period_started_at" AS t2_r49,
"users"."external" AS t2_r50,
"users"."incoming_email_token" AS t2_r51,
"users"."auditor" AS t2_r52,
"users"."require_two_factor_authentication_from_group" AS t2_r53,
"users"."two_factor_grace_period" AS t2_r54,
"users"."last_activity_on" AS t2_r55,
"users"."notified_of_own_activity" AS t2_r56,
"users"."preferred_language" AS t2_r57,
"users"."email_opted_in" AS t2_r58,
"users"."email_opted_in_ip" AS t2_r59,
"users"."email_opted_in_source_id" AS t2_r60,
"users"."email_opted_in_at" AS t2_r61,
"users"."theme_id" AS t2_r62,
"users"."accepted_term_id" AS t2_r63,
"users"."feed_token" AS t2_r64,
"users"."private_profile" AS t2_r65,
"users"."roadmap_layout" AS t2_r66,
"users"."include_private_contributions" AS t2_r67,
"users"."commit_email" AS t2_r68,
"users"."group_view" AS t2_r69,
"users"."managing_group_id" AS t2_r70,
"users"."first_name" AS t2_r71,
"users"."last_name" AS t2_r72,
"users"."static_object_token" AS t2_r73,
"users"."role" AS t2_r74,
"users"."user_type" AS t2_r75,
"users"."static_object_token_encrypted" AS t2_r76,
"users"."otp_secret_expires_at" AS t2_r77,
"users"."onboarding_in_progress" AS t2_r78
FROM
"namespaces"
INNER JOIN "project_group_links" ON "namespaces"."id" = "project_group_links"."group_id"
LEFT OUTER JOIN "routes" ON "routes"."source_type" = 'Namespace'
AND "routes"."source_id" = "namespaces"."id"
LEFT OUTER JOIN "members" ON "members"."source_type" = 'Namespace'
AND "members"."requested_at" IS NULL
AND "members"."access_level" != 5
AND "members"."source_id" = "namespaces"."id"
AND "members"."type" = 'GroupMember'
LEFT OUTER JOIN "users" ON "users"."id" = "members"."user_id"
WHERE
"namespaces"."type" = 'Group'
AND "project_group_links"."project_id" = 21
AND (
(
LOWER(routes.path) = LOWER('gitlab-org')
)
OR (
LOWER(routes.path) = LOWER('toolbox')
)
OR (
LOWER(routes.path) = LOWER('Commit451')
)
)
project.group.self_and_ancestors.with_route.with_users (performed twice 🙁 )
Group Load (0.5ms) SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 29 LIMIT 1
Group Load (0.3ms) SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 29
Route Load (0.2ms) SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Namespace' AND "routes"."source_id" = 29
GroupMember Load (0.3ms) SELECT "members".* FROM "members" WHERE "members"."source_type" = 'Namespace' AND "members"."type" = 'GroupMember' AND "members"."requested_at" IS NULL AND "members"."access_level" != 5 AND "members"."source_id" = 29
User Load (0.6ms) SELECT "users".* FROM "users" WHERE "users"."id" IN (1, 10, 11, 4, 2)After
Group.from_union([project.invited_groups.where_full_path_in(extractor.names, use_includes: false), project.group.self_and_ancestors.where_full_path_in(extractor.names, use_includes: false)]).with_route.with_users
Group Load (1.5ms)
SELECT
"namespaces".*
FROM
(
(
SELECT
"namespaces".*
FROM
"namespaces"
INNER JOIN "project_group_links" ON "namespaces"."id" = "project_group_links"."group_id"
INNER JOIN "routes" ON "routes"."source_type" = 'Namespace'
AND "routes"."source_id" = "namespaces"."id"
WHERE
"namespaces"."type" = 'Group'
AND "project_group_links"."project_id" = 21
AND (
(
LOWER(routes.path) = LOWER('gitlab-org')
)
OR (
LOWER(routes.path) = LOWER('toolbox')
)
OR (
LOWER(routes.path) = LOWER('Commit451')
)
)
)
UNION
(
SELECT
"namespaces".*
FROM
"namespaces"
INNER JOIN "routes" ON "routes"."source_type" = 'Namespace'
AND "routes"."source_id" = "namespaces"."id"
WHERE
"namespaces"."type" = 'Group'
AND "namespaces"."id" = 29
AND (
(
LOWER(routes.path) = LOWER('gitlab-org')
)
OR (
LOWER(routes.path) = LOWER('toolbox')
)
OR (
LOWER(routes.path) = LOWER('Commit451')
)
)
)
) namespaces
WHERE
"namespaces"."type" = 'Group'
Route Load (0.3ms) SELECT "routes".* FROM "routes" WHERE "routes"."source_type" = 'Namespace' AND "routes"."source_id" IN (22, 24, 29)
GroupMember Load (0.3ms)
SELECT
"members".*
FROM
"members"
WHERE
"members"."source_type" = 'Namespace'
AND "members"."type" = 'GroupMember'
AND "members"."requested_at" IS NULL
AND "members"."access_level" != 5
AND "members"."source_id" IN (22, 24, 29)
User Load (0.3ms) SELECT "users".* FROM "users" WHERE "users"."id" IN (1, 10, 11, 4, 2, 17, 6, 9, 14, 12)MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Part of #219916 (closed)
Edited by Joe Woodward