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 <[email protected]>

Author: lunny

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

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,

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,23 +373,57 @@ 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 {
 		return nil
 	}
 
 	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()
 }

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
+}

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

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{})
+}

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"`

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

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 {

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