In EpicsFinder when we search for epics in many groups (typically when a group has many subgroups), then we have to check if user can read epics in each of this group. Because this is now done separately for each group (there is N+1 issue), we could optimize this by checking permissions only for subgroups which contain some epic (the idea is that not all subgroups will contain epics).
Before running user_can_read_groups, on all related groups, we could call something like groups.where(id: Epic.select(:group_id)). We don't have to check every sub group permission if the group doesn't contain any epic.
This is quite simple change with big impact. This improvement should be significant for any bigger groups. For example for gitlab-org group in situations when a guest or anonymous user loads epics, we would check permissions for only 21 subgroups instead of 246 (this difference may be even higher as it doesn't include all sub-sub-groups
@cdybenko this improvement should be significant for any bigger groups. For example for gitlab-org group in situations when a guest or anonymous user loads epics, we would check permissions for only 21 subgroups instead of 246 (this difference may be even higher as it doesn't include all sub-sub-groups).
This requires relatively simple query change, on the other side it will require DB query changes so something between 2-3 -> so perhaps 3 is safer, @jarka WDYT?
This is now enabled by default on .com (I don't expect so big difference across all groups as in case of gitlab-org - the reason is that most of groups are not so big as gitlab-org so not so many sub-group checks is done for them anyway).
Great result! The latest Sitespeed metrics are at a 30 day low across the board for the gitlab-org epics list:
I expect the Fully loaded result is an anomaly as we're seeing it on many pages on this reading. Of course this result in general could be an anomaly, we'll need more data.
What's interesting additionally is that this result is much closer to the result for gitlab-org/support:
30-day minimum TTFB is 500-600ms in both (Grafana).
@johnhope I think the spike should by an unrelated anomaly.
gitlab-com/support doesn't have too many subgroups so there is not much difference in this case. For gitlab-org, the optimization is better visible there:
This is now enabled by default on .com (I don't expect so big difference across all groups as in case of gitlab-org - the reason is that most of groups are not so big as gitlab-org so not so many sub-group checks is done for them anyway).
As expected the difference across all groups is not so huge (compared to gitlab-org group only - #327454 (comment 569019264)), but still quite solid:
gitlab-com/support doesn't have too many subgroups so there is not much difference in this case. For gitlab-org, the optimization is better visible there
@jprovaznik They're good for comparison though! Previously, gitlab-org LCP was much higher than gitlab-com/support. The fact that they're almost equal now indicates that some complexity related to group hierarchy and/or table-size has been removed. That's a great result!
It's borne out in our Dev LCP leaderboard, where the gitlab-org epics endpoint was in the amber and far down the leaderboard. It's now in the top 20 and firmly in the green.
@johnhope this optimization was rolled out, from the graph TTFB (and related) is now constantly better, "fully loaded" doesn't show an improvement but I think it's not relevant in this case (this optimized SQL which is part of the "main"/first page request). If you agree, can we close this one?