Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache last commit to accelerate the repository directory page visit #10069

Merged
merged 10 commits into from Feb 1, 2020

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 30, 2020

Will replace #6045, resolve #491

On my local test (macOS ) with default cache setting(memory):

aports (home directory): 4010ms -> 225ms
vdsm (home directory): 1510ms -> 112ms

BTW: aports /main has many files, so I haven't tested it. We need a limitation on files listed. That will be another PR.

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 30, 2020
@lunny lunny added this to the 1.12.0 milestone Jan 30, 2020
@6543
Copy link
Member

6543 commented Jan 30, 2020

@lunny instead of a hard lim, could we paginate it somehow?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 30, 2020
@6543
Copy link
Member

6543 commented Jan 30, 2020

can you add some tests?

@silverwind
Copy link
Member

Why the separate options? Is the new cache expected to go big?

@6543
Copy link
Member

6543 commented Jan 30, 2020

I like the added Enable option!
@silverwind for me: I'm totaly fine with more options ...
so you can tweek around as instance admin

@lafriks
Copy link
Member

lafriks commented Jan 30, 2020

I don't think we need separate cache for last commit, just use cache we already have in place

@silverwind
Copy link
Member

Yeah, if there is no compelling reason to make it a separate cache, I'd prefer it to be included in the regular cache.

@lunny lunny force-pushed the lunny/cache_file_last_commit3 branch from 2b05749 to 1f0edd1 Compare January 31, 2020 02:07
@lunny
Copy link
Member Author

lunny commented Jan 31, 2020

@lunny instead of a hard lim, could we paginate it somehow?

That will be another PR.

@lafriks @silverwind I have sent another commit that will use the original cache config defautly.

I think the default cache time 16h is not a good default value and I also needs some extra config items like commits_count and etc. So I added a seperate cache config.

@lunny
Copy link
Member Author

lunny commented Jan 31, 2020

@6543 I will try to add a test but since it's a page performance optimization for big repository but the test repositories are small. The test will only confirm the results are the same before enable the cache or after.

@codecov-io
Copy link

codecov-io commented Jan 31, 2020

Codecov Report

Merging #10069 into master will increase coverage by 0.01%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10069      +/-   ##
==========================================
+ Coverage   43.48%   43.49%   +0.01%     
==========================================
  Files         567      567              
  Lines       79067    79104      +37     
==========================================
+ Hits        34381    34409      +28     
- Misses      40440    40447       +7     
- Partials     4246     4248       +2
Impacted Files Coverage Δ
routers/repo/branch.go 55.72% <100%> (+0.34%) ⬆️
modules/references/references.go 80.32% <80.85%> (+0.41%) ⬆️
services/pull/check.go 54.54% <0%> (-2.1%) ⬇️
services/pull/patch.go 66.03% <0%> (-1.89%) ⬇️
modules/queue/workerpool.go 46.61% <0%> (-1.78%) ⬇️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️
modules/queue/manager.go 62.05% <0%> (+1.53%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️

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 8c15871...300e790. Read the comment docs.

@6543
Copy link
Member

6543 commented Jan 31, 2020

@6543 I will try to add a test but since it's a page performance optimization for big repository but the test repositories are small. The test will only confirm the results are the same before enable the cache or after.

i think it is enouth if you add test to test if cache works at all ... (set cache-size to low value .. ; open repo & test if it is in the cash) ?

@GiteaBot GiteaBot 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 Jan 31, 2020
@lunny lunny force-pushed the lunny/cache_file_last_commit3 branch from 1f0edd1 to e23901c Compare January 31, 2020 05:34
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny only following settings are needed for [cache.last_commit] section

  • ENABLED: true: Enable the cache. - edit: setting ITEM_TTL to 0 will disable cache so no need for this option also
  • ITEM_TTL: 16h: Time to keep items in cache if not used, Setting it to 0 disables caching.
  • COMMITS_COUNT: 1000: Only enable the cache when repository's commits count great than.

You can reuse same cache and set specific TTL for last_commit keys.

@lafriks
Copy link
Member

lafriks commented Jan 31, 2020

@lunny I created PR to your PR with what I meant: lunny#4

@lunny lunny force-pushed the lunny/cache_file_last_commit3 branch from 58633a0 to b5ed5bd Compare January 31, 2020 07:49
@lunny
Copy link
Member Author

lunny commented Jan 31, 2020

@lafriks done.

@6543
Copy link
Member

6543 commented Jan 31, 2020

without

  • aports/src/branch/master/main: 78375ms(first) - 76574ms(snd.)
  • aports/src/branch/master/community: 38033ms(first) - 36449ms(snd.)

with

  • aports/src/branch/master/main: 77753ms(first) - 591ms(snd.)
  • aports/src/branch/master/community: 37208ms(first) - 685ms(snd.)

@silverwind
Copy link
Member

Maybe pre-populate cache on startup and push?

@6543
Copy link
Member

6543 commented Jan 31, 2020

I think the endresult is just more load on the system ... if we implement this i like to be able to disable this @silverwind

@lunny
Copy link
Member Author

lunny commented Jan 31, 2020

@silverwind I will send another PR to do that. Because I think it's need a queue that will make this PR more complicated.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 1, 2020
@lunny
Copy link
Member Author

lunny commented Feb 1, 2020

@lafriks need your approval.

@zeripath zeripath merged commit ce7062a into go-gitea:master Feb 1, 2020
@lunny lunny deleted the lunny/cache_file_last_commit3 branch February 2, 2020 01:50
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow page loads with a large repo
8 participants