From cb1f776e8d1e8e708d77a6c8222fd047acadf323 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Mar 2024 14:45:22 +0800 Subject: [PATCH 1/6] Do some performance optimize for issues list and view issue/pull --- models/issues/comment.go | 8 ++---- models/issues/comment_list.go | 19 +++++++++++--- models/issues/issue_list.go | 25 +++++++++++++++--- models/migrations/migrations.go | 2 ++ models/migrations/v1_22/v287.go | 14 ++++++++++ models/repo/attachment.go | 2 +- routers/api/v1/repo/issue_comment.go | 4 --- routers/web/repo/issue.go | 39 ++++++++++++---------------- 8 files changed, 73 insertions(+), 40 deletions(-) create mode 100644 models/migrations/v1_22/v287.go diff --git a/models/issues/comment.go b/models/issues/comment.go index c7b22f3cca073..8025b51de7cdd 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -671,7 +671,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 } @@ -689,11 +690,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_list.go b/models/issues/comment_list.go index 30a437ea50dcf..cf991a03a0ee9 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -369,6 +369,18 @@ func (comments CommentList) loadDependentIssues(ctx context.Context) error { return nil } +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() +} + // LoadAttachments loads attachments func (comments CommentList) LoadAttachments(ctx context.Context) (err error) { if len(comments) == 0 { @@ -376,16 +388,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 diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index a932ac2554369..f17749ff98ba2 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 @@ -599,3 +598,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 beb1f3bb96381..d4621e6f08102 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -558,6 +558,8 @@ var migrations = []Migration{ NewMigration("Add PreviousDuration to ActionRun", v1_22.AddPreviousDurationToActionRun), // v286 -> v287 NewMigration("Add support for SHA256 git repositories", v1_22.AdjustDBForSha256), + // v287 -> v288 + NewMigration("Add Index to attachment.comment_id", v1_22.AddCommentIDIndexofAttachment), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_22/v287.go b/models/migrations/v1_22/v287.go new file mode 100644 index 0000000000000..0bfffe5d0518e --- /dev/null +++ b/models/migrations/v1_22/v287.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 1a588398c1a40..9b0de11fdc831 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 763419b7a2520..423c08936279f 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 65e74a2d90e3f..263b1717916bc 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -320,15 +320,15 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption uti 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) @@ -1591,20 +1591,20 @@ func ViewIssue(ctx *context.Context) { // Render comments and and fetch participants. participants[0] = issue.Poster + + if err := issue.Comments.LoadAttachments(ctx); err != nil { + ctx.ServerError("LoadAttachments", 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, @@ -1652,7 +1652,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 @@ -1718,10 +1717,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 { From dafae5de956d3fd914887cfe53515199e251bcfa Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Mar 2024 14:47:17 +0800 Subject: [PATCH 2/6] Add comment --- models/issues/comment_list.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index cf991a03a0ee9..febda75ef8205 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -369,6 +369,7 @@ func (comments CommentList) loadDependentIssues(ctx context.Context) error { return nil } +// getAttachmentCommentIDs only return the comment ids which possiblly has attachments func (comments CommentList) getAttachmentCommentIDs() []int64 { ids := make(container.Set[int64], len(comments)) for _, comment := range comments { From 0119e1445f9e996aa1663cff105a5abc36a07cc2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Mar 2024 15:03:44 +0800 Subject: [PATCH 3/6] Remove unnecessary database queries --- models/issues/comment_list.go | 36 ++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index febda75ef8205..f10687e4e6724 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() } @@ -427,7 +431,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() } From 24a388a378c5f92d4f1816a29f0ac760d31e78ad Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Mar 2024 15:21:55 +0800 Subject: [PATCH 4/6] Improve the optimization --- models/issues/comment_code.go | 2 +- models/issues/comment_list.go | 22 ++++++++++++++++++++++ routers/web/repo/issue.go | 2 +- routers/web/repo/pull_review.go | 8 +++----- 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 384a595dd90dc..74a7a86f26f4f 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 f10687e4e6724..5a06a5a34c190 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -386,6 +386,28 @@ func (comments CommentList) getAttachmentCommentIDs() []int64 { 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 { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 263b1717916bc..c2ec0247b9210 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1592,7 +1592,7 @@ func ViewIssue(ctx *context.Context) { // Render comments and and fetch participants. participants[0] = issue.Poster - if err := issue.Comments.LoadAttachments(ctx); err != nil { + if err := issue.Comments.LoadAttachmentsByIssue(ctx); err != nil { ctx.ServerError("LoadAttachments", err) return } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 64212291e162e..afc2a3391d3a7 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -177,11 +177,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 From c7455d6c1525b51397c19ff5b1960d7746b2a5c9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 1 Mar 2024 15:28:48 +0800 Subject: [PATCH 5/6] Fix spell --- models/issues/comment_list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 5a06a5a34c190..0047b054bac1f 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -373,7 +373,7 @@ func (comments CommentList) loadDependentIssues(ctx context.Context) error { return nil } -// getAttachmentCommentIDs only return the comment ids which possiblly has attachments +// 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 { From 63b5b4b2fec26c3c893ded571827ffc216a166bc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 12 Mar 2024 14:57:18 +0800 Subject: [PATCH 6/6] Update routers/web/repo/issue.go Co-authored-by: Zettat123 --- routers/web/repo/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 8f6c0b35250c8..8935cc80e2f79 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1606,7 +1606,7 @@ func ViewIssue(ctx *context.Context) { participants[0] = issue.Poster if err := issue.Comments.LoadAttachmentsByIssue(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) + ctx.ServerError("LoadAttachmentsByIssue", err) return } if err := issue.Comments.LoadPosters(ctx); err != nil {