packfile: cache undeltified objects to improve decode performance #218

ajnavarro · 2017-01-19T16:25:45Z

Simple object cache that keep in memory the last undeltified objects. When no more objects can be keept into memory, the oldest one is deleted.

Benchmarks:

master branch:

clone: ~4m2s
count: ~3m40s count: 207956

this PR:

clone: ~19s
count: ~14.2s count: 207956

ajnavarro · 2017-01-19T16:27:39Z

Feel free to add any comment or ways to improve this.

codecov-io · 2017-01-19T16:35:08Z

Current coverage is 76.36% (diff: 80.88%)

Merging #218 into master will decrease coverage by 0.55%

@@             master       #218   diff @@
==========================================
  Files            96         98     +2   
  Lines          6299       6359    +60   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4845       4856    +11   
- Misses          922        978    +56   
+ Partials        532        525     -7

Powered by Codecov. Last update 85a1642...b5fad06

smola

Looks great!

We should discuss about if cache should be a package on its own.
It would be nice if you could measure what's the speed up of cloning the git/git repository with this change.

smola · 2017-01-20T08:56:53Z

plumbing/format/packfile/cache.go

+	if max <= 0 {
+		max = 1
+	}
+	c.maxElements = max


extra space after if

smola · 2017-01-20T09:02:45Z

plumbing/format/packfile/cache.go

+
+func (c *cache) Add(o plumbing.EncodedObject) {
+	if len(c.order) >= c.maxElements {
+		d, order := c.order[0], c.order[1:]


This is quite inefficient, we should probably implement a queue as a circular list backed by a slice.

definitely.

smola · 2017-01-20T09:04:10Z

plumbing/format/packfile/decoder.go

@@ -62,6 +67,8 @@ type Decoder struct {

 	offsetToType map[int64]plumbing.ObjectType
 	decoderType  plumbing.ObjectType
+
+	cache *cache


Maybe we could make cache public in its own package, since an object cache might be useful in other cases...

smola · 2017-01-20T09:04:44Z

common_test.go

@@ -63,8 +63,7 @@ func (s *BaseSuite) NewRepositoryFromPackfile(f *fixtures.Fixture) *Repository {
 	p := f.Packfile()
 	defer p.Close()

-	n := packfile.NewScanner(p)
-	d, err := packfile.NewDecoder(n, r.s)
+	d, err := packfile.NewDecoder(p, r.s)


Why does this PR change the way of instantiating Scanner/Decoder?

smola · 2017-01-20T09:06:11Z

plumbing/format/packfile/decoder.go

@@ -128,9 +138,20 @@ func (d *Decoder) Decode() (checksum plumbing.Hash, err error) {
 	return d.s.Checksum()
 }

-func (d *Decoder) doDecode() error {
+func (d *Decoder) Count() (uint32, error) {
 	_, count, err := d.s.Header()


It looks like if Count is called twice, the decoding is broken?

smola · 2017-01-20T09:06:52Z

plumbing/format/packfile/decoder.go

+		return 0, err
+	}
+
+	d.cache.SetMaxElements(int(count / magicCacheNumber))


Setting the max elements of cache inside this method (public Count) looks wierd.

mcuadros · 2017-01-20T09:09:02Z

plumbing/format/packfile/decoder.go

@@ -128,9 +138,20 @@ func (d *Decoder) Decode() (checksum plumbing.Hash, err error) {
 	return d.s.Checksum()
 }

-func (d *Decoder) doDecode() error {
+func (d *Decoder) Count() (uint32, error) {


can you the documentation, I don't know the goal of this function

mcuadros · 2017-01-20T09:10:29Z

plumbing/format/packfile/cache.go

+	"gopkg.in/src-d/go-git.v4/plumbing"
+)
+
+type cache struct {


can you add a description about how operates this cache?

mcuadros · 2017-01-20T09:13:06Z

The improvement is great, but why? Will be great know the average of the re-used objects in a repository, maybe we can have a more sophisticated algorithm. How is done this at jgit and libgit?

ajnavarro · 2017-01-20T16:12:21Z

JGit implementation: https://github.com/eclipse/jgit/blob/57a263f1823d164142235a72072154f0568cb61c/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/DeltaBaseCache.java

alcortesm

This is an awesome contribution.

alcortesm · 2017-01-23T09:23:02Z

plumbing/format/packfile/cache.go

+
+func (c *cache) Add(o plumbing.EncodedObject) {
+	if len(c.order) >= c.maxElements {
+		d, order := c.order[0], c.order[1:]


definitely.

ajnavarro · 2017-01-23T12:24:47Z

I wrote from scratch the implementation, taking into account all your comments. Basically, is the same implementation as JGit.

ajnavarro · 2017-01-23T12:27:01Z

plumbing/format/packfile/decoder.go

@@ -105,6 +108,8 @@ func NewDecoderForType(s *Scanner, o storer.EncodedObjectStorer,

 		offsetToType: make(map[int64]plumbing.ObjectType, 0),
 		decoderType:  t,
+
+		cache: cache.NewObjectFIFO(cache.MaxSize),


The cache.MaxSize should be added as external repository configuration. Some idea of how to do it correctly?

Simple object cache that keep in memory the last undeltified objects. When no more objects can be keept into memory, the oldest one is deleted.

ajnavarro · 2017-01-23T14:25:43Z

Code used to check times:

package main

import (
	"time"
	"os"

	"gopkg.in/src-d/go-git.v4"
	"gopkg.in/src-d/go-git.v4/plumbing"
	"gopkg.in/src-d/go-git.v4/storage/filesystem"
	
	. "gopkg.in/src-d/go-git.v4/examples"
	osfs "srcd.works/go-billy.v1/os"
)

func main() {
	for i := 0; i < 3; i++ {
		directory := "/tmp/git/"
		os.RemoveAll(directory)
		s, err := filesystem.NewStorage(osfs.New(directory))
		CheckIfError(err)

		r, err := git.NewRepository(s)
		CheckIfError(err)
		Info("cloning")
		now := time.Now()
		err = r.Clone(
			&git.CloneOptions{
				URL: "file:///path/to/git/repository",
			},
		)
		CheckIfError(err)
		spent := time.Since(now)
		Info("clone: %s", spent)

		Info("starting to iterate")
		iter, err := s.ObjectStorage.IterEncodedObjects(plumbing.AnyObject)
		CheckIfError(err)

		count := 0
		now = time.Now()
		err = iter.ForEach(func(o plumbing.EncodedObject) error {
			count++
			return nil
		})
		CheckIfError(err)

		spent = time.Since(now)
		Info("count: %s count: %v", spent, count)
	}
}

smola · 2017-01-25T13:28:19Z

We should probably improve this in the future with an LRU cache with maximum memory size, but so far it looks great. LGTM.

* Simple object cache that keeps in memory the last undeltified objects. When no more objects can be kept into memory, the oldest one is deleted (FIFO). This speeds up packfile operations preventing redundant seeks and decodes.

ajnavarro requested review from smola, mcuadros and alcortesm January 19, 2017 16:27

smola suggested changes Jan 20, 2017

View reviewed changes

mcuadros suggested changes Jan 20, 2017

View reviewed changes

alcortesm approved these changes Jan 23, 2017

View reviewed changes

ajnavarro commented Jan 23, 2017

View reviewed changes

packfile: cache undeltified objects to improve decode performance

b5fad06

Simple object cache that keep in memory the last undeltified objects. When no more objects can be keept into memory, the oldest one is deleted.

mcuadros approved these changes Jan 24, 2017

View reviewed changes

smola approved these changes Jan 25, 2017

View reviewed changes

smola merged commit 97c0273 into src-d:master Jan 25, 2017

smola mentioned this pull request Jan 26, 2017

Performance degradation #203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

packfile: cache undeltified objects to improve decode performance #218

packfile: cache undeltified objects to improve decode performance #218

ajnavarro commented Jan 19, 2017 •

edited

Loading

ajnavarro commented Jan 19, 2017

codecov-io commented Jan 19, 2017 •

edited

Loading

smola left a comment

smola Jan 20, 2017

smola Jan 20, 2017

alcortesm Jan 23, 2017

smola Jan 20, 2017

smola Jan 20, 2017

smola Jan 20, 2017

smola Jan 20, 2017

mcuadros Jan 20, 2017

mcuadros Jan 20, 2017

mcuadros commented Jan 20, 2017

ajnavarro commented Jan 20, 2017

alcortesm left a comment

alcortesm Jan 23, 2017

ajnavarro commented Jan 23, 2017

ajnavarro Jan 23, 2017

ajnavarro commented Jan 23, 2017

smola commented Jan 25, 2017

packfile: cache undeltified objects to improve decode performance #218

packfile: cache undeltified objects to improve decode performance #218

Conversation

ajnavarro commented Jan 19, 2017 • edited Loading

ajnavarro commented Jan 19, 2017

codecov-io commented Jan 19, 2017 • edited Loading

Current coverage is 76.36% (diff: 80.88%)

smola left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcuadros commented Jan 20, 2017

ajnavarro commented Jan 20, 2017

alcortesm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajnavarro commented Jan 23, 2017

Choose a reason for hiding this comment

ajnavarro commented Jan 23, 2017

smola commented Jan 25, 2017

ajnavarro commented Jan 19, 2017 •

edited

Loading

codecov-io commented Jan 19, 2017 •

edited

Loading