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

Improve render directory #15891

Closed
wants to merge 19 commits into from
Closed

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented May 16, 2021

This PR represents a small improvement for the native git backend's get
last commit. It reduces the time taken on ports from 13s to 6.5s which is
now faster than the go-git variant.

Signed-off-by: Andrew Thornton art27@cantab.net

This PR represents a small improvement for the native git backend's get
last commit. It reduces the time taken on ports from 13s to 9s which is
just 1s slower than the go-git variant.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added performance/speed performance issues with slow downs performance/bigrepo Performance Issues affecting Big Repositories labels May 16, 2021
@zeripath zeripath added this to the 1.15.0 milestone May 16, 2021
@zeripath
Copy link
Contributor Author

zeripath commented May 16, 2021

There are still significant slowdowns with subdirectories - but this is also improved - down to 9s

I think I've cracked this somewhat.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 16, 2021
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2021

Codecov Report

Merging #15891 (cc6d732) into main (36dce0e) will decrease coverage by 0.01%.
The diff coverage is 56.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #15891      +/-   ##
==========================================
- Coverage   44.10%   44.08%   -0.02%     
==========================================
  Files         682      682              
  Lines       82375    82521     +146     
==========================================
+ Hits        36328    36381      +53     
- Misses      40140    40212      +72     
- Partials     5907     5928      +21     
Impacted Files Coverage Δ
modules/git/pipeline/lfs_nogogit.go 0.00% <0.00%> (ø)
modules/indexer/code/bleve.go 68.22% <0.00%> (-0.59%) ⬇️
modules/indexer/code/elastic_search.go 1.64% <0.00%> (-0.02%) ⬇️
modules/git/repo_language_stats_nogogit.go 47.56% <40.00%> (+2.98%) ⬆️
modules/git/batch_reader.go 50.63% <51.80%> (-3.12%) ⬇️
modules/git/commit_info_nogogit.go 68.48% <71.87%> (+0.39%) ⬆️
modules/charset/charset.go 71.71% <0.00%> (-6.07%) ⬇️
modules/queue/queue_bytefifo.go 75.14% <0.00%> (-3.56%) ⬇️
modules/git/repo_base_nogogit.go 82.85% <0.00%> (-2.86%) ⬇️
modules/git/utils.go 62.50% <0.00%> (-2.78%) ⬇️
... and 11 more

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 3720503...cc6d732. Read the comment docs.

Finally we are faster than go-git on ports for the root directory

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Set the missing discard(1) in repo_language_stats.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@lunny
Copy link
Member

lunny commented May 31, 2021

Thanks for your effort. Unfortunately, haven't see an obvious increment of the speed of linux kernel repo.

I tested with linux kernel home page,
Before twice time are 37850ms and 39430ms
After twice time are 37796ms and 37421ms

my test env is macOS

@zeripath
Copy link
Contributor Author

@lunny can you double check that test repo has a commit-graph written?

@zeripath
Copy link
Contributor Author

zeripath commented May 31, 2021

(in .git/objects/info/commit-graph)

@zeripath
Copy link
Contributor Author

It's really weird as I am I get 2s for rendering git.git on my laptop.

Could you try:

https://github.com/zeripath/gitea/tree/use-os-pipe

This switches to use an os.Pipe with all the above changes in place.

@lunny
Copy link
Member

lunny commented May 31, 2021

I have generated that commit graph file, but from your patch, it seems you haven't read it?

@lunny
Copy link
Member

lunny commented May 31, 2021

@zeripath
Before generated commit-graph file
Main branch twice time are 32s and 34s
This PR twice time are 32s and 32s

After generated commit-graph file
Main branch twice time are 23s and 23s
This PR twice time are 23s and 23s

@zeripath
Copy link
Contributor Author

I'm not reading it directly - I'm relying on git reading it - the commit-graph would speed up the git rev-list --format=%T commitid -- treepath call - so having one should exclude that as a cause of the problem.

The main slow down in all of this code appears to be getting information out of and possibly to the git cat-file --batch pipes. Mostly this PR simply re-ordered things in an attempt to reduce the time spent filling the buffer and time spent locked in bufio.fill(...) etc. It was enough for my (quite fast) laptop but clearly not enough for yours and others.

Try the os.Pipes thing - if there's any improvement it would be good to know.

@zeripath
Copy link
Contributor Author

Ah I wonder if your systems are stdout buffering?

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

zeripath commented Jun 3, 2021

Closing in favour of #16059 which is faster

@zeripath zeripath closed this Jun 3, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. performance/bigrepo Performance Issues affecting Big Repositories performance/speed performance issues with slow downs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants