From 6384ab93a2dbac9045ee19099455cbcfe82d0201 Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Thu, 30 Aug 2018 20:28:40 +0200 Subject: storage/dotgit: add KeepDescriptors option This option maintains packfile file descriptors opened after reading objects from them. It improves performance as it does not have to be opening packfiles each time an object is needed. Also adds Close to EncodedObjectStorer to close all the files manualy. Signed-off-by: Javi Fontan --- plumbing/storer/object.go | 2 ++ plumbing/storer/object_test.go | 4 +++ storage/filesystem/dotgit/dotgit.go | 43 +++++++++++++++++++++++++++++++- storage/filesystem/dotgit/dotgit_test.go | 28 +++++++++++++++++++++ storage/filesystem/object.go | 10 +++++++- storage/filesystem/storage.go | 4 +++ storage/memory/storage.go | 3 +++ 7 files changed, 92 insertions(+), 2 deletions(-) diff --git a/plumbing/storer/object.go b/plumbing/storer/object.go index 92aa629..806b6bb 100644 --- a/plumbing/storer/object.go +++ b/plumbing/storer/object.go @@ -40,6 +40,8 @@ type EncodedObjectStorer interface { // HasEncodedObject returns ErrObjNotFound if the object doesn't // exist. If the object does exist, it returns nil. HasEncodedObject(plumbing.Hash) error + // Close any opened files. + Close() error } // DeltaObjectStorer is an EncodedObjectStorer that can return delta diff --git a/plumbing/storer/object_test.go b/plumbing/storer/object_test.go index 6b4fe0f..adddec4 100644 --- a/plumbing/storer/object_test.go +++ b/plumbing/storer/object_test.go @@ -157,3 +157,7 @@ func (o *MockObjectStorage) IterEncodedObjects(t plumbing.ObjectType) (EncodedOb func (o *MockObjectStorage) Begin() Transaction { return nil } + +func (o *MockObjectStorage) Close() error { + return nil +} diff --git a/storage/filesystem/dotgit/dotgit.go b/storage/filesystem/dotgit/dotgit.go index 00dd2a4..df5cd10 100644 --- a/storage/filesystem/dotgit/dotgit.go +++ b/storage/filesystem/dotgit/dotgit.go @@ -62,6 +62,9 @@ type Options struct { // ExclusiveAccess means that the filesystem is not modified externally // while the repo is open. ExclusiveAccess bool + // KeepDescriptors makes the file descriptors to be reused but they will + // need to be manually closed calling Close(). + KeepDescriptors bool } // The DotGit type represents a local git repository on disk. This @@ -78,6 +81,8 @@ type DotGit struct { objectMap map[plumbing.Hash]struct{} packList []plumbing.Hash packMap map[plumbing.Hash]struct{} + + files map[string]billy.File } // New returns a DotGit value ready to be used. The path argument must @@ -123,6 +128,28 @@ func (d *DotGit) Initialize() error { return nil } +// Close closes all opened files. +func (d *DotGit) Close() error { + var firstError error + if d.files != nil { + for _, f := range d.files { + err := f.Close() + if err != nil && firstError == nil { + firstError = err + continue + } + } + + d.files = nil + } + + if firstError != nil { + return firstError + } + + return nil +} + // ConfigWriter returns a file pointer for write to the config file func (d *DotGit) ConfigWriter() (billy.File, error) { return d.fs.Create(configPath) @@ -217,12 +244,22 @@ func (d *DotGit) objectPackPath(hash plumbing.Hash, extension string) string { } func (d *DotGit) objectPackOpen(hash plumbing.Hash, extension string) (billy.File, error) { + if d.files == nil { + d.files = make(map[string]billy.File) + } + err := d.hasPack(hash) if err != nil { return nil, err } - pack, err := d.fs.Open(d.objectPackPath(hash, extension)) + path := d.objectPackPath(hash, extension) + f, ok := d.files[path] + if ok { + return f, nil + } + + pack, err := d.fs.Open(path) if err != nil { if os.IsNotExist(err) { return nil, ErrPackfileNotFound @@ -231,6 +268,10 @@ func (d *DotGit) objectPackOpen(hash plumbing.Hash, extension string) (billy.Fil return nil, err } + if d.options.KeepDescriptors && extension == "pack" { + d.files[path] = pack + } + return pack, nil } diff --git a/storage/filesystem/dotgit/dotgit_test.go b/storage/filesystem/dotgit/dotgit_test.go index c34543e..50f8e64 100644 --- a/storage/filesystem/dotgit/dotgit_test.go +++ b/storage/filesystem/dotgit/dotgit_test.go @@ -465,6 +465,34 @@ func (s *SuiteDotGit) TestObjectPack(c *C) { c.Assert(filepath.Ext(pack.Name()), Equals, ".pack") } +func (s *SuiteDotGit) TestObjectPackWithKeepDescriptors(c *C) { + f := fixtures.Basic().ByTag(".git").One() + fs := f.DotGit() + dir := NewWithOptions(fs, Options{KeepDescriptors: true}) + + pack, err := dir.ObjectPack(f.PackfileHash) + c.Assert(err, IsNil) + c.Assert(filepath.Ext(pack.Name()), Equals, ".pack") + + pack2, err := dir.ObjectPack(f.PackfileHash) + c.Assert(err, IsNil) + c.Assert(pack, Equals, pack2) + + err = dir.Close() + c.Assert(err, IsNil) + + pack2, err = dir.ObjectPack(f.PackfileHash) + c.Assert(err, IsNil) + c.Assert(pack, Not(Equals), pack2) + + err = pack2.Close() + c.Assert(err, IsNil) + + err = dir.Close() + c.Assert(err, NotNil) + +} + func (s *SuiteDotGit) TestObjectPackIdx(c *C) { f := fixtures.Basic().ByTag(".git").One() fs := f.DotGit() diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go index 3519385..3545e27 100644 --- a/storage/filesystem/object.go +++ b/storage/filesystem/object.go @@ -74,6 +74,7 @@ func (s *ObjectStorage) loadIdxFile(h plumbing.Hash) (err error) { } defer ioutil.CheckClose(f, &err) + idxf := idxfile.NewMemoryIndex() d := idxfile.NewDecoder(f) if err = d.Decode(idxf); err != nil { @@ -280,7 +281,9 @@ func (s *ObjectStorage) getFromPackfile(h plumbing.Hash, canBeDelta bool) ( return nil, err } - defer ioutil.CheckClose(f, &err) + if !s.options.KeepDescriptors { + defer ioutil.CheckClose(f, &err) + } idx := s.index[pack] if canBeDelta { @@ -423,6 +426,11 @@ func (s *ObjectStorage) buildPackfileIters(t plumbing.ObjectType, seen map[plumb }, nil } +// Close closes all opened files. +func (s *ObjectStorage) Close() error { + return s.dir.Close() +} + type lazyPackfilesIter struct { hashes []plumbing.Hash open func(h plumbing.Hash) (storer.EncodedObjectIter, error) diff --git a/storage/filesystem/storage.go b/storage/filesystem/storage.go index 25b3653..7fae789 100644 --- a/storage/filesystem/storage.go +++ b/storage/filesystem/storage.go @@ -27,6 +27,9 @@ type Options struct { // ExclusiveAccess means that the filesystem is not modified externally // while the repo is open. ExclusiveAccess bool + // KeepDescriptors makes the file descriptors to be reused but they will + // need to be manually closed calling Close(). + KeepDescriptors bool } // NewStorage returns a new Storage backed by a given `fs.Filesystem` @@ -41,6 +44,7 @@ func NewStorageWithOptions( ) (*Storage, error) { dirOps := dotgit.Options{ ExclusiveAccess: ops.ExclusiveAccess, + KeepDescriptors: ops.KeepDescriptors, } dir := dotgit.NewWithOptions(fs, dirOps) diff --git a/storage/memory/storage.go b/storage/memory/storage.go index 2e32509..3d3b348 100644 --- a/storage/memory/storage.go +++ b/storage/memory/storage.go @@ -183,6 +183,9 @@ func (o *ObjectStorage) ObjectPacks() ([]plumbing.Hash, error) { func (o *ObjectStorage) DeleteOldObjectPackAndIndex(plumbing.Hash, time.Time) error { return nil } +func (s *ObjectStorage) Close() error { + return nil +} var errNotSupported = fmt.Errorf("Not supported") -- cgit From 5260b87fd08f70df6f1eb297ccb04d74d32dd6c8 Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Tue, 4 Sep 2018 18:26:59 +0200 Subject: plumbing/storer: do not expose Close in EncodedObjectStorer interface Signed-off-by: Javi Fontan --- plumbing/storer/object.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/plumbing/storer/object.go b/plumbing/storer/object.go index 806b6bb..92aa629 100644 --- a/plumbing/storer/object.go +++ b/plumbing/storer/object.go @@ -40,8 +40,6 @@ type EncodedObjectStorer interface { // HasEncodedObject returns ErrObjNotFound if the object doesn't // exist. If the object does exist, it returns nil. HasEncodedObject(plumbing.Hash) error - // Close any opened files. - Close() error } // DeltaObjectStorer is an EncodedObjectStorer that can return delta -- cgit From 9013dde72d0387a74b728ee336019728ba159d1c Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Wed, 5 Sep 2018 11:01:06 +0200 Subject: storage/filesystem: add KeepDescriptors test Also delete Close from MockObjectStorage and memory storer. Signed-off-by: Javi Fontan --- plumbing/storer/object_test.go | 4 ---- storage/filesystem/object_test.go | 31 +++++++++++++++++++++++++++++++ storage/memory/storage.go | 3 --- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/plumbing/storer/object_test.go b/plumbing/storer/object_test.go index adddec4..6b4fe0f 100644 --- a/plumbing/storer/object_test.go +++ b/plumbing/storer/object_test.go @@ -157,7 +157,3 @@ func (o *MockObjectStorage) IterEncodedObjects(t plumbing.ObjectType) (EncodedOb func (o *MockObjectStorage) Begin() Transaction { return nil } - -func (o *MockObjectStorage) Close() error { - return nil -} diff --git a/storage/filesystem/object_test.go b/storage/filesystem/object_test.go index b1408b7..6feb6ae 100644 --- a/storage/filesystem/object_test.go +++ b/storage/filesystem/object_test.go @@ -48,6 +48,37 @@ func (s *FsSuite) TestGetFromPackfile(c *C) { }) } +func (s *FsSuite) TestGetFromPackfileKeepDescriptors(c *C) { + fixtures.Basic().ByTag(".git").Test(c, func(f *fixtures.Fixture) { + fs := f.DotGit() + dg := dotgit.NewWithOptions(fs, dotgit.Options{KeepDescriptors: true}) + o, err := NewObjectStorageWithOptions(dg, Options{KeepDescriptors: true}) + c.Assert(err, IsNil) + + expected := plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5") + obj, err := o.EncodedObject(plumbing.AnyObject, expected) + c.Assert(err, IsNil) + c.Assert(obj.Hash(), Equals, expected) + + packfiles, err := dg.ObjectPacks() + c.Assert(err, IsNil) + + pack1, err := dg.ObjectPack(packfiles[0]) + c.Assert(err, IsNil) + + err = o.Close() + c.Assert(err, IsNil) + + pack2, err := dg.ObjectPack(packfiles[0]) + c.Assert(err, IsNil) + c.Assert(pack1, Not(Equals), pack2) + + err = o.Close() + c.Assert(err, IsNil) + + }) +} + func (s *FsSuite) TestGetFromPackfileMultiplePackfiles(c *C) { fs := fixtures.ByTag(".git").ByTag("multi-packfile").One().DotGit() o, err := NewObjectStorage(dotgit.New(fs)) diff --git a/storage/memory/storage.go b/storage/memory/storage.go index 3d3b348..2e32509 100644 --- a/storage/memory/storage.go +++ b/storage/memory/storage.go @@ -183,9 +183,6 @@ func (o *ObjectStorage) ObjectPacks() ([]plumbing.Hash, error) { func (o *ObjectStorage) DeleteOldObjectPackAndIndex(plumbing.Hash, time.Time) error { return nil } -func (s *ObjectStorage) Close() error { - return nil -} var errNotSupported = fmt.Errorf("Not supported") -- cgit From 8176f084d861891d1846a2d46bf669d0d3463ebd Mon Sep 17 00:00:00 2001 From: Javi Fontan Date: Thu, 6 Sep 2018 19:09:02 +0200 Subject: storage/filesystem: compare files using offset in test Using equals to compare files it uses diff to do so. This can potentially consume lots of ram. Changed the comparison to use file offsets. If the descriptor is reused the offset is maintained. Signed-off-by: Javi Fontan --- storage/filesystem/dotgit/dotgit_test.go | 15 +++++++++++++-- storage/filesystem/object_test.go | 8 +++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/storage/filesystem/dotgit/dotgit_test.go b/storage/filesystem/dotgit/dotgit_test.go index 50f8e64..308c6b7 100644 --- a/storage/filesystem/dotgit/dotgit_test.go +++ b/storage/filesystem/dotgit/dotgit_test.go @@ -474,16 +474,27 @@ func (s *SuiteDotGit) TestObjectPackWithKeepDescriptors(c *C) { c.Assert(err, IsNil) c.Assert(filepath.Ext(pack.Name()), Equals, ".pack") + // Move to an specific offset + pack.Seek(42, os.SEEK_SET) + pack2, err := dir.ObjectPack(f.PackfileHash) c.Assert(err, IsNil) - c.Assert(pack, Equals, pack2) + + // If the file is the same the offset should be the same + offset, err := pack2.Seek(0, os.SEEK_CUR) + c.Assert(err, IsNil) + c.Assert(offset, Equals, int64(42)) err = dir.Close() c.Assert(err, IsNil) pack2, err = dir.ObjectPack(f.PackfileHash) c.Assert(err, IsNil) - c.Assert(pack, Not(Equals), pack2) + + // If the file is opened again its offset should be 0 + offset, err = pack2.Seek(0, os.SEEK_CUR) + c.Assert(err, IsNil) + c.Assert(offset, Equals, int64(0)) err = pack2.Close() c.Assert(err, IsNil) diff --git a/storage/filesystem/object_test.go b/storage/filesystem/object_test.go index 6feb6ae..4a921a9 100644 --- a/storage/filesystem/object_test.go +++ b/storage/filesystem/object_test.go @@ -2,6 +2,7 @@ package filesystem import ( "io/ioutil" + "os" "testing" "gopkg.in/src-d/go-git.v4/plumbing" @@ -66,12 +67,17 @@ func (s *FsSuite) TestGetFromPackfileKeepDescriptors(c *C) { pack1, err := dg.ObjectPack(packfiles[0]) c.Assert(err, IsNil) + pack1.Seek(42, os.SEEK_SET) + err = o.Close() c.Assert(err, IsNil) pack2, err := dg.ObjectPack(packfiles[0]) c.Assert(err, IsNil) - c.Assert(pack1, Not(Equals), pack2) + + offset, err := pack2.Seek(0, os.SEEK_CUR) + c.Assert(err, IsNil) + c.Assert(offset, Equals, int64(0)) err = o.Close() c.Assert(err, IsNil) -- cgit