loading up the forgejo repo on tangled to test page performance
0
fork

Configure Feed

Select the types of activity you want to include in your feed.

Use db.ListOptions directly instead of Paginator interface to make it easier to use and fix performance of /pulls and /issues (#29990)

This PR uses `db.ListOptions` instead of `Paginor` to make the code
simpler.
And it also fixed the performance problem when viewing /pulls or
/issues. Before the counting in fact will also do the search.

---------

Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: silverwind <me@silverwind.io>
(cherry picked from commit 3f26fe2fa2c7141c9e622297e50a70f3e0003e4d)

authored by

Lunny Xiao
Jason Song
silverwind
and committed by
Earl Warren
9ca245ad 5016bc5d

+49 -34
+5 -17
models/issues/issue_search.go
··· 21 21 22 22 // IssuesOptions represents options of an issue. 23 23 type IssuesOptions struct { //nolint 24 - db.Paginator 24 + Paginator *db.ListOptions 25 25 RepoIDs []int64 // overwrites RepoCond if the length is not 0 26 26 AllPublic bool // include also all public repositories 27 27 RepoCond builder.Cond ··· 104 104 return sess 105 105 } 106 106 107 - // Warning: Do not use GetSkipTake() for *db.ListOptions 108 - // Its implementation could reset the page size with setting.API.MaxResponseItems 109 - if listOptions, ok := opts.Paginator.(*db.ListOptions); ok { 110 - if listOptions.Page >= 0 && listOptions.PageSize > 0 { 111 - var start int 112 - if listOptions.Page == 0 { 113 - start = 0 114 - } else { 115 - start = (listOptions.Page - 1) * listOptions.PageSize 116 - } 117 - sess.Limit(listOptions.PageSize, start) 118 - } 119 - return sess 107 + start := 0 108 + if opts.Paginator.Page > 1 { 109 + start = (opts.Paginator.Page - 1) * opts.Paginator.PageSize 120 110 } 121 - 122 - start, limit := opts.Paginator.GetSkipTake() 123 - sess.Limit(limit, start) 111 + sess.Limit(opts.Paginator.PageSize, start) 124 112 125 113 return sess 126 114 }
+5 -1
models/issues/issue_stats.go
··· 68 68 } 69 69 70 70 // CountIssues number return of issues by given conditions. 71 - func CountIssues(ctx context.Context, opts *IssuesOptions) (int64, error) { 71 + func CountIssues(ctx context.Context, opts *IssuesOptions, otherConds ...builder.Cond) (int64, error) { 72 72 sess := db.GetEngine(ctx). 73 73 Select("COUNT(issue.id) AS count"). 74 74 Table("issue"). 75 75 Join("INNER", "repository", "`issue`.repo_id = `repository`.id") 76 76 applyConditions(sess, opts) 77 + 78 + for _, cond := range otherConds { 79 + sess.And(cond) 80 + } 77 81 78 82 return sess.Count() 79 83 }
+7 -14
modules/indexer/internal/paginator.go
··· 10 10 ) 11 11 12 12 // ParsePaginator parses a db.Paginator into a skip and limit 13 - func ParsePaginator(paginator db.Paginator, max ...int) (int, int) { 13 + func ParsePaginator(paginator *db.ListOptions, max ...int) (int, int) { 14 14 // Use a very large number to indicate no limit 15 15 unlimited := math.MaxInt32 16 16 if len(max) > 0 { ··· 19 19 } 20 20 21 21 if paginator == nil || paginator.IsListAll() { 22 + // It shouldn't happen. In actual usage scenarios, there should not be requests to search all. 23 + // But if it does happen, respect it and return "unlimited". 24 + // And it's also useful for testing. 22 25 return 0, unlimited 23 26 } 24 27 25 - // Warning: Do not use GetSkipTake() for *db.ListOptions 26 - // Its implementation could reset the page size with setting.API.MaxResponseItems 27 - if listOptions, ok := paginator.(*db.ListOptions); ok { 28 - if listOptions.Page >= 0 && listOptions.PageSize > 0 { 29 - var start int 30 - if listOptions.Page == 0 { 31 - start = 0 32 - } else { 33 - start = (listOptions.Page - 1) * listOptions.PageSize 34 - } 35 - return start, listOptions.PageSize 36 - } 37 - return 0, unlimited 28 + if paginator.PageSize == 0 { 29 + // Do not return any results when searching, it's used to get the total count only. 30 + return 0, 0 38 31 } 39 32 40 33 return paginator.GetSkipTake()
+11
modules/indexer/issues/db/db.go
··· 78 78 return nil, err 79 79 } 80 80 81 + // If pagesize == 0, return total count only. It's a special case for search count. 82 + if options.Paginator != nil && options.Paginator.PageSize == 0 { 83 + total, err := issue_model.CountIssues(ctx, opt, cond) 84 + if err != nil { 85 + return nil, err 86 + } 87 + return &internal.SearchResult{ 88 + Total: total, 89 + }, nil 90 + } 91 + 81 92 ids, total, err := issue_model.IssueIDs(ctx, opt, cond) 82 93 if err != nil { 83 94 return nil, err
+1 -1
modules/indexer/issues/indexer.go
··· 308 308 309 309 // CountIssues counts issues by options. It is a shortcut of SearchIssues(ctx, opts) but only returns the total count. 310 310 func CountIssues(ctx context.Context, opts *SearchOptions) (int64, error) { 311 - opts = opts.Copy(func(options *SearchOptions) { opts.Paginator = &db_model.ListOptions{PageSize: 0} }) 311 + opts = opts.Copy(func(options *SearchOptions) { options.Paginator = &db_model.ListOptions{PageSize: 0} }) 312 312 313 313 _, total, err := SearchIssues(ctx, opts) 314 314 return total, err
+1 -1
modules/indexer/issues/internal/model.go
··· 106 106 UpdatedAfterUnix optional.Option[int64] 107 107 UpdatedBeforeUnix optional.Option[int64] 108 108 109 - db.Paginator 109 + Paginator *db.ListOptions 110 110 111 111 SortBy SortBy // sort by field 112 112 }
+7
modules/indexer/issues/internal/tests/tests.go
··· 77 77 assert.Equal(t, c.ExpectedIDs, ids) 78 78 assert.Equal(t, c.ExpectedTotal, result.Total) 79 79 } 80 + 81 + // test counting 82 + c.SearchOptions.Paginator = &db.ListOptions{PageSize: 0} 83 + countResult, err := indexer.Search(context.Background(), c.SearchOptions) 84 + require.NoError(t, err) 85 + assert.Empty(t, countResult.Hits) 86 + assert.Equal(t, result.Total, countResult.Total) 80 87 }) 81 88 } 82 89 }
+12
modules/indexer/issues/meilisearch/meilisearch.go
··· 218 218 219 219 skip, limit := indexer_internal.ParsePaginator(options.Paginator, maxTotalHits) 220 220 221 + counting := limit == 0 222 + if counting { 223 + // If set limit to 0, it will be 20 by default, and -1 is not allowed. 224 + // See https://www.meilisearch.com/docs/reference/api/search#limit 225 + // So set limit to 1 to make the cost as low as possible, then clear the result before returning. 226 + limit = 1 227 + } 228 + 221 229 keyword := options.Keyword 222 230 if !options.IsFuzzyKeyword { 223 231 // to make it non fuzzy ("typo tolerance" in meilisearch terms), we have to quote the keyword(s) ··· 234 242 }) 235 243 if err != nil { 236 244 return nil, err 245 + } 246 + 247 + if counting { 248 + searchRes.Hits = nil 237 249 } 238 250 239 251 hits, err := convertHits(searchRes)