From c9854bee98e3cfd6e50a797d4da9585929a37856 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 12 Mar 2024 15:23:44 +0800 Subject: [PATCH 1/3] Do some performance optimize for issues list and view issue/pull (gitea#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 Conflicts: models/issues/comment_code.go: function was renamed in Forgejo models/migrations/migrations.go: migration already ported --- models/issues/comment.go | 8 +-- models/issues/comment_code.go | 4 +- models/issues/comment_list.go | 78 +++++++++++++++++++++------- models/issues/issue_list.go | 25 +++++++-- 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 ++- 8 files changed, 106 insertions(+), 62 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 984fb9c9f..1e962b52f 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 735c9f089..2f6f57e0d 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -90,7 +90,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_mod return pathToLineToComment, nil } -func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) { +func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) (CommentList, error) { var comments CommentList if review == nil { review = &Review{ID: 0} @@ -169,7 +169,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu } // FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number) -func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) ([]*Comment, error) { +func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) (CommentList, error) { opts := FindCommentsOptions{ Type: CommentTypeCode, IssueID: comment.IssueID, diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 30a437ea5..0047b054b 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 9ccb93bf4..218891ad3 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -391,9 +391,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 @@ -612,3 +611,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/repo/attachment.go b/models/repo/attachment.go index 64df6b166..546e409de 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 c3005dee3..819859b99 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 a4e3f7165..da811c8a7 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -328,15 +328,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) @@ -1602,20 +1602,20 @@ func ViewIssue(ctx *context.Context) { // Render comments 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, @@ -1663,7 +1663,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 @@ -1729,10 +1728,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 afa3b17d3..81af2cbb5 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -169,11 +169,9 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori } ctx.Data["PageIsPullFiles"] = (origin == "diff") - 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 33444a33082df04b05bc9d6894db47069bd7b002 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Mon, 8 Apr 2024 15:16:40 +0200 Subject: [PATCH 2/3] Fix: missing value for In() condition --- models/issues/issue_list.go | 2 +- models/issues/issue_list_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index 218891ad3..da55ff1b0 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -616,7 +616,7 @@ 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"). + In("issue_id", issueIDs). Find(&issueUsers); err != nil { return err } diff --git a/models/issues/issue_list_test.go b/models/issues/issue_list_test.go index 9069e1012..10ba38a64 100644 --- a/models/issues/issue_list_test.go +++ b/models/issues/issue_list_test.go @@ -72,4 +72,9 @@ func TestIssueList_LoadAttributes(t *testing.T) { assert.Nil(t, issue.Project) } } + + assert.NoError(t, issueList.LoadIsRead(db.DefaultContext, 1)) + for _, issue := range issueList { + assert.Equal(t, issue.ID == 1, issue.IsRead, "unexpected is_read value for issue[%d]", issue.ID) + } } From 9ae805538ac7db406a0fac6fa1692d7ed57423c6 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Mon, 8 Apr 2024 15:24:00 +0200 Subject: [PATCH 3/3] Use HasAttachmentSupport method --- models/issues/comment_list.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 0047b054b..347dbe99e 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -377,9 +377,7 @@ func (comments CommentList) loadDependentIssues(ctx context.Context) error { 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 { + if comment.Type.HasAttachmentSupport() { ids.Add(comment.ID) } }