Use native git variants by default with go-git variants as build tag #13673

zeripath · 2020-11-22T17:29:54Z

This PR provides alternative git functions to go-git derived functions.

The reasoning is that go-git appears to have a number of significant memory issues and in any case large parts of our git infrastructure use git directly in any case.

zeripath · 2020-11-22T17:30:57Z

Current status of this PR:

Native non-go-git variants are built by default.
Add the build tag gogit to build with go-git alternatives.
The sqlite and postgres integration tests run with go-git variants on drone.
Using the TEST_TAGS environment variable during make to set the build tags at testing.

zeripath · 2020-11-22T17:37:13Z

As an example, prior to this PR running:

for i in $(seq 1 100);do curl "http://localhost/gitea/administrator/qtconnectivity/commit/61afd66f148af4a8d3e0cad12b7c9419f1122cbe" &; done

on the gitea would balloon in memory use. This no longer happens.

codecov-io · 2020-11-22T18:00:27Z

Codecov Report

Merging #13673 (6170fbd) into master (2791cc1) will decrease coverage by 0.04%.
The diff coverage is 26.20%.

@@            Coverage Diff             @@
##           master   #13673      +/-   ##
==========================================
- Coverage   42.25%   42.20%   -0.05%     
==========================================
  Files         697      700       +3     
  Lines       76596    76862     +266     
==========================================
+ Hits        32363    32437      +74     
- Misses      38919    39077     +158     
- Partials     5314     5348      +34

Impacted Files	Coverage Δ
modules/git/blob.go	`59.18% <ø> (ø)`
modules/git/commit.go	`53.47% <0.00%> (-0.14%)`	⬇️
modules/git/commit_reader.go	`0.00% <0.00%> (ø)`
modules/git/notes.go	`64.10% <ø> (ø)`
modules/git/parse.go	`62.50% <ø> (ø)`
modules/git/pipeline/lfs.go	`0.00% <0.00%> (ø)`
modules/git/repo.go	`45.17% <ø> (ø)`
modules/git/repo_blob.go	`100.00% <ø> (ø)`
modules/git/repo_branch.go	`76.66% <ø> (ø)`
modules/git/repo_commit.go	`50.91% <ø> (ø)`
... and 68 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 2791cc1...6170fbd. Read the comment docs.

lafriks · 2020-11-22T18:04:07Z

How does this compare if MADV_DONTNEED is set? that will be default in golang 1.16 if I'm not mistaken

zeripath · 2020-11-22T20:23:30Z

Post curl 100 times with nogogit

Post curl 100 times with GODEBUG=MADV_DONTNEED ./gitea web

lafriks · 2020-11-22T20:56:10Z

Can't really read much in these images, does that include memory used by git processes or only gitea? What about performance wise?

zeripath · 2020-11-22T23:17:13Z

This is Gitea's memory. If you zoom in you will see that nogogit Gitea takes up 99.51Mb following the run but gogit Gitea takes up 240Mb. If you run the curl task again gogit will continue to increase.

There's a memory leak somewhere. (And it's in the red tree to the left on the gogit tree)

Peak memory usage for Gitea is higher with go-git than git - although I'm not completely able to assert this.

Now this PR appears slightly slower in the getcommitpaths I will see if I can speed that up though and there's a bug I've noticed elsewhere.

lunny · 2020-11-23T03:14:55Z

How about to move go-git related files to a sub directory and git-shell related to another sub directory if we cannot make all operations to an interface?

zeripath · 2020-11-23T07:58:41Z

@lunny that's the intended endpoint of this pr - once I've got the bugs ironed out and a more efficient getcommitpaths

zeripath · 2020-11-23T19:21:34Z

OK so the changes are slightly ~~slower~~ FASTER in GetLastCommitForPaths:

Nogogit: 2020/11/28 11:36:53 Completed GET /administrator/git/ 0  in 3.467982691s
GoGit: 2020/11/23 18:14:42 Completed GET /administrator/git 0  in 4.160055868s

The algorithm here is different and simply involves checking every commit. This is clearly not going to be the fastest solution but that I've managed to make it faster than the current algorithm suggests that even faster systems are possible.

Managed to shave another 0.5 second off by avoiding more allocations.
Managed to shave off another 1 second due to avoiding converting between 20byte and 40byte SHAs unnecessarily.

modules/repository/cache.go

Signed-off-by: Andrew Thornton <[email protected]>

Makefile

Signed-off-by: Andrew Thornton <[email protected]>

6543 · 2020-12-17T11:21:06Z

@zeripath can you add one integration test for gogit?

EDIT: so we make sure both implementations work - and we can switch to one or another depending on the need ;)

6543

as i can tell this is a huge imprvement

If I missed a bug I'm sorry - but I think we shoul get it in

lunny · 2020-12-17T14:00:42Z

Let's merge this at first even if maybe there are some missed.

silverwind · 2020-12-17T14:30:47Z

.drone.yml

+      GOPROXY: https://goproxy.cn # proxy.golang.org is blocked in China, this proxy is not
+      GOSUMDB: sum.golang.org
+      TAGS: bindata gogit sqlite sqlite_unlock_notify
+


Sorry I'm late here but is it really necessary to run the linter twice here? Do TAGS really affect the linting result? I thought TAGS only affect the build.

I guess it's beyond me to understand how TAGS can affect linting (which should just be a static analysis of files). Care to explain? Does the go linter actually do runtime analysis as well?

golangci-lint has a --build-tags to support lint different tags.

Ok I guess I need to study golangci-lint more. I was under the impression that linters in general just do static analysis of file contents, but I guess this linter does more than that.

Thought I generally wonder if it's really necessary to do this double linting every single CI run. The go linting already takes around 3 minutes per run.

The issue is that the go-git variants require linting too.

Can't we just pass a list of files to the linters as argument? I think it should be possible.

There is a slight bug in the commit_reader introduced in go-gitea#13673 whereby commit messages which have a final unterminated line miss their final line. This PR fixes this. Signed-off-by: Andrew Thornton <[email protected]>

There is a slight bug in the commit_reader introduced in #13673 whereby commit messages which have a final unterminated line miss their final line. This PR fixes this. Signed-off-by: Andrew Thornton <[email protected]>

zeripath added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Nov 22, 2020

zeripath added this to the 1.14.0 milestone Nov 22, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 22, 2020

zeripath force-pushed the no-go-git branch from d293ab3 to aebde42 Compare November 23, 2020 18:12

zeripath mentioned this pull request Nov 27, 2020

Gitea goes reproducibly "out of memory" on a big repository #13461

Closed

6 tasks

lunny reviewed Nov 28, 2020

View reviewed changes

modules/repository/cache.go Outdated Show resolved Hide resolved

zeripath added 13 commits November 29, 2020 09:19

Move last commit cache back into modules/git

c76d2ad

Signed-off-by: Andrew Thornton <[email protected]>

Remove go-git from the interface for last commit cache

fd69b5b

Signed-off-by: Andrew Thornton <[email protected]>

move cacheref to last_commit_cache

2013955

Signed-off-by: Andrew Thornton <[email protected]>

Remove go-git from routers/private/hook

58f0579

Signed-off-by: Andrew Thornton <[email protected]>

Move FindLFSFiles to pipeline

d45a18f

Signed-off-by: Andrew Thornton <[email protected]>

Make no-go-git variants

164a02e

Signed-off-by: Andrew Thornton <[email protected]>

Submodule RefID

764948f

Signed-off-by: Andrew Thornton <[email protected]>

fix issue with GetCommitsInfo

d8d498d

Signed-off-by: Andrew Thornton <[email protected]>

fix GetLastCommitForPaths

abddc4e

Signed-off-by: Andrew Thornton <[email protected]>

Improve efficiency

1e396b7

Signed-off-by: Andrew Thornton <[email protected]>

More efficiency

4b35e94

Signed-off-by: Andrew Thornton <[email protected]>

even faster

95d6652

Signed-off-by: Andrew Thornton <[email protected]>

Reduce duplication

20632e2

zeripath added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed pr/wip This PR is not ready for review labels Dec 10, 2020

zeripath changed the title ~~WIP: Provide alternatives to go-git~~ Switch to use native git variants by default but provide go-git variants as build tag Dec 10, 2020

zeripath changed the title ~~Switch to use native git variants by default but provide go-git variants as build tag~~ Use native git variants by default but provide go-git variants as build tag Dec 10, 2020

zeripath changed the title ~~Use native git variants by default but provide go-git variants as build tag~~ Use native git variants by default with go-git variants as build tag Dec 10, 2020

zeripath and others added 3 commits December 10, 2020 16:58

Merge branch 'master' into no-go-git

531da0f

Merge branch 'master' into no-go-git

03c8cd6

Merge branch 'master' into no-go-git

4633679

6543 reviewed Dec 11, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

as per @6543

320adb4

Signed-off-by: Andrew Thornton <[email protected]>

zeripath mentioned this pull request Dec 12, 2020

Gitea depletes Ram + CPU #12417

Closed

zeripath and others added 2 commits December 14, 2020 19:56

Merge branch 'master' into no-go-git

5582fba

Merge branch 'master' into no-go-git

39c8dfe

Merge branch 'master' into no-go-git

29a5951

6543 approved these changes Dec 17, 2020

View reviewed changes

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 Dec 17, 2020

lunny approved these changes Dec 17, 2020

View reviewed changes

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 Dec 17, 2020

lunny merged commit 511f613 into go-gitea:master Dec 17, 2020

zeripath deleted the no-go-git branch December 17, 2020 14:12

silverwind reviewed Dec 17, 2020

View reviewed changes

zeripath mentioned this pull request Dec 17, 2020

Fix bug in commit verification introduced in #13673 #14040

Merged

skyline75489 mentioned this pull request Dec 31, 2020

Replace 'show-ref' command with 'branch' and 'tag' #14201

Closed

go-gitea locked and limited conversation to collaborators Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use native git variants by default with go-git variants as build tag #13673

Use native git variants by default with go-git variants as build tag #13673

zeripath commented Nov 22, 2020 •

edited

Loading

zeripath commented Nov 22, 2020 •

edited

Loading

zeripath commented Nov 22, 2020

codecov-io commented Nov 22, 2020 •

edited

Loading

lafriks commented Nov 22, 2020

zeripath commented Nov 22, 2020

lafriks commented Nov 22, 2020

zeripath commented Nov 22, 2020

lunny commented Nov 23, 2020

zeripath commented Nov 23, 2020

zeripath commented Nov 23, 2020 •

edited

Loading

6543 commented Dec 17, 2020 •

edited

Loading

6543 left a comment

lunny commented Dec 17, 2020

silverwind Dec 17, 2020

zeripath Dec 17, 2020

silverwind Dec 17, 2020

lunny Dec 17, 2020

silverwind Dec 17, 2020

silverwind Dec 17, 2020 •

edited

Loading

zeripath Dec 17, 2020

silverwind Dec 17, 2020 •

edited

Loading

Use native git variants by default with go-git variants as build tag #13673

Use native git variants by default with go-git variants as build tag #13673

Conversation

zeripath commented Nov 22, 2020 • edited Loading

zeripath commented Nov 22, 2020 • edited Loading

zeripath commented Nov 22, 2020

codecov-io commented Nov 22, 2020 • edited Loading

Codecov Report

lafriks commented Nov 22, 2020

zeripath commented Nov 22, 2020

Post curl 100 times with nogogit

Post curl 100 times with GODEBUG=MADV_DONTNEED ./gitea web

lafriks commented Nov 22, 2020

zeripath commented Nov 22, 2020

lunny commented Nov 23, 2020

zeripath commented Nov 23, 2020

zeripath commented Nov 23, 2020 • edited Loading

6543 commented Dec 17, 2020 • edited Loading

6543 left a comment

Choose a reason for hiding this comment

lunny commented Dec 17, 2020

silverwind Dec 17, 2020

Choose a reason for hiding this comment

zeripath Dec 17, 2020

Choose a reason for hiding this comment

silverwind Dec 17, 2020

Choose a reason for hiding this comment

lunny Dec 17, 2020

Choose a reason for hiding this comment

silverwind Dec 17, 2020

Choose a reason for hiding this comment

silverwind Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

zeripath Dec 17, 2020

Choose a reason for hiding this comment

silverwind Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

zeripath commented Nov 22, 2020 •

edited

Loading

zeripath commented Nov 22, 2020 •

edited

Loading

codecov-io commented Nov 22, 2020 •

edited

Loading

zeripath commented Nov 23, 2020 •

edited

Loading

6543 commented Dec 17, 2020 •

edited

Loading

silverwind Dec 17, 2020 •

edited

Loading

silverwind Dec 17, 2020 •

edited

Loading