Faster commit lookup #91

ethantkoenig · 2017-11-19T01:55:38Z

A faster implementation of GetCommitsInfo, particularly for repositories with many files. Addresses go-gitea/gitea#491 and go-gitea/gitea#502.

A revival of the failed #53 (which was reverted by #73).

BENCHMARKS

Old implementations:

BenchmarkEntries_GetCommitsInfo/gitea-4         	     200	  89001816 ns/op
BenchmarkEntries_GetCommitsInfo/manyfiles-4     	       1	47869780305 ns/op
BenchmarkEntries_GetCommitsInfo/moby-4          	     100	 174120584 ns/op
BenchmarkEntries_GetCommitsInfo/go-4            	      20	 727309260 ns/op
BenchmarkEntries_GetCommitsInfo/linux-4         	      20	 853282140 ns/op

New implementation:

BenchmarkEntries_GetCommitsInfo/gitea-4         	     200	  79968242 ns/op
BenchmarkEntries_GetCommitsInfo/manyfiles-4     	      50	 261940680 ns/op
BenchmarkEntries_GetCommitsInfo/moby-4          	     100	 154584374 ns/op
BenchmarkEntries_GetCommitsInfo/go-4            	      20	 715422795 ns/op
BenchmarkEntries_GetCommitsInfo/linux-4         	      20	 833090406 ns/op

In summary, we see a >150x improvement in repos with many files, and mild improvements in other repos.

IMPLEMENTATION DETAILS:
Runs targeted and untargeted searches in parallel. The untargeted search (1 thread) sifts through the output of a git log --name-status -c -- <tree-path> command, seeing which files the most recent commits have affected. The targeted search (multiple threads) runs git rev-list -1 HEAD -- <entry> commands to get the latest commit for specific file that have not been found by the untargeted search. The targeted search is similar to the old implementation, with a few enhancements (e.g. uses git rev-list instead of git log).

If there are only a few remaining entries, then the untargeted search stops to allow the targeted-search threads to finish without any interference (in my tests, this interference had a non-negligible impact on performance).

TESTING:
I have not been able to test this new implementation on a Windows machine. If someone could test the new implementation on Windows (to check for forward-slash vs. backslash bugs), that would be great.

bkcsoft · 2017-11-23T06:15:34Z

commit_info.go

+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
+	defer cancel()
+
+	args := []string{"log", getCommitsInfoPretty, "--name-status", "-c"}


Will this command ever be faster than the targetedSearch ?

Yes, it is faster for repos with many files (which would otherwise require many calls to targetedSearch vs. one untargeted search). See the benchmark results for the manyfiles repo.

Maybe have 2 benchmarks, one for each type of search, just to convince me 😛

@bkcsoft I believe the existing benchmarks are telling on their own. Using only the targetedSearch is essentially the same as the old implementation (aside from a few small tweaks). The only explanation for the very large (>150x) performance improvement in the manyfiles repo between the old and new implementations is that running an "untargeted" search in parallel is helpful (in this case, very helpful).

The advantage of using a "untargeted" approach in repos with many files is also reflected in the benchmarks for #53 (which solely used an untargeted approach)

bkcsoft · 2017-11-23T06:18:34Z

commit_info.go

+		}
+		if line[0] >= 'A' && line[0] <= 'X' { // a file was changed by the current commit
+			tabIndex := strings.IndexByte(line, '\t')
+			if tabIndex < 1 {


MM is valid for Modified files in a Merge commit 🤔

c757efaaa0267ec823ddc0858cf6ec930e93a074 1510393589 Merge branch 'gitlab-git-f7537ce03a29b' into 'master' MM CHANGELOG.md

Then you have moves like this: (no clue what the digits are for 🤔 )

R088 internal/server/auth.go internal/server/auth/auth.go R070 internal/ref/helper.go internal/git/ref.go R055 ruby/vendor/gitlab_git/lib/gitlab/git/committer.rb ruby/vendor/gitlab_git/lib/gitlab/git/user.rb

Similarity-score apparently:

<X><score> The rename or copy score (denoting the percentage of similarity between the source and target of the move or copy). For example "R100" or "C75".

@bkcsoft The code will handle MM correctly (as far as I can tell, since the line still begins with an upper case character). I'll update the code to correctly handle R... and C... (where there will be two filepaths, and we should look at the second one)

Ooh, I read tabIndex > 1 which would fail. But it's tabIndex < 1 😅

lafriks · 2017-12-01T07:07:19Z

commit_info.go

+func (state *getCommitsInfoState) numRemainingEntries() int {
+	state.lock.Lock()
+	numRemaining := len(state.entries) - len(state.commits)
+	state.lock.Unlock()


It would be better to move this just after lock and use defer state.lock.Unlock()

I intentionally did not use defer for performance. This function will be called many times, so small changes can have a meaningful impact on overall performance. See golang/go#6980

I will go on that with @ethantkoenig defer add some overhead

@lafriks friendly ping

lafriks · 2017-12-01T07:07:59Z

commit_info.go

+		state.targetedPaths[entryPath] = struct{}{}
+		break
+	}
+	state.lock.Unlock()


Same here - move this just after lock and use defer state.lock.Unlock()

Same here since no return in between.

lafriks · 2017-12-01T07:08:43Z

commit_info.go

+		state.commits[entryPath] = commit
+		updated = true
+	}
+	state.lock.Unlock()


same here with defer

sapk · 2017-12-01T19:40:42Z

commit_info.go

+func (state *getCommitsInfoState) numRemainingEntries() int {
+	state.lock.Lock()
+	numRemaining := len(state.entries) - len(state.commits)
+	state.lock.Unlock()


I will go on that with @ethantkoenig defer add some overhead

sapk · 2017-12-01T19:41:37Z

commit_info.go

+		state.targetedPaths[entryPath] = struct{}{}
+		break
+	}
+	state.lock.Unlock()


Same here since no return in between.

sapk · 2017-12-01T19:42:16Z

LGTM

lafriks · 2017-12-09T19:11:45Z

@ethantkoenig Is it really worth risking locking up if unexpected error occurs? Can you retest test how big performance penalty in this case it is with defer?

ethantkoenig · 2017-12-10T01:02:40Z

@lafriks Overhead of using defer was less than I expected (essentially neglible), so I've switched to using defer.

lafriks · 2017-12-10T10:01:55Z

LGTM

lafriks · 2017-12-10T10:02:57Z

@ethantkoenig please update git dep in gitea repository

ethantkoenig force-pushed the fast_commits branch 2 times, most recently from 141bc21 to f366cad Compare November 19, 2017 03:03

bkcsoft reviewed Nov 23, 2017

View reviewed changes

bkcsoft suggested changes Nov 23, 2017

View reviewed changes

lafriks suggested changes Dec 1, 2017

View reviewed changes

bkcsoft approved these changes Dec 1, 2017

View reviewed changes

sapk approved these changes Dec 1, 2017

View reviewed changes

tboerger added the lgtm/need 1 label Dec 1, 2017

ethantkoenig added 4 commits December 9, 2017 17:04

Faster commit lookup

489f7b0

Fix copy/rename bug

e39b29f

Comment

2706f5e

Use defer

acb3e0f

ethantkoenig force-pushed the fast_commits branch from f39f745 to acb3e0f Compare December 10, 2017 01:04

lafriks approved these changes Dec 10, 2017

View reviewed changes

tboerger added lgtm/done and removed lgtm/need 1 labels Dec 10, 2017

lafriks merged commit 02eccf8 into go-gitea:master Dec 10, 2017

ethantkoenig deleted the fast_commits branch December 10, 2017 16:56

This was referenced Dec 10, 2017

Update code.gitea.io/git go-gitea/gitea#3137

Merged

Repository home page show error file last commit message. go-gitea/gitea#3202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster commit lookup #91

Faster commit lookup #91

ethantkoenig commented Nov 19, 2017 •

edited

Loading

bkcsoft Nov 23, 2017

ethantkoenig Nov 23, 2017 •

edited

Loading

bkcsoft Nov 23, 2017 •

edited

Loading

ethantkoenig Nov 23, 2017 •

edited

Loading

bkcsoft Nov 23, 2017

bkcsoft Nov 23, 2017 •

edited

Loading

bkcsoft Nov 23, 2017

ethantkoenig Nov 23, 2017

ethantkoenig Nov 23, 2017

bkcsoft Dec 1, 2017

lafriks Dec 1, 2017

ethantkoenig Dec 1, 2017 •

edited

Loading

sapk Dec 1, 2017

ethantkoenig Dec 8, 2017

lafriks Dec 1, 2017

sapk Dec 1, 2017

lafriks Dec 1, 2017

sapk Dec 1, 2017

sapk Dec 1, 2017

sapk commented Dec 1, 2017

lafriks commented Dec 9, 2017

ethantkoenig commented Dec 10, 2017

lafriks commented Dec 10, 2017

lafriks commented Dec 10, 2017

Faster commit lookup #91

Faster commit lookup #91

Conversation

ethantkoenig commented Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

ethantkoenig Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

bkcsoft Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

ethantkoenig Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Dec 1, 2017

lafriks commented Dec 9, 2017

ethantkoenig commented Dec 10, 2017

lafriks commented Dec 10, 2017

lafriks commented Dec 10, 2017

ethantkoenig commented Nov 19, 2017 •

edited

Loading

ethantkoenig Nov 23, 2017 •

edited

Loading

bkcsoft Nov 23, 2017 •

edited

Loading

ethantkoenig Nov 23, 2017 •

edited

Loading

bkcsoft Nov 23, 2017 •

edited

Loading

ethantkoenig Dec 1, 2017 •

edited

Loading