From d996c5d5179c99855e69156a034eca055e9329a4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 12 Mar 2024 12:57:19 +0800 Subject: [PATCH] Some performance optimization on dashboard and issues page (#29010) This PR do some loading speed optimization for feeds user interface pages. - Load action users batchly but not one by one. - Load action repositories batchly but not one by one. - Load action's Repo Owners batchly but not one by one. - Load action's possible issues batchly but not one by one. - Load action's possible comments batchly but not one by one. (cherry picked from commit aed3b53abdd02a3ffbf9e8eb90272ff567333073) --- models/activities/action.go | 116 +++++++++++++------------- models/activities/action_list.go | 136 ++++++++++++++++++++++++++----- models/issues/issue_list.go | 10 +++ models/repo/repo.go | 3 + models/repo/repo_list.go | 35 ++++++++ modules/util/slice.go | 9 ++ routers/web/repo/repo.go | 6 +- 7 files changed, 234 insertions(+), 81 deletions(-) diff --git a/models/activities/action.go b/models/activities/action.go index 659376767..f85a493e2 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -150,6 +150,7 @@ type Action struct { Repo *repo_model.Repository `xorm:"-"` CommentID int64 `xorm:"INDEX"` Comment *issues_model.Comment `xorm:"-"` + Issue *issues_model.Issue `xorm:"-"` // get the issue id from content IsDeleted bool `xorm:"NOT NULL DEFAULT false"` RefName string IsPrivate bool `xorm:"NOT NULL DEFAULT false"` @@ -292,11 +293,6 @@ func (a *Action) GetRepoAbsoluteLink(ctx context.Context) string { return setting.AppURL + url.PathEscape(a.GetRepoUserName(ctx)) + "/" + url.PathEscape(a.GetRepoName(ctx)) } -// GetCommentHTMLURL returns link to action comment. -func (a *Action) GetCommentHTMLURL(ctx context.Context) string { - return a.getCommentHTMLURL(ctx) -} - func (a *Action) loadComment(ctx context.Context) (err error) { if a.CommentID == 0 || a.Comment != nil { return nil @@ -305,7 +301,8 @@ func (a *Action) loadComment(ctx context.Context) (err error) { return err } -func (a *Action) getCommentHTMLURL(ctx context.Context) string { +// GetCommentHTMLURL returns link to action comment. +func (a *Action) GetCommentHTMLURL(ctx context.Context) string { if a == nil { return "#" } @@ -313,34 +310,19 @@ func (a *Action) getCommentHTMLURL(ctx context.Context) string { if a.Comment != nil { return a.Comment.HTMLURL(ctx) } - if len(a.GetIssueInfos()) == 0 { + + if err := a.LoadIssue(ctx); err != nil || a.Issue == nil { return "#" } - // Return link to issue - issueIDString := a.GetIssueInfos()[0] - issueID, err := strconv.ParseInt(issueIDString, 10, 64) - if err != nil { + if err := a.Issue.LoadRepo(ctx); err != nil { return "#" } - issue, err := issues_model.GetIssueByID(ctx, issueID) - if err != nil { - return "#" - } - - if err = issue.LoadRepo(ctx); err != nil { - return "#" - } - - return issue.HTMLURL() + return a.Issue.HTMLURL() } // GetCommentLink returns link to action comment. func (a *Action) GetCommentLink(ctx context.Context) string { - return a.getCommentLink(ctx) -} - -func (a *Action) getCommentLink(ctx context.Context) string { if a == nil { return "#" } @@ -348,26 +330,15 @@ func (a *Action) getCommentLink(ctx context.Context) string { if a.Comment != nil { return a.Comment.Link(ctx) } - if len(a.GetIssueInfos()) == 0 { + + if err := a.LoadIssue(ctx); err != nil || a.Issue == nil { return "#" } - // Return link to issue - issueIDString := a.GetIssueInfos()[0] - issueID, err := strconv.ParseInt(issueIDString, 10, 64) - if err != nil { + if err := a.Issue.LoadRepo(ctx); err != nil { return "#" } - issue, err := issues_model.GetIssueByID(ctx, issueID) - if err != nil { - return "#" - } - - if err = issue.LoadRepo(ctx); err != nil { - return "#" - } - - return issue.Link() + return a.Issue.Link() } // GetBranch returns the action's repository branch. @@ -395,6 +366,10 @@ func (a *Action) GetCreate() time.Time { return a.CreatedUnix.AsTime() } +func (a *Action) IsIssueEvent() bool { + return a.OpType.InActions("comment_issue", "approve_pull_request", "reject_pull_request", "comment_pull", "merge_pull_request") +} + // GetIssueInfos returns a list of associated information with the action. func (a *Action) GetIssueInfos() []string { // make sure it always returns 3 elements, because there are some access to the a[1] and a[2] without checking the length @@ -405,27 +380,52 @@ func (a *Action) GetIssueInfos() []string { return ret } -// GetIssueTitle returns the title of first issue associated with the action. -func (a *Action) GetIssueTitle(ctx context.Context) string { - index, _ := strconv.ParseInt(a.GetIssueInfos()[0], 10, 64) - issue, err := issues_model.GetIssueByIndex(ctx, a.RepoID, index) - if err != nil { - log.Error("GetIssueByIndex: %v", err) - return "500 when get issue" +func (a *Action) getIssueIndex() int64 { + infos := a.GetIssueInfos() + if len(infos) == 0 { + return 0 } - return issue.Title + index, _ := strconv.ParseInt(infos[0], 10, 64) + return index } -// GetIssueContent returns the content of first issue associated with -// this action. -func (a *Action) GetIssueContent(ctx context.Context) string { - index, _ := strconv.ParseInt(a.GetIssueInfos()[0], 10, 64) - issue, err := issues_model.GetIssueByIndex(ctx, a.RepoID, index) - if err != nil { - log.Error("GetIssueByIndex: %v", err) - return "500 when get issue" +func (a *Action) LoadIssue(ctx context.Context) error { + if a.Issue != nil { + return nil } - return issue.Content + if index := a.getIssueIndex(); index > 0 { + issue, err := issues_model.GetIssueByIndex(ctx, a.RepoID, index) + if err != nil { + return err + } + a.Issue = issue + a.Issue.Repo = a.Repo + } + return nil +} + +// GetIssueTitle returns the title of first issue associated with the action. +func (a *Action) GetIssueTitle(ctx context.Context) string { + if err := a.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return "<500 when get issue>" + } + if a.Issue == nil { + return "" + } + return a.Issue.Title +} + +// GetIssueContent returns the content of first issue associated with this action. +func (a *Action) GetIssueContent(ctx context.Context) string { + if err := a.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return "<500 when get issue>" + } + if a.Issue == nil { + return "" + } + return a.Issue.Content } // GetFeedsOptions options for retrieving feeds @@ -465,7 +465,7 @@ func GetFeeds(ctx context.Context, opts GetFeedsOptions) (ActionList, int64, err return nil, 0, fmt.Errorf("FindAndCount: %w", err) } - if err := ActionList(actions).loadAttributes(ctx); err != nil { + if err := ActionList(actions).LoadAttributes(ctx); err != nil { return nil, 0, fmt.Errorf("LoadAttributes: %w", err) } diff --git a/models/activities/action_list.go b/models/activities/action_list.go index 3d74397c6..fdf0f35d4 100644 --- a/models/activities/action_list.go +++ b/models/activities/action_list.go @@ -6,11 +6,16 @@ package activities import ( "context" "fmt" + "strconv" "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" + "code.gitea.io/gitea/modules/util" + + "xorm.io/builder" ) // ActionList defines a list of actions @@ -24,7 +29,7 @@ func (actions ActionList) getUserIDs() []int64 { return userIDs.Values() } -func (actions ActionList) loadUsers(ctx context.Context) (map[int64]*user_model.User, error) { +func (actions ActionList) LoadActUsers(ctx context.Context) (map[int64]*user_model.User, error) { if len(actions) == 0 { return nil, nil } @@ -52,7 +57,7 @@ func (actions ActionList) getRepoIDs() []int64 { return repoIDs.Values() } -func (actions ActionList) loadRepositories(ctx context.Context) error { +func (actions ActionList) LoadRepositories(ctx context.Context) error { if len(actions) == 0 { return nil } @@ -63,11 +68,11 @@ func (actions ActionList) loadRepositories(ctx context.Context) error { if err != nil { return fmt.Errorf("find repository: %w", err) } - for _, action := range actions { action.Repo = repoMaps[action.RepoID] } - return nil + repos := repo_model.RepositoryList(util.ValuesOfMap(repoMaps)) + return repos.LoadUnits(ctx) } func (actions ActionList) loadRepoOwner(ctx context.Context, userMap map[int64]*user_model.User) (err error) { @@ -75,37 +80,124 @@ func (actions ActionList) loadRepoOwner(ctx context.Context, userMap map[int64]* userMap = make(map[int64]*user_model.User) } + userSet := make(container.Set[int64], len(actions)) for _, action := range actions { if action.Repo == nil { continue } - repoOwner, ok := userMap[action.Repo.OwnerID] - if !ok { - repoOwner, err = user_model.GetUserByID(ctx, action.Repo.OwnerID) - if err != nil { - if user_model.IsErrUserNotExist(err) { - continue - } - return err - } - userMap[repoOwner.ID] = repoOwner + if _, ok := userMap[action.Repo.OwnerID]; !ok { + userSet.Add(action.Repo.OwnerID) + } + } + + if err := db.GetEngine(ctx). + In("id", userSet.Values()). + Find(&userMap); err != nil { + return fmt.Errorf("find user: %w", err) + } + + for _, action := range actions { + if action.Repo != nil { + action.Repo.Owner = userMap[action.Repo.OwnerID] } - action.Repo.Owner = repoOwner } return nil } -// loadAttributes loads all attributes -func (actions ActionList) loadAttributes(ctx context.Context) error { - userMap, err := actions.loadUsers(ctx) +// LoadAttributes loads all attributes +func (actions ActionList) LoadAttributes(ctx context.Context) error { + // the load sequence cannot be changed because of the dependencies + userMap, err := actions.LoadActUsers(ctx) if err != nil { return err } - - if err := actions.loadRepositories(ctx); err != nil { + if err := actions.LoadRepositories(ctx); err != nil { return err } - - return actions.loadRepoOwner(ctx, userMap) + if err := actions.loadRepoOwner(ctx, userMap); err != nil { + return err + } + if err := actions.LoadIssues(ctx); err != nil { + return err + } + return actions.LoadComments(ctx) +} + +func (actions ActionList) LoadComments(ctx context.Context) error { + if len(actions) == 0 { + return nil + } + + commentIDs := make([]int64, 0, len(actions)) + for _, action := range actions { + if action.CommentID > 0 { + commentIDs = append(commentIDs, action.CommentID) + } + } + + commentsMap := make(map[int64]*issues_model.Comment, len(commentIDs)) + if err := db.GetEngine(ctx).In("id", commentIDs).Find(&commentsMap); err != nil { + return fmt.Errorf("find comment: %w", err) + } + + for _, action := range actions { + if action.CommentID > 0 { + action.Comment = commentsMap[action.CommentID] + if action.Comment != nil { + action.Comment.Issue = action.Issue + } + } + } + return nil +} + +func (actions ActionList) LoadIssues(ctx context.Context) error { + if len(actions) == 0 { + return nil + } + + conditions := builder.NewCond() + issueNum := 0 + for _, action := range actions { + if action.IsIssueEvent() { + infos := action.GetIssueInfos() + if len(infos) == 0 { + continue + } + index, _ := strconv.ParseInt(infos[0], 10, 64) + if index > 0 { + conditions = conditions.Or(builder.Eq{ + "repo_id": action.RepoID, + "`index`": index, + }) + issueNum++ + } + } + } + if !conditions.IsValid() { + return nil + } + + issuesMap := make(map[string]*issues_model.Issue, issueNum) + issues := make([]*issues_model.Issue, 0, issueNum) + if err := db.GetEngine(ctx).Where(conditions).Find(&issues); err != nil { + return fmt.Errorf("find issue: %w", err) + } + for _, issue := range issues { + issuesMap[fmt.Sprintf("%d-%d", issue.RepoID, issue.Index)] = issue + } + + for _, action := range actions { + if !action.IsIssueEvent() { + continue + } + if index := action.getIssueIndex(); index > 0 { + if issue, ok := issuesMap[fmt.Sprintf("%d-%d", action.RepoID, index)]; ok { + action.Issue = issue + action.Issue.Repo = action.Repo + } + } + } + return nil } diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index a932ac255..0fb8447ff 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -476,6 +476,16 @@ func (issues IssueList) loadTotalTrackedTimes(ctx context.Context) (err error) { } trackedTimes := make(map[int64]int64, len(issues)) + reposMap := make(map[int64]*repo_model.Repository, len(issues)) + for _, issue := range issues { + reposMap[issue.RepoID] = issue.Repo + } + repos := repo_model.RepositoryListOfMap(reposMap) + + if err := repos.LoadUnits(ctx); err != nil { + return err + } + ids := make([]int64, 0, len(issues)) for _, issue := range issues { if issue.Repo.IsTimetrackerEnabled(ctx) { diff --git a/models/repo/repo.go b/models/repo/repo.go index 81ed6efa2..4e1b6fcbc 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -553,6 +553,9 @@ func (repo *Repository) GetBaseRepo(ctx context.Context) (err error) { return nil } + if repo.BaseRepo != nil { + return nil + } repo.BaseRepo, err = GetRepositoryByID(ctx, repo.ForkID) return err } diff --git a/models/repo/repo_list.go b/models/repo/repo_list.go index 6b452291e..cb7cd47a8 100644 --- a/models/repo/repo_list.go +++ b/models/repo/repo_list.go @@ -63,6 +63,41 @@ func RepositoryListOfMap(repoMap map[int64]*Repository) RepositoryList { return RepositoryList(ValuesRepository(repoMap)) } +func (repos RepositoryList) LoadUnits(ctx context.Context) error { + if len(repos) == 0 { + return nil + } + + // Load units. + units := make([]*RepoUnit, 0, len(repos)*6) + if err := db.GetEngine(ctx). + In("repo_id", repos.IDs()). + Find(&units); err != nil { + return fmt.Errorf("find units: %w", err) + } + + unitsMap := make(map[int64][]*RepoUnit, len(repos)) + for _, unit := range units { + if !unit.Type.UnitGlobalDisabled() { + unitsMap[unit.RepoID] = append(unitsMap[unit.RepoID], unit) + } + } + + for _, repo := range repos { + repo.Units = unitsMap[repo.ID] + } + + return nil +} + +func (repos RepositoryList) IDs() []int64 { + repoIDs := make([]int64, len(repos)) + for i := range repos { + repoIDs[i] = repos[i].ID + } + return repoIDs +} + // LoadAttributes loads the attributes for the given RepositoryList func (repos RepositoryList) LoadAttributes(ctx context.Context) error { if len(repos) == 0 { diff --git a/modules/util/slice.go b/modules/util/slice.go index a7073fede..f00e84bf0 100644 --- a/modules/util/slice.go +++ b/modules/util/slice.go @@ -53,3 +53,12 @@ func Sorted[S ~[]E, E cmp.Ordered](values S) S { slices.Sort(values) return values } + +// TODO: Replace with "maps.Values" once available +func ValuesOfMap[K comparable, V any](m map[K]V) []V { + values := make([]V, 0, len(m)) + for _, v := range m { + values = append(values, v) + } + return values +} diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 57723e6a9..07e6c937b 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -543,9 +543,13 @@ func InitiateDownload(ctx *context.Context) { // SearchRepo repositories via options func SearchRepo(ctx *context.Context) { + page := ctx.FormInt("page") + if page <= 0 { + page = 1 + } opts := &repo_model.SearchRepoOptions{ ListOptions: db.ListOptions{ - Page: ctx.FormInt("page"), + Page: page, PageSize: convert.ToCorrectPageSize(ctx.FormInt("limit")), }, Actor: ctx.Doer,