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

Fix one performance/correctness regression in #6478 found on Rails repository. #6686

Merged
merged 5 commits into from
Apr 21, 2019

Conversation

filipnavara
Copy link
Contributor

Fix flaw in the commit history lookup that caused unnecessary traversal when the repository contains a lot of merge commits.

Also return the merge commit as the changed one if the file or directory was changed as part of the merge, eg. through conflict resolution. This matches behavior observed on GitHub.

Note that this doesn't fix the listing performance regression just yet. There's still problem when listing directories containing very old files. In that case the commit history has to be traversed all the way back to when the file was last modified.

when the repository contains a lot of merge commits.

Also return the merge commit as the changed one if the file or
directory was changed as part of the merge, eg. through conflict
resolution.

Signed-off-by: Filip Navara <filip.navara@gmail.com>
@codecov-io
Copy link

codecov-io commented Apr 19, 2019

Codecov Report

Merging #6686 into master will decrease coverage by <.01%.
The diff coverage is 84.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6686      +/-   ##
==========================================
- Coverage    40.8%   40.79%   -0.01%     
==========================================
  Files         421      421              
  Lines       57571    57570       -1     
==========================================
- Hits        23494    23488       -6     
- Misses      30951    30955       +4     
- Partials     3126     3127       +1
Impacted Files Coverage Δ
modules/git/commit_info.go 69.09% <84.37%> (+1.62%) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
modules/log/event.go 64.46% <0%> (-1.53%) ⬇️
models/repo_list.go 66.84% <0%> (-1.06%) ⬇️

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 04ff3dd...e458a4f. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 19, 2019
in a merge commit follow only the first parent.
@lunny
Copy link
Member

lunny commented Apr 20, 2019

The rails home page will load from 8s to 6.4s on my macOS after merge this PR.

@filipnavara
Copy link
Contributor Author

@lunny Thanks for trying it. That corresponds with numbers from my local testing.

@filipnavara
Copy link
Contributor Author

@lunny Can you also try the branch at https://github.com/filipnavara/gitea/tree/go-git-perf-fixes? It contains this PR + some optimizations to go-git. On my machine that's enough to push it to around ~4.5 seconds. There's still room for some improvement but I wanted to address the low-hanging fruit first.

For the root directory it's now similar to the performance I got before #6478. That may not necessarily be good enough since invoking git is super slow on Windows by comparison. For subdirectories it's significantly faster.

@filipnavara filipnavara marked this pull request as ready for review April 20, 2019 15:55
@lafriks lafriks added the issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself label Apr 20, 2019
@lafriks lafriks added this to the 1.9.0 milestone Apr 20, 2019
@lafriks lafriks added type/enhancement An improvement of existing functionality and removed issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself labels Apr 20, 2019
@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 Apr 20, 2019
@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 Apr 20, 2019
@lunny
Copy link
Member

lunny commented Apr 21, 2019

@filipnavara Do you want to update go-git recent version on this PR?

@filipnavara
Copy link
Contributor Author

filipnavara commented Apr 21, 2019

@lunny not worth it at the moment. I have some go-git PRs waiting in the pipeline that have to be merged first.

@lunny lunny merged commit b83114f into go-gitea:master Apr 21, 2019
@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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants