Improve the performance of pull-request merging #641

typeless · 2017-01-11T02:55:14Z

Merging pull-request without checking out a working tree

Inspired by this.

appleboy · 2017-01-11T06:02:04Z

Lint error, please fix lint error from the following command.

make lint

typeless · 2017-01-11T06:05:40Z

@appleboy I have fixed it in the last commit and tested with make lint for my local repo.

appleboy · 2017-01-11T06:18:08Z

@typeless Where is your last commit? Still fail: https://drone.gitea.io/go-gitea/gitea/1072

typeless · 2017-01-11T06:20:41Z

@appleboy 0e58fa9

Am I missing something?

appleboy · 2017-01-11T06:24:57Z

@typeless I don't know why the drone show error on this commit. Please recommit to test it again.

typeless · 2017-01-11T06:32:03Z

@appleboy Alright

Bwko · 2017-01-11T19:13:19Z

@typeless Do you have any benchmarks? I'm using the latest master(without this PR) and merging a repo of 1.7gig succeeds in about 5 secs 😯

typeless · 2017-01-12T02:26:05Z

@Bwko For the record, the large repo that troubled me is about 13G the size of .git.

$ time git clone ../git-repositories/foo/my-large-repo.git/
Cloning into 'my-large-repo'...
done.
Checking out files: 100% (530225/530225), done.

real    5m58.914s
user    1m23.368s
sys     0m31.164s

The large repo is in fact an Android board support package which is a mixture of the Google official code and a vendor-tailored distribution.

And for Linux kernel, which is about 2G (bare repo)

$ time git clone gitea-repositories/foo/linux.git
Cloning into 'linux'...
done.
Checking out files: 100% (56233/56233), done.

real    0m6.242s
user    0m4.680s
sys     0m1.216s

Very close to what you have benchmarked.

The both clones were done within the same filesystem, by the way.

Edit:

For downloading the official android platform code:

$ curl https://storage.googleapis.com/git-repo-downloads/repo > ~/bin/repo
$ chmod a+x ~/bin/repo

Then use the repo tool to download the code

$ mkdir large-repo
$ cd large-repo
$ repo init -u https://android.googlesource.com/platform/manifest -b android-4.0.1_r1
$ repo sync -j32

The -j32 is similar to make -j, assuming your machine has 32 cores.
Be aware that this is a bandwidth killer.

It'd better to kill the .repo and the sparsely populated .git directories before pushing to Gitea.

$ rm -rf .repo
$ find -name .git -type d -exec rm -rf {} +

References: https://source.android.com/source/downloading.html

Bwko · 2017-01-12T15:42:51Z

@typeless That looks really promising 👍
Found a bug, on your forked repo click new file, fill that file with contents use the Create a new branch for this commit and start a pull request. option.
An then create a PR, merging gives the following error:

[...routers/repo/pull.go:420 MergePullRequest()] [E] Merge: git commit-tree a9405d5ce9265ff2b5647b1f77ade6f44b6c419e -p master -p refs/pull/5/head: 
*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

typeless · 2017-01-13T15:21:18Z

Hasn't been tested yet

Bwko · 2017-01-25T20:20:06Z

@typeless If you repeat the steps from my previous comment you'll get: fatal: invalid date format: RFC9999

typeless · 2017-01-26T02:16:51Z

@Bwko Thanks for your help.
I don't know if there is a better way but I would like to add an automated testing facility like how #741 does. I don't like to re-create those fake repos and PRs from the ground up again and again 😆

lunny · 2017-02-11T07:30:55Z

conflicted

lunny · 2017-02-14T06:23:44Z

@typeless any update?

typeless · 2017-02-21T09:07:25Z

@Bwko @metalmatze
Rebased and manually resolved conflicts. PTAL.

P.S. The relavent commits: bf647ce and d100615

lunny · 2017-02-21T09:16:00Z

It's ready for review now?

typeless · 2017-02-21T09:26:17Z

@lunny I'd like to wait for #741 getting merged so that I can add an end-to-end test for this. For now, I just want to ping the related authors about what the PR does to their commits.

lunny · 2017-03-06T02:40:38Z

models/pull.go

+		fmt.Sprintf("PullRequest.Merge (git write-tree): %s", baseRepoPath),
+		[]string{"GIT_DIR=" + baseRepoPath, "GIT_INDEX_FILE=" + indexTmpPath},
+		"git", "write-tree"); err != nil {
+		return fmt.Errorf("git write-tree: %s", stderr)


err also should be include the error string.

lunny · 2017-03-06T11:40:37Z

So let's move this to v1.2 since we have no more time to review this one after #741 merged.

appleboy · 2017-06-16T02:43:04Z

@typeless Great work. build success. https://drone.gitea.io/go-gitea/gitea/3579

lunny · 2017-06-16T02:49:02Z

Any performance tests?

typeless · 2017-06-16T02:52:14Z

@lunny I was considering about it. But for showing any significance we need to prepare a large repo in the integration framework. I have no solid idea yet about how it should be added.

lunny · 2017-06-16T05:05:19Z

maybe clone linux repo on the tests?

typeless · 2017-06-16T06:10:36Z

@lunny maybe we can use the migration feature to clone a Linux repository for testing. But I am afraid of getting banned if we suck the bandwidth of the server too heavily/frequently. 😄

lafriks · 2017-06-16T08:20:42Z

I don't think performance tests should be done in drone

sapk · 2017-06-16T09:10:22Z

We could still have a benchmark suite at least to validate any performance PR. Those should not be run on drone even more if they are network/cpu/memory intensive.

beddari · 2017-09-15T11:27:03Z

Any status updates?

codecov-io · 2017-09-18T12:14:54Z

Codecov Report

Merging #641 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
- Coverage   27.32%   27.25%   -0.07%     
==========================================
  Files          86       86              
  Lines       17135    17177      +42     
==========================================
  Hits         4682     4682              
- Misses      11775    11817      +42     
  Partials      678      678

Impacted Files	Coverage Δ
models/pull.go	`19.53% <0%> (-1.05%)`	⬇️

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 ca68a75...c5babc2. Read the comment docs.

lunny · 2017-09-18T15:06:09Z

@typeless Is it possible to add a performance test?

typeless · 2017-09-19T01:31:46Z

I was thinking about it but not sure about how it should be done in an idiomatic way. Also the merge operation is stateful and not easily repeatable in a typical for i:=0; i<b.N; i++ {} loop. Any ideas? Maybe I am missing something.

lunny · 2017-09-19T04:36:07Z

maybe you can see https://github.com/go-gitea/gitea/pull/2519/files#diff-dbb0d04b955af59a55a562dc9e8c9912R39

bkcsoft · 2017-09-19T21:34:06Z

models/pull.go

+			"GIT_AUTHOR_EMAIL=" + headCommit.Author.Email,
+			"GIT_AUTHOR_DATE=" + headCommit.Author.When.Format("Mon, 02 Jan 2006 15:04:05 -0700"),
+			"GIT_COMMITTER_NAME=" + doer.NewGitSig().Name,
+			"GIT_COMMITTER_EMAIL=" + doer.NewGitSig().Email,


Does this hide the Email if the commiter has that set in their config?

Yes. https://sourcegraph.com/github.com/go-gitea/gitea@6c6533fb31bbc82e7907b1dc233212045849569b/-/blob/models/user.go#L170

typeless · 2017-09-22T11:56:12Z

A benchmark has been added.

The old way of PR merging has to checkout the working tree of the repository. This change make it possible to do the merge by manipulating the index only.

lafriks · 2017-09-22T13:35:18Z

models/pull.go

-		"git", "clone", baseGitRepo.Path, tmpBasePath); err != nil {
-		return fmt.Errorf("git clone: %s", stderr)
+	// A temporary Git working tree for git-merge-index and git-merge-one-file.
+	workTreeTmpPath, err := ioutil.TempDir("", "gitea-merge-")


Is not there missing +pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())?

~~ioutil.TempDir will generate a suffix automatically. See https://golang.org/src/io/ioutil/tempfile.go?s=2138:2195#L66~~
Sorry, I see what you mean now. I'll fix it later.

Is not there missing +pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())?

This is actually correct despite a bit of inconsistent. I will clean this up a little bit.

The path indexTmpPath and workTreeTmpPath conrespond to GIT_INDEX_FILE and GIT_WORK_TREE respectively. I looked at this in a hurry last friday and got confused as well. 😆

typeless · 2017-10-05T02:46:08Z

This is actually not a comprehensive approach that covers all cases where git-merge can handle. So I think the change doesn't warrant the risks. The change should be made into the git tool or delegated to `libgit2' or src-d/go-git.

lunny added this to the 1.1.0 milestone Jan 11, 2017

lunny added the type/enhancement An improvement of existing functionality label Jan 11, 2017

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

bkcsoft added the pr/wip This PR is not ready for review label Jan 21, 2017

typeless force-pushed the speedup-pr-merge branch 3 times, most recently from 4703f84 to cd29b89 Compare February 21, 2017 09:04

lunny requested changes Mar 6, 2017

View reviewed changes

lunny modified the milestones: 1.2.0, 1.1.0 Mar 6, 2017

typeless force-pushed the speedup-pr-merge branch from cd29b89 to 3019cf8 Compare March 8, 2017 02:14

lunny approved these changes Mar 8, 2017

View reviewed changes

typeless changed the title ~~Improve the performance of pull-request merging~~ [WIP] Improve the performance of pull-request merging Jun 19, 2017

typeless force-pushed the speedup-pr-merge branch from 3f2df09 to ea2879b Compare June 19, 2017 09:00

typeless changed the title ~~[WIP] Improve the performance of pull-request merging~~ Improve the performance of pull-request merging Jun 19, 2017

typeless force-pushed the speedup-pr-merge branch 2 times, most recently from 7051e60 to 7f92154 Compare June 23, 2017 08:51

typeless force-pushed the speedup-pr-merge branch from 7f92154 to 8962e92 Compare September 18, 2017 11:57

bkcsoft reviewed Sep 19, 2017

View reviewed changes

typeless force-pushed the speedup-pr-merge branch from 8962e92 to e49bf7d Compare September 20, 2017 01:48

typeless added 2 commits September 22, 2017 19:58

Speedup pull-request mergeing

8179ef8

The old way of PR merging has to checkout the working tree of the repository. This change make it possible to do the merge by manipulating the index only.

Add a straightforward benchmark

c5babc2

typeless force-pushed the speedup-pr-merge branch from 812f160 to c5babc2 Compare September 22, 2017 11:58

lafriks requested changes Sep 22, 2017

View reviewed changes

typeless closed this Oct 5, 2017

typeless mentioned this pull request Dec 28, 2018

Merging PRs takes very long time for large repository #601

Closed

6 tasks

typeless deleted the speedup-pr-merge branch April 3, 2019 05:01

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

Improve the performance of pull-request merging #641

Improve the performance of pull-request merging #641

Conversation

typeless commented Jan 11, 2017 • edited Loading

appleboy commented Jan 11, 2017

typeless commented Jan 11, 2017

appleboy commented Jan 11, 2017

typeless commented Jan 11, 2017

appleboy commented Jan 11, 2017

typeless commented Jan 11, 2017

Bwko commented Jan 11, 2017 • edited Loading

typeless commented Jan 12, 2017 • edited Loading

Edit:

Bwko commented Jan 12, 2017

typeless commented Jan 13, 2017

Bwko commented Jan 25, 2017

typeless commented Jan 26, 2017

lunny commented Feb 11, 2017

lunny commented Feb 14, 2017

typeless commented Feb 21, 2017 • edited Loading

lunny commented Feb 21, 2017

typeless commented Feb 21, 2017

lunny Mar 6, 2017

Choose a reason for hiding this comment

lunny commented Mar 6, 2017

appleboy commented Jun 16, 2017

lunny commented Jun 16, 2017

typeless commented Jun 16, 2017

lunny commented Jun 16, 2017

typeless commented Jun 16, 2017

lafriks commented Jun 16, 2017

sapk commented Jun 16, 2017

beddari commented Sep 15, 2017

codecov-io commented Sep 18, 2017 • edited Loading

Codecov Report

lunny commented Sep 18, 2017

typeless commented Sep 19, 2017

lunny commented Sep 19, 2017

bkcsoft Sep 19, 2017

Choose a reason for hiding this comment

typeless Sep 20, 2017

Choose a reason for hiding this comment

typeless commented Sep 22, 2017

lafriks Sep 22, 2017

Choose a reason for hiding this comment

typeless Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

typeless Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

typeless commented Oct 5, 2017

typeless commented Jan 11, 2017 •

edited

Loading

Bwko commented Jan 11, 2017 •

edited

Loading

typeless commented Jan 12, 2017 •

edited

Loading

typeless commented Feb 21, 2017 •

edited

Loading

codecov-io commented Sep 18, 2017 •

edited

Loading

typeless Sep 22, 2017 •

edited

Loading

typeless Sep 25, 2017 •

edited

Loading