Add commit count caching #2774

lafriks · 2017-10-23T23:15:23Z

Especially on large repositories like linux kernel getting commit count from git takes more or less 7 seconds, so let's cache it as commit count is needed on every repository page load.

For linux kernel repository (more then 700K commits) it brings down repository page load time from 11.5 seconds to 3.3 seconds on my development machine.

[Macaron] 2017-10-24 16:50:25: Started GET /user1/linux for [::1]
[Macaron] 2017-10-24 16:50:36: Completed /user1/linux 200 OK in 11.4833512s
[Macaron] 2017-10-24 16:50:56: Started GET /user1/linux for [::1]
[Macaron] 2017-10-24 16:50:59: Completed /user1/linux 200 OK in 3.2968169s

codecov-io · 2017-10-23T23:23:05Z

Codecov Report

Merging #2774 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2774      +/-   ##
==========================================
- Coverage   27.23%   27.21%   -0.03%     
==========================================
  Files          88       88              
  Lines       17333    17348      +15     
==========================================
  Hits         4721     4721              
- Misses      11927    11942      +15     
  Partials      685      685

Impacted Files	Coverage Δ
models/update.go	`10.75% <0%> (-0.43%)`	⬇️
models/repo.go	`13.06% <0%> (-0.07%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ab580c...fcc0bef. Read the comment docs.

ethantkoenig

A few minor things

ethantkoenig · 2017-10-24T03:52:08Z

models/update.go

 			if err != nil {
 				return nil, fmt.Errorf("pushUpdateAddTag: %v", err)
 			}
 		}
 	} else if !isDelRef {
 		// If is branch reference
+
+		// Clear cache for branch commit counr


typo: counr

ethantkoenig

(forgot to include a comment)

ethantkoenig · 2017-10-24T03:54:35Z

modules/context/repo.go

+// GetCommitsCount returns cached commit count for current view
+func (r *Repository) GetCommitsCount() (int64, error) {
+	if r.IsViewBranch {
+		return cache.GetInt64(fmt.Sprintf("branch-commits-count-%d-%s", r.Repository.ID, r.BranchName), func() (int64, error) {


Could we add helper functions for these cache keys instead of having "magic" strings? e.g

func BranchCommitsCountKey(repoID int64, branchName string) string { ... }

With helper functions, we don't have to worry about mistyped strings, and it makes the code cleaner IMO

bkcsoft · 2017-10-24T12:43:15Z

modules/context/repo.go

+// GetCommitsCount returns cached commit count for current view
+func (r *Repository) GetCommitsCount() (int64, error) {
+	if r.IsViewBranch {
+		return cache.GetInt64(fmt.Sprintf("branch-commits-count-%d-%s", r.Repository.ID, r.BranchName), func() (int64, error) {


To me it feels like this should be in the model instead. Because now we end up with some places having it cached, and other places direct access 😕

@bkcsoft
~~Theoretically I can move it to model and make it less context dependant as branch and tag names can not be the with same name anyway~~

EDIT: I can't really move it to model as it is context dependent

lafriks · 2017-10-24T15:58:11Z

@ethantkoenig fixed

ethantkoenig · 2017-10-24T16:32:41Z

models/repo.go

@@ -258,6 +258,11 @@ func (repo *Repository) APIFormat(mode AccessMode) *api.Repository {
 	return repo.innerAPIFormat(mode, false)
 }

+// GetCommitsCountCacheKey returns cache key used for commits count caching.
+func (repo *Repository) GetCommitsCountCacheKey(contextName string) string {


This naming scheme is ambiguous. Although branch names cannot conflict with tag names, commit names may conflict with either branch or tag names (i.e. if a branch/tag's name matches a commit's SHA).

I think we'll need multiple functions for each type (branch, tag, commit)

We can't display it in repo page anyway but ok I'll add it back :)

lafriks · 2017-10-25T06:32:26Z

@ethantkoenig done

ethantkoenig · 2017-10-25T16:02:46Z

LGTM

lunny · 2017-10-25T16:07:45Z

modules/setting/setting.go

 func newCacheService() {
-	CacheAdapter = Cfg.Section("cache").Key("ADAPTER").In("memory", []string{"memory", "redis", "memcache"})
-	switch CacheAdapter {
+	sec := Cfg.Section("mailer")


Why mailer not cache?

Ouch, good catch 👍

I mean all the cache. If I have only serval repos on my private VPS. I maybe don't want to enable cache to spent the memory.

lunny · 2017-10-25T16:08:33Z

And the cache could be disabled?

lafriks · 2017-10-25T16:21:39Z

@lunny do you mean disabling all caching or only commit count caching?

lafriks · 2017-10-25T16:29:26Z

@lunny I can add option for that but does that mean also disabling go-macaron cache? I don't know if it is actually used currently

lafriks · 2017-10-25T16:36:30Z

Just to note - commits are cached for 1 hour and if not accessed in this time are removed from memory. It could possibly be better to add configurable cache time for each item

lafriks · 2017-10-25T19:26:10Z

@lunny fixed and added option to change caching time or disable it (setting caching time to 0)

lunny · 2017-10-26T01:26:43Z

LGTM

* Add commit count caching * Small refactoring * Add different key prefix for refs and commits * Add configuratuion option to allow to change caching time or disable it

lafriks added the type/enhancement An improvement of existing functionality label Oct 23, 2017

lafriks added this to the 1.3.0 milestone Oct 23, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 23, 2017

ethantkoenig suggested changes Oct 24, 2017

View reviewed changes

bkcsoft reviewed Oct 24, 2017

View reviewed changes

lafriks force-pushed the feat/add_caching branch from 8b0904d to 02dba2e Compare October 24, 2017 14:21

ethantkoenig suggested changes Oct 24, 2017

View reviewed changes

lafriks force-pushed the feat/add_caching branch from 02dba2e to a0234a3 Compare October 25, 2017 06:32

ethantkoenig approved these changes Oct 25, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 25, 2017

lunny requested changes Oct 25, 2017

View reviewed changes

lafriks added 4 commits October 25, 2017 21:36

Add commit count caching

d3bb548

Small refactoring

2aa90de

Add different key prefix for refs and commits

89b8156

Add configuratuion option to allow to change caching time or disable it

faaf672

lafriks force-pushed the feat/add_caching branch from a0234a3 to faaf672 Compare October 25, 2017 19:20

lunny approved these changes Oct 26, 2017

View reviewed changes

tboerger removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 26, 2017

tboerger added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Oct 26, 2017

Merge branch 'master' into feat/add_caching

fcc0bef

lunny merged commit eca05b0 into go-gitea:master Oct 26, 2017

lunny mentioned this pull request Nov 15, 2017

HTTP 500 when entering project page #2920

Closed

8 tasks

lafriks deleted the feat/add_caching branch November 15, 2017 16:04

lunny mentioned this pull request Apr 19, 2019

Improve listing performance by using go-git #6478

Merged

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commit count caching #2774

Add commit count caching #2774

lafriks commented Oct 23, 2017 •

edited

Loading

codecov-io commented Oct 23, 2017 •

edited

Loading

ethantkoenig left a comment

ethantkoenig Oct 24, 2017

ethantkoenig left a comment

ethantkoenig Oct 24, 2017

bkcsoft Oct 24, 2017

lafriks Oct 24, 2017 •

edited

Loading

lafriks commented Oct 24, 2017

ethantkoenig Oct 24, 2017 •

edited

Loading

lafriks Oct 24, 2017

lafriks commented Oct 25, 2017

ethantkoenig commented Oct 25, 2017

lunny Oct 25, 2017

lafriks Oct 25, 2017

lunny Oct 25, 2017

lunny commented Oct 25, 2017

lafriks commented Oct 25, 2017

lafriks commented Oct 25, 2017

lafriks commented Oct 25, 2017

lafriks commented Oct 25, 2017

lunny commented Oct 26, 2017

Add commit count caching #2774

Add commit count caching #2774

Conversation

lafriks commented Oct 23, 2017 • edited Loading

codecov-io commented Oct 23, 2017 • edited Loading

Codecov Report

ethantkoenig left a comment

Choose a reason for hiding this comment

ethantkoenig Oct 24, 2017

Choose a reason for hiding this comment

ethantkoenig left a comment

Choose a reason for hiding this comment

ethantkoenig Oct 24, 2017

Choose a reason for hiding this comment

bkcsoft Oct 24, 2017

Choose a reason for hiding this comment

lafriks Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

lafriks commented Oct 24, 2017

ethantkoenig Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

lafriks Oct 24, 2017

Choose a reason for hiding this comment

lafriks commented Oct 25, 2017

ethantkoenig commented Oct 25, 2017

lunny Oct 25, 2017

Choose a reason for hiding this comment

lafriks Oct 25, 2017

Choose a reason for hiding this comment

lunny Oct 25, 2017

Choose a reason for hiding this comment

lunny commented Oct 25, 2017

lafriks commented Oct 25, 2017

lafriks commented Oct 25, 2017

lafriks commented Oct 25, 2017

lafriks commented Oct 25, 2017

lunny commented Oct 26, 2017

lafriks commented Oct 23, 2017 •

edited

Loading

codecov-io commented Oct 23, 2017 •

edited

Loading

lafriks Oct 24, 2017 •

edited

Loading

ethantkoenig Oct 24, 2017 •

edited

Loading