perf: improve the way we check if refs are not pointing to commits

Fixes #775
Fixes #777

Before, each time we got a new reference, we were checking if it
pointed to a commit. This incurred in a performance penalty and
an increase of 2x in memory usage and 4x in allocations, as more
objects were being read from the repository.

Instead, now we use the already present resolveCommit to check it
the moment we get the object, since we have to get it anyway.
This way, we have the exact same behaviour without having any
performance penalty or memory usage increase.

Signed-off-by: Miguel Molina <[email protected]>

Author: erizocosmico

commits.go

@@ -229,46 +229,50 @@ func (i *commitIter) loadNextRef() (err error) {
 			return err
 		}
 
-		ignored, err := isIgnoredReference(i.repo.Repository, i.ref)
-		if err != nil {
-			if i.skipGitErrors {
-				continue
-			}
-
-			return err
-		}
-
-		if ignored {
+		if isIgnoredReference(i.ref) {
 			continue
 		}
 
-		i.queue = append(i.queue, i.ref.Hash())
-
 		return nil
 	}
 }
 
 func (i *commitIter) Next() (*object.Commit, error) {
 	for {
+		var commit *object.Commit
+		var err error
+
 		if i.ref == nil {
 			if err := i.loadNextRef(); err != nil {
 				return nil, err
 			}
-		}
 
-		if len(i.queue) == 0 {
-			i.ref = nil
-			continue
-		}
+			if _, ok := i.seen[i.ref.Hash()]; ok {
+				continue
+			}
+			i.seen[i.ref.Hash()] = struct{}{}
 
-		hash := i.queue[0]
-		i.queue = i.queue[1:]
-		if _, ok := i.seen[hash]; ok {
-			continue
+			commit, err = resolveCommit(i.repo, i.ref.Hash())
+			if errInvalidCommit.Is(err) {
+				i.ref = nil
+				continue
+			}
+		} else {
+			if len(i.queue) == 0 {
+				i.ref = nil
+				continue
+			}
+
+			hash := i.queue[0]
+			i.queue = i.queue[1:]
+			if _, ok := i.seen[hash]; ok {
+				continue
+			}
+			i.seen[hash] = struct{}{}
+
+			commit, err = i.repo.CommitObject(hash)
 		}
-		i.seen[hash] = struct{}{}
 
-		commit, err := i.repo.CommitObject(hash)
 		if err != nil {
 			if i.skipGitErrors {
 				continue

integration_test.go

@@ -594,7 +594,7 @@ func TestMissingHeadRefs(t *testing.T) {
 
 	rows, err := sql.RowIterToRows(iter)
 	require.NoError(err)
-	require.Len(rows, 54)
+	require.Len(rows, 56)
 }
 
 func BenchmarkQueries(b *testing.B) {

ref_commits.go

@@ -305,32 +305,19 @@ func (i *refCommitsRowIter) next() (sql.Row, error) {
 					i.repo.Close()
 					return nil, err
 				}
-
-				ignored, err := isIgnoredReference(i.repo.Repository, ref)
-				if err != nil {
-					if i.skipGitErrors {
-						continue
-					}
-
-					return nil, err
-				}
-
-				if ignored {
-					continue
-				}
 			} else {
 				ref = plumbing.NewHashReference(plumbing.ReferenceName("HEAD"), i.head.Hash())
 				i.head = nil
 			}
 
 			i.ref = ref
-			if !i.shouldVisitRef(ref) {
+			if !i.shouldVisitRef(ref) || isIgnoredReference(ref) {
 				continue
 			}
 
 			commit, err := resolveCommit(i.repo, ref.Hash())
 			if err != nil {
-				if i.skipGitErrors {
+				if errInvalidCommit.Is(err) || i.skipGitErrors {
 					continue
 				}
 

references.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"strings"
 
-	git "gopkg.in/src-d/go-git.v4"
 	"gopkg.in/src-d/go-mysql-server.v0/sql"
 
 	"gopkg.in/src-d/go-git.v4/plumbing"
@@ -300,16 +299,7 @@ func (i *refRowIter) next() (sql.Row, error) {
 			return nil, err
 		}
 
-		ignored, err := isIgnoredReference(i.repo.Repository, o)
-		if err != nil {
-			if i.skipGitErrors {
-				continue
-			}
-
-			return nil, err
-		}
-
-		if ignored {
+		if isIgnoredReference(o) {
 			continue
 		}
 
@@ -351,15 +341,6 @@ func referenceToRow(repositoryID string, c *plumbing.Reference) sql.Row {
 	)
 }
 
-func isIgnoredReference(repo *git.Repository, ref *plumbing.Reference) (bool, error) {
-	if ref.Type() != plumbing.HashReference {
-		return true, nil
-	}
-
-	obj, err := repo.Object(plumbing.AnyObject, ref.Hash())
-	if err != nil {
-		return false, err
-	}
-
-	return obj.Type() != plumbing.CommitObject, nil
+func isIgnoredReference(r *plumbing.Reference) bool {
+	return r.Type() != plumbing.HashReference
 }

squash_iterator.go

@@ -489,16 +489,7 @@ func (i *squashRefIter) Advance() error {
 			}
 		}
 
-		ignored, err := isIgnoredReference(i.repo.Repository, ref)
-		if err != nil {
-			if i.skipGitErrors {
-				continue
-			}
-
-			return err
-		}
-
-		if ignored {
+		if isIgnoredReference(ref) {
 			continue
 		}
 
@@ -743,16 +734,7 @@ func (i *squashRepoRefsIter) Advance() error {
 			}
 		}
 
-		ignored, err := isIgnoredReference(i.repos.Repository().Repository, ref)
-		if err != nil {
-			if i.skipGitErrors {
-				continue
-			}
-
-			return err
-		}
-
-		if ignored {
+		if isIgnoredReference(ref) {
 			continue
 		}
 
@@ -890,16 +872,7 @@ func (i *squashRemoteRefsIter) Advance() error {
 			}
 		}
 
-		ignored, err := isIgnoredReference(i.Repository().Repository, ref)
-		if err != nil {
-			if i.skipGitErrors {
-				continue
-			}
-
-			return err
-		}
-
-		if ignored {
+		if isIgnoredReference(ref) {
 			continue
 		}
 
@@ -1010,7 +983,7 @@ func (i *squashRefRefCommitsIter) Advance() error {
 					"error": err,
 				}).Error("unable to get commit")
 
-				if i.skipGitErrors {
+				if errInvalidCommit.Is(err) || i.skipGitErrors {
 					continue
 				}
 
@@ -1116,7 +1089,7 @@ func (i *squashRefHeadRefCommitsIter) Advance() error {
 				"error": err,
 			}).Error("unable to get commit")
 
-			if i.skipGitErrors {
+			if errInvalidCommit.Is(err) || i.skipGitErrors {
 				continue
 			}