From d8bd6f34f09bc9a6602bebb33bdc9e1f255a0d7c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 12 Mar 2024 15:23:44 +0800 Subject: [PATCH] Do some performance optimize for issues list and view issue/pull (#29515) This PR do some performance optimzations. - [x] Add `index` for the column `comment_id` of `Attachment` table to accelerate query from the database. - [x] Remove unnecessary database queries when viewing issues. Before some conditions which id = 0 will be sent to the database - [x] Remove duplicated load posters - [x] Batch loading attachements, isread of comments on viewing issue --------- Co-authored-by: Zettat123 --- models/issues/comment.go | 8 +-- models/issues/comment_code.go | 2 +- models/issues/comment_list.go | 78 +++++++++++++++++++++------- models/issues/issue_list.go | 25 +++++++-- models/migrations/migrations.go | 2 + models/migrations/v1_22/v291.go | 14 +++++ models/repo/attachment.go | 2 +- routers/api/v1/repo/issue_comment.go | 4 -- routers/web/repo/issue.go | 39 ++++++-------- routers/web/repo/pull_review.go | 8 ++- 10 files changed, 121 insertions(+), 61 deletions(-) create mode 100644 models/migrations/v1_22/v291.go diff --git a/models/issues/comment.go b/models/issues/comment.go index e37f844b5c..6f65a5dbbc 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -673,7 +673,8 @@ func (c *Comment) LoadTime(ctx context.Context) error { return err } -func (c *Comment) loadReactions(ctx context.Context, repo *repo_model.Repository) (err error) { +// LoadReactions loads comment reactions +func (c *Comment) LoadReactions(ctx context.Context, repo *repo_model.Repository) (err error) { if c.Reactions != nil { return nil } @@ -691,11 +692,6 @@ func (c *Comment) loadReactions(ctx context.Context, repo *repo_model.Repository return nil } -// LoadReactions loads comment reactions -func (c *Comment) LoadReactions(ctx context.Context, repo *repo_model.Repository) error { - return c.loadReactions(ctx, repo) -} - func (c *Comment) loadReview(ctx context.Context) (err error) { if c.ReviewID == 0 { return nil diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 384a595dd9..74a7a86f26 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -122,7 +122,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu } // FetchCodeCommentsByLine fetches the code comments for a given treePath and line number -func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) ([]*Comment, error) { +func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) (CommentList, error) { opts := FindCommentsOptions{ Type: CommentTypeCode, IssueID: issue.ID, diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 30a437ea50..0047b054ba 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -19,7 +19,9 @@ type CommentList []*Comment func (comments CommentList) getPosterIDs() []int64 { posterIDs := make(container.Set[int64], len(comments)) for _, comment := range comments { - posterIDs.Add(comment.PosterID) + if comment.PosterID > 0 { + posterIDs.Add(comment.PosterID) + } } return posterIDs.Values() } @@ -41,18 +43,12 @@ func (comments CommentList) LoadPosters(ctx context.Context) error { return nil } -func (comments CommentList) getCommentIDs() []int64 { - ids := make([]int64, 0, len(comments)) - for _, comment := range comments { - ids = append(ids, comment.ID) - } - return ids -} - func (comments CommentList) getLabelIDs() []int64 { ids := make(container.Set[int64], len(comments)) for _, comment := range comments { - ids.Add(comment.LabelID) + if comment.LabelID > 0 { + ids.Add(comment.LabelID) + } } return ids.Values() } @@ -100,7 +96,9 @@ func (comments CommentList) loadLabels(ctx context.Context) error { func (comments CommentList) getMilestoneIDs() []int64 { ids := make(container.Set[int64], len(comments)) for _, comment := range comments { - ids.Add(comment.MilestoneID) + if comment.MilestoneID > 0 { + ids.Add(comment.MilestoneID) + } } return ids.Values() } @@ -141,7 +139,9 @@ func (comments CommentList) loadMilestones(ctx context.Context) error { func (comments CommentList) getOldMilestoneIDs() []int64 { ids := make(container.Set[int64], len(comments)) for _, comment := range comments { - ids.Add(comment.OldMilestoneID) + if comment.OldMilestoneID > 0 { + ids.Add(comment.OldMilestoneID) + } } return ids.Values() } @@ -182,7 +182,9 @@ func (comments CommentList) loadOldMilestones(ctx context.Context) error { func (comments CommentList) getAssigneeIDs() []int64 { ids := make(container.Set[int64], len(comments)) for _, comment := range comments { - ids.Add(comment.AssigneeID) + if comment.AssigneeID > 0 { + ids.Add(comment.AssigneeID) + } } return ids.Values() } @@ -314,7 +316,9 @@ func (comments CommentList) getDependentIssueIDs() []int64 { if comment.DependentIssue != nil { continue } - ids.Add(comment.DependentIssueID) + if comment.DependentIssueID > 0 { + ids.Add(comment.DependentIssueID) + } } return ids.Values() } @@ -369,6 +373,41 @@ func (comments CommentList) loadDependentIssues(ctx context.Context) error { return nil } +// getAttachmentCommentIDs only return the comment ids which possibly has attachments +func (comments CommentList) getAttachmentCommentIDs() []int64 { + ids := make(container.Set[int64], len(comments)) + for _, comment := range comments { + if comment.Type == CommentTypeComment || + comment.Type == CommentTypeReview || + comment.Type == CommentTypeCode { + ids.Add(comment.ID) + } + } + return ids.Values() +} + +// LoadAttachmentsByIssue loads attachments by issue id +func (comments CommentList) LoadAttachmentsByIssue(ctx context.Context) error { + if len(comments) == 0 { + return nil + } + + attachments := make([]*repo_model.Attachment, 0, len(comments)/2) + if err := db.GetEngine(ctx).Where("issue_id=? AND comment_id>0", comments[0].IssueID).Find(&attachments); err != nil { + return err + } + + commentAttachmentsMap := make(map[int64][]*repo_model.Attachment, len(comments)) + for _, attach := range attachments { + commentAttachmentsMap[attach.CommentID] = append(commentAttachmentsMap[attach.CommentID], attach) + } + + for _, comment := range comments { + comment.Attachments = commentAttachmentsMap[comment.ID] + } + return nil +} + // LoadAttachments loads attachments func (comments CommentList) LoadAttachments(ctx context.Context) (err error) { if len(comments) == 0 { @@ -376,16 +415,15 @@ func (comments CommentList) LoadAttachments(ctx context.Context) (err error) { } attachments := make(map[int64][]*repo_model.Attachment, len(comments)) - commentsIDs := comments.getCommentIDs() + commentsIDs := comments.getAttachmentCommentIDs() left := len(commentsIDs) for left > 0 { limit := db.DefaultMaxInSize if left < limit { limit = left } - rows, err := db.GetEngine(ctx).Table("attachment"). - Join("INNER", "comment", "comment.id = attachment.comment_id"). - In("comment.id", commentsIDs[:limit]). + rows, err := db.GetEngine(ctx). + In("comment_id", commentsIDs[:limit]). Rows(new(repo_model.Attachment)) if err != nil { return err @@ -415,7 +453,9 @@ func (comments CommentList) LoadAttachments(ctx context.Context) (err error) { func (comments CommentList) getReviewIDs() []int64 { ids := make(container.Set[int64], len(comments)) for _, comment := range comments { - ids.Add(comment.ReviewID) + if comment.ReviewID > 0 { + ids.Add(comment.ReviewID) + } } return ids.Values() } diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index 0fb8447ff7..41a90d133d 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -388,9 +388,8 @@ func (issues IssueList) LoadAttachments(ctx context.Context) (err error) { if left < limit { limit = left } - rows, err := db.GetEngine(ctx).Table("attachment"). - Join("INNER", "issue", "issue.id = attachment.issue_id"). - In("issue.id", issuesIDs[:limit]). + rows, err := db.GetEngine(ctx). + In("issue_id", issuesIDs[:limit]). Rows(new(repo_model.Attachment)) if err != nil { return err @@ -609,3 +608,23 @@ func (issues IssueList) GetApprovalCounts(ctx context.Context) (map[int64][]*Rev return approvalCountMap, nil } + +func (issues IssueList) LoadIsRead(ctx context.Context, userID int64) error { + issueIDs := issues.getIssueIDs() + issueUsers := make([]*IssueUser, 0, len(issueIDs)) + if err := db.GetEngine(ctx).Where("uid =?", userID). + In("issue_id"). + Find(&issueUsers); err != nil { + return err + } + + for _, issueUser := range issueUsers { + for _, issue := range issues { + if issue.ID == issueUser.IssueID { + issue.IsRead = issueUser.IsRead + } + } + } + + return nil +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index ce77432db4..87fddefb88 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -566,6 +566,8 @@ var migrations = []Migration{ NewMigration("Add default_wiki_branch to repository table", v1_22.AddDefaultWikiBranch), // v290 -> v291 NewMigration("Add PayloadVersion to HookTask", v1_22.AddPayloadVersionToHookTaskTable), + // v291 -> v292 + NewMigration("Add Index to attachment.comment_id", v1_22.AddCommentIDIndexofAttachment), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_22/v291.go b/models/migrations/v1_22/v291.go new file mode 100644 index 0000000000..0bfffe5d05 --- /dev/null +++ b/models/migrations/v1_22/v291.go @@ -0,0 +1,14 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_22 //nolint + +import "xorm.io/xorm" + +func AddCommentIDIndexofAttachment(x *xorm.Engine) error { + type Attachment struct { + CommentID int64 `xorm:"INDEX"` + } + + return x.Sync(&Attachment{}) +} diff --git a/models/repo/attachment.go b/models/repo/attachment.go index 1a588398c1..9b0de11fdc 100644 --- a/models/repo/attachment.go +++ b/models/repo/attachment.go @@ -24,7 +24,7 @@ type Attachment struct { IssueID int64 `xorm:"INDEX"` // maybe zero when creating ReleaseID int64 `xorm:"INDEX"` // maybe zero when creating UploaderID int64 `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added - CommentID int64 + CommentID int64 `xorm:"INDEX"` Name string DownloadCount int64 `xorm:"DEFAULT 0"` Size int64 `xorm:"DEFAULT 0"` diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 21aabadf3d..070571ba62 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -323,10 +323,6 @@ func ListRepoIssueComments(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "LoadIssues", err) return } - if err := comments.LoadPosters(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadPosters", err) - return - } if err := comments.LoadAttachments(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadAttachments", err) return diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 83a5b76bf1..8935cc80e2 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -324,15 +324,15 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt return } - // Get posters. - for i := range issues { - // Check read status - if !ctx.IsSigned { - issues[i].IsRead = true - } else if err = issues[i].GetIsRead(ctx, ctx.Doer.ID); err != nil { - ctx.ServerError("GetIsRead", err) + if ctx.IsSigned { + if err := issues.LoadIsRead(ctx, ctx.Doer.ID); err != nil { + ctx.ServerError("LoadIsRead", err) return } + } else { + for i := range issues { + issues[i].IsRead = true + } } commitStatuses, lastStatus, err := pull_service.GetIssuesAllCommitStatus(ctx, issues) @@ -1604,20 +1604,20 @@ func ViewIssue(ctx *context.Context) { // Render comments and and fetch participants. participants[0] = issue.Poster + + if err := issue.Comments.LoadAttachmentsByIssue(ctx); err != nil { + ctx.ServerError("LoadAttachmentsByIssue", err) + return + } + if err := issue.Comments.LoadPosters(ctx); err != nil { + ctx.ServerError("LoadPosters", err) + return + } + for _, comment = range issue.Comments { comment.Issue = issue - if err := comment.LoadPoster(ctx); err != nil { - ctx.ServerError("LoadPoster", err) - return - } - if comment.Type == issues_model.CommentTypeComment || comment.Type == issues_model.CommentTypeReview { - if err := comment.LoadAttachments(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) - return - } - comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{ Links: markup.Links{ Base: ctx.Repo.RepoLink, @@ -1665,7 +1665,6 @@ func ViewIssue(ctx *context.Context) { comment.Milestone = ghostMilestone } } else if comment.Type == issues_model.CommentTypeProject { - if err = comment.LoadProject(ctx); err != nil { ctx.ServerError("LoadProject", err) return @@ -1731,10 +1730,6 @@ func ViewIssue(ctx *context.Context) { for _, codeComments := range comment.Review.CodeComments { for _, lineComments := range codeComments { for _, c := range lineComments { - if err := c.LoadAttachments(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) - return - } // Check tag. role, ok = marked[c.PosterID] if ok { diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index bce807aacd..5385ebfc97 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -179,11 +179,9 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori return } - for _, c := range comments { - if err := c.LoadAttachments(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) - return - } + if err := comments.LoadAttachments(ctx); err != nil { + ctx.ServerError("LoadAttachments", err) + return } ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled