From dd1acd7ce4b4afc5d1c803f85767def242a899bb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 18 Apr 2019 13:00:03 +0800 Subject: [PATCH] Comments list performance optimization (#5305) --- models/issue.go | 4 + models/issue_comment.go | 66 ++-- models/issue_comment_list.go | 498 ++++++++++++++++++++++++++- routers/api/v1/repo/issue_comment.go | 25 +- 4 files changed, 553 insertions(+), 40 deletions(-) diff --git a/models/issue.go b/models/issue.go index 753345d38..ddc7fa24a 100644 --- a/models/issue.go +++ b/models/issue.go @@ -272,6 +272,10 @@ func (issue *Issue) loadAttributes(e Engine) (err error) { if err = issue.loadComments(e); err != nil { return err } + + if err = CommentList(issue.Comments).loadAttributes(e); err != nil { + return err + } if issue.isTimetrackerEnabled(e) { if err = issue.loadTotalTimes(e); err != nil { return err diff --git a/models/issue_comment.go b/models/issue_comment.go index 360f5a659..a1c2364b0 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -154,6 +154,34 @@ func (c *Comment) LoadIssue() (err error) { return } +func (c *Comment) loadPoster(e Engine) (err error) { + if c.Poster != nil { + return nil + } + + c.Poster, err = getUserByID(e, c.PosterID) + if err != nil { + if IsErrUserNotExist(err) { + c.PosterID = -1 + c.Poster = NewGhostUser() + } else { + log.Error("getUserByID[%d]: %v", c.ID, err) + } + } + return err +} + +func (c *Comment) loadAttachments(e Engine) (err error) { + if len(c.Attachments) > 0 { + return + } + c.Attachments, err = getAttachmentsByCommentID(e, c.ID) + if err != nil { + log.Error("getAttachmentsByCommentID[%d]: %v", c.ID, err) + } + return err +} + // AfterDelete is invoked from XORM after the object is deleted. func (c *Comment) AfterDelete() { if c.ID <= 0 { @@ -997,32 +1025,6 @@ func FindComments(opts FindCommentsOptions) ([]*Comment, error) { return findComments(x, opts) } -// GetCommentsByIssueID returns all comments of an issue. -func GetCommentsByIssueID(issueID int64) ([]*Comment, error) { - return findComments(x, FindCommentsOptions{ - IssueID: issueID, - Type: CommentTypeUnknown, - }) -} - -// GetCommentsByIssueIDSince returns a list of comments of an issue since a given time point. -func GetCommentsByIssueIDSince(issueID, since int64) ([]*Comment, error) { - return findComments(x, FindCommentsOptions{ - IssueID: issueID, - Type: CommentTypeUnknown, - Since: since, - }) -} - -// GetCommentsByRepoIDSince returns a list of comments for all issues in a repo since a given time point. -func GetCommentsByRepoIDSince(repoID, since int64) ([]*Comment, error) { - return findComments(x, FindCommentsOptions{ - RepoID: repoID, - Type: CommentTypeUnknown, - Since: since, - }) -} - // UpdateComment updates information of comment. func UpdateComment(doer *User, c *Comment, oldContent string) error { if _, err := x.ID(c.ID).AllCols().Update(c); err != nil { @@ -1039,6 +1041,9 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error { if err := c.Issue.LoadAttributes(); err != nil { return err } + if err := c.loadPoster(x); err != nil { + return err + } mode, _ := AccessLevel(doer, c.Issue.Repo) if err := PrepareWebhooks(c.Issue.Repo, HookEventIssueComment, &api.IssueCommentPayload{ @@ -1087,6 +1092,7 @@ func DeleteComment(doer *User, comment *Comment) error { if err := sess.Commit(); err != nil { return err } + sess.Close() if err := comment.LoadPoster(); err != nil { return err @@ -1098,6 +1104,9 @@ func DeleteComment(doer *User, comment *Comment) error { if err := comment.Issue.LoadAttributes(); err != nil { return err } + if err := comment.loadPoster(x); err != nil { + return err + } mode, _ := AccessLevel(doer, comment.Issue.Repo) @@ -1154,6 +1163,11 @@ func fetchCodeCommentsByReview(e Engine, issue *Issue, currentUser *User, review if err := issue.loadRepo(e); err != nil { return nil, err } + + if err := CommentList(comments).loadPosters(e); err != nil { + return nil, err + } + // Find all reviews by ReviewID reviews := make(map[int64]*Review) var ids = make([]int64, 0, len(comments)) diff --git a/models/issue_comment_list.go b/models/issue_comment_list.go index 35c1253f6..a8c812328 100644 --- a/models/issue_comment_list.go +++ b/models/issue_comment_list.go @@ -8,18 +8,13 @@ package models type CommentList []*Comment func (comments CommentList) getPosterIDs() []int64 { - commentIDs := make(map[int64]struct{}, len(comments)) + posterIDs := make(map[int64]struct{}, len(comments)) for _, comment := range comments { - if _, ok := commentIDs[comment.PosterID]; !ok { - commentIDs[comment.PosterID] = struct{}{} + if _, ok := posterIDs[comment.PosterID]; !ok { + posterIDs[comment.PosterID] = struct{}{} } } - return keysInt64(commentIDs) -} - -// LoadPosters loads posters from database -func (comments CommentList) LoadPosters() error { - return comments.loadPosters(x) + return keysInt64(posterIDs) } func (comments CommentList) loadPosters(e Engine) error { @@ -56,3 +51,488 @@ func (comments CommentList) loadPosters(e Engine) error { } return nil } + +func (comments CommentList) getCommentIDs() []int64 { + var ids = make([]int64, 0, len(comments)) + for _, comment := range comments { + ids = append(ids, comment.ID) + } + return ids +} + +func (comments CommentList) getLabelIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if _, ok := ids[comment.LabelID]; !ok { + ids[comment.LabelID] = struct{}{} + } + } + return keysInt64(ids) +} + +func (comments CommentList) loadLabels(e Engine) error { + if len(comments) == 0 { + return nil + } + + var labelIDs = comments.getLabelIDs() + var commentLabels = make(map[int64]*Label, len(labelIDs)) + var left = len(labelIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := e. + In("id", labelIDs[:limit]). + Rows(new(Label)) + if err != nil { + return err + } + + for rows.Next() { + var label Label + err = rows.Scan(&label) + if err != nil { + rows.Close() + return err + } + commentLabels[label.ID] = &label + } + rows.Close() + left = left - limit + labelIDs = labelIDs[limit:] + } + + for _, comment := range comments { + comment.Label = commentLabels[comment.ID] + } + return nil +} + +func (comments CommentList) getMilestoneIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if _, ok := ids[comment.MilestoneID]; !ok { + ids[comment.MilestoneID] = struct{}{} + } + } + return keysInt64(ids) +} + +func (comments CommentList) loadMilestones(e Engine) error { + if len(comments) == 0 { + return nil + } + + milestoneIDs := comments.getMilestoneIDs() + if len(milestoneIDs) == 0 { + return nil + } + + milestoneMaps := make(map[int64]*Milestone, len(milestoneIDs)) + var left = len(milestoneIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + err := e. + In("id", milestoneIDs[:limit]). + Find(&milestoneMaps) + if err != nil { + return err + } + left = left - limit + milestoneIDs = milestoneIDs[limit:] + } + + for _, issue := range comments { + issue.Milestone = milestoneMaps[issue.MilestoneID] + } + return nil +} + +func (comments CommentList) getOldMilestoneIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if _, ok := ids[comment.OldMilestoneID]; !ok { + ids[comment.OldMilestoneID] = struct{}{} + } + } + return keysInt64(ids) +} + +func (comments CommentList) loadOldMilestones(e Engine) error { + if len(comments) == 0 { + return nil + } + + milestoneIDs := comments.getOldMilestoneIDs() + if len(milestoneIDs) == 0 { + return nil + } + + milestoneMaps := make(map[int64]*Milestone, len(milestoneIDs)) + var left = len(milestoneIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + err := e. + In("id", milestoneIDs[:limit]). + Find(&milestoneMaps) + if err != nil { + return err + } + left = left - limit + milestoneIDs = milestoneIDs[limit:] + } + + for _, issue := range comments { + issue.OldMilestone = milestoneMaps[issue.MilestoneID] + } + return nil +} + +func (comments CommentList) getAssigneeIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if _, ok := ids[comment.AssigneeID]; !ok { + ids[comment.AssigneeID] = struct{}{} + } + } + return keysInt64(ids) +} + +func (comments CommentList) loadAssignees(e Engine) error { + if len(comments) == 0 { + return nil + } + + var assigneeIDs = comments.getAssigneeIDs() + var assignees = make(map[int64]*User, len(assigneeIDs)) + var left = len(assigneeIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := e. + In("id", assigneeIDs[:limit]). + Rows(new(User)) + if err != nil { + return err + } + + for rows.Next() { + var user User + err = rows.Scan(&user) + if err != nil { + rows.Close() + return err + } + + assignees[user.ID] = &user + } + rows.Close() + + left = left - limit + assigneeIDs = assigneeIDs[limit:] + } + + for _, comment := range comments { + comment.Assignee = assignees[comment.AssigneeID] + } + return nil +} + +// getIssueIDs returns all the issue ids on this comment list which issue hasn't been loaded +func (comments CommentList) getIssueIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if comment.Issue != nil { + continue + } + if _, ok := ids[comment.IssueID]; !ok { + ids[comment.IssueID] = struct{}{} + } + } + return keysInt64(ids) +} + +// Issues returns all the issues of comments +func (comments CommentList) Issues() IssueList { + var issues = make(map[int64]*Issue, len(comments)) + for _, comment := range comments { + if comment.Issue != nil { + if _, ok := issues[comment.Issue.ID]; !ok { + issues[comment.Issue.ID] = comment.Issue + } + } + } + + var issueList = make([]*Issue, 0, len(issues)) + for _, issue := range issues { + issueList = append(issueList, issue) + } + return issueList +} + +func (comments CommentList) loadIssues(e Engine) error { + if len(comments) == 0 { + return nil + } + + var issueIDs = comments.getIssueIDs() + var issues = make(map[int64]*Issue, len(issueIDs)) + var left = len(issueIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := e. + In("id", issueIDs[:limit]). + Rows(new(Issue)) + if err != nil { + return err + } + + for rows.Next() { + var issue Issue + err = rows.Scan(&issue) + if err != nil { + rows.Close() + return err + } + + issues[issue.ID] = &issue + } + rows.Close() + + left = left - limit + issueIDs = issueIDs[limit:] + } + + for _, comment := range comments { + if comment.Issue == nil { + comment.Issue = issues[comment.IssueID] + } + } + return nil +} + +func (comments CommentList) getDependentIssueIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if comment.DependentIssue != nil { + continue + } + if _, ok := ids[comment.DependentIssueID]; !ok { + ids[comment.DependentIssueID] = struct{}{} + } + } + return keysInt64(ids) +} + +func (comments CommentList) loadDependentIssues(e Engine) error { + if len(comments) == 0 { + return nil + } + + var issueIDs = comments.getDependentIssueIDs() + var issues = make(map[int64]*Issue, len(issueIDs)) + var left = len(issueIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := e. + In("id", issueIDs[:limit]). + Rows(new(Issue)) + if err != nil { + return err + } + + for rows.Next() { + var issue Issue + err = rows.Scan(&issue) + if err != nil { + rows.Close() + return err + } + + issues[issue.ID] = &issue + } + rows.Close() + + left = left - limit + issueIDs = issueIDs[limit:] + } + + for _, comment := range comments { + if comment.DependentIssue == nil { + comment.DependentIssue = issues[comment.DependentIssueID] + } + } + return nil +} + +func (comments CommentList) loadAttachments(e Engine) (err error) { + if len(comments) == 0 { + return nil + } + + var attachments = make(map[int64][]*Attachment, len(comments)) + var commentsIDs = comments.getCommentIDs() + var left = len(commentsIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := e.Table("attachment"). + Join("INNER", "comment", "comment.id = attachment.comment_id"). + In("comment.id", commentsIDs[:limit]). + Rows(new(Attachment)) + if err != nil { + return err + } + + for rows.Next() { + var attachment Attachment + err = rows.Scan(&attachment) + if err != nil { + rows.Close() + return err + } + attachments[attachment.CommentID] = append(attachments[attachment.CommentID], &attachment) + } + + rows.Close() + left = left - limit + commentsIDs = commentsIDs[limit:] + } + + for _, comment := range comments { + comment.Attachments = attachments[comment.ID] + } + return nil +} + +func (comments CommentList) getReviewIDs() []int64 { + var ids = make(map[int64]struct{}, len(comments)) + for _, comment := range comments { + if _, ok := ids[comment.ReviewID]; !ok { + ids[comment.ReviewID] = struct{}{} + } + } + return keysInt64(ids) +} + +func (comments CommentList) loadReviews(e Engine) error { + if len(comments) == 0 { + return nil + } + + var reviewIDs = comments.getReviewIDs() + var reviews = make(map[int64]*Review, len(reviewIDs)) + var left = len(reviewIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := e. + In("id", reviewIDs[:limit]). + Rows(new(Review)) + if err != nil { + return err + } + + for rows.Next() { + var review Review + err = rows.Scan(&review) + if err != nil { + rows.Close() + return err + } + + reviews[review.ID] = &review + } + rows.Close() + + left = left - limit + reviewIDs = reviewIDs[limit:] + } + + for _, comment := range comments { + comment.Review = reviews[comment.ReviewID] + } + return nil +} + +// loadAttributes loads all attributes +func (comments CommentList) loadAttributes(e Engine) (err error) { + if err = comments.loadPosters(e); err != nil { + return + } + + if err = comments.loadLabels(e); err != nil { + return + } + + if err = comments.loadMilestones(e); err != nil { + return + } + + if err = comments.loadOldMilestones(e); err != nil { + return + } + + if err = comments.loadAssignees(e); err != nil { + return + } + + if err = comments.loadAttachments(e); err != nil { + return + } + + if err = comments.loadReviews(e); err != nil { + return + } + + if err = comments.loadIssues(e); err != nil { + return + } + + if err = comments.loadDependentIssues(e); err != nil { + return + } + + return nil +} + +// LoadAttributes loads attributes of the comments, except for attachments and +// comments +func (comments CommentList) LoadAttributes() error { + return comments.loadAttributes(x) +} + +// LoadAttachments loads attachments +func (comments CommentList) LoadAttachments() error { + return comments.loadAttachments(x) +} + +// LoadPosters loads posters +func (comments CommentList) LoadPosters() error { + return comments.loadPosters(x) +} + +// LoadIssues loads issues of comments +func (comments CommentList) LoadIssues() error { + return comments.loadIssues(x) +} diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index fd085b66b..7a720deab 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -57,6 +57,7 @@ func ListIssueComments(ctx *context.APIContext) { ctx.Error(500, "GetRawIssueByIndex", err) return } + issue.Repo = ctx.Repo.Repository comments, err := models.FindComments(models.FindCommentsOptions{ IssueID: issue.ID, @@ -64,16 +65,18 @@ func ListIssueComments(ctx *context.APIContext) { Type: models.CommentTypeComment, }) if err != nil { - ctx.Error(500, "GetCommentsByIssueIDSince", err) + ctx.Error(500, "FindComments", err) return } - apiComments := make([]*api.Comment, len(comments)) - if err = models.CommentList(comments).LoadPosters(); err != nil { + if err := models.CommentList(comments).LoadPosters(); err != nil { ctx.Error(500, "LoadPosters", err) return } - for i := range comments { + + apiComments := make([]*api.Comment, len(comments)) + for i, comment := range comments { + comment.Issue = issue apiComments[i] = comments[i].APIFormat() } ctx.JSON(200, &apiComments) @@ -115,7 +118,7 @@ func ListRepoIssueComments(ctx *context.APIContext) { Type: models.CommentTypeComment, }) if err != nil { - ctx.Error(500, "GetCommentsByRepoIDSince", err) + ctx.Error(500, "FindComments", err) return } @@ -125,6 +128,18 @@ func ListRepoIssueComments(ctx *context.APIContext) { } apiComments := make([]*api.Comment, len(comments)) + if err := models.CommentList(comments).LoadIssues(); err != nil { + ctx.Error(500, "LoadIssues", err) + return + } + if err := models.CommentList(comments).LoadPosters(); err != nil { + ctx.Error(500, "LoadPosters", err) + return + } + if _, err := models.CommentList(comments).Issues().LoadRepositories(); err != nil { + ctx.Error(500, "LoadRepositories", err) + return + } for i := range comments { apiComments[i] = comments[i].APIFormat() }