plumbing: format/packfile, prevent large objects from being read into memory completely

[zeripath]

This PR adds code to prevent large objects from being read into memory
from packfiles or the filesystem through the use of a new Option, the LargeObjectThreshold.

Objects greater than the provided (optional) LargeObjectThreshold are now no longer directly stored in the cache
or read completely into memory.

This PR differs and improves the previous broken #303 by fixing several
bugs in the reader and transparently wrapping ReaderAt as a Reader.

Signed-off-by: Andrew Thornton art27@cantab.net

---

(UPDATED: Make LOT an Option on the ObjectStorage - defaulted to off.)

[zeripath]

@hiddeco This should fix the problems!

[zeripath]

This PR differs from the previous #303 here by wrapping the reader in a reader at - so as to prevent problems from the scanner moving on etc.

[zeripath]

There was also a bug here in the original #303 that this fixes.

[zeripath]

This line was changed from the original PR where it was if discard < 0 {

discard here is a uint not an int64 - so checking if it's less than 0 will always fail.

This was the cause of the truncated checkouts.

[zeripath]

@radeksimko would you be able to test?

[radeksimko]

@zeripath I can confirm that our relevant downstream tests (which have go-git in codepath) are passing with a1fc3fc

[zeripath]

thanks for checking! On my tests I couldn't see a speed difference but did you see one?

[hiddeco]

Thank you for the quick turnaround on the second attempt @zeripath 🥇

As you can see above, I created a PR to lazily let CI run some tests we added to cover some of the previously failing parts. Will probably not get into testing it in detail this week, as we worked around the major issues for now by relying less on go-git.

[zeripath]

@hiddeco have you noticed any problems with this?

[zeripath]

Anyone found any problems?

[hiddeco]

I have thus far not found any problems but have not attempted write / push operations yet.

[xkeyC]

Good fix,I think it will save me!
My git clone ate about 500M of RAM.🤣

How to test it? The internal package prevents me from importing this branch normally.

github.com\go-git\go-git@v4.7.0+incompatible\repository.go:14:2: use of internal package gopkg.in/src-d/go-git.v4/internal/revision not allowed

[jfontan]

I've tested a repository with big blobs (and one big commit data) both packed a unpacked. Everything seems to work ok.

[zeripath]

So do we think we're ready to try again?

Should I make it configurable somehow?

[mcuadros]

Sorry, I had a major family issue that now is solved. Can you make it configurable the limit and also an option to disable?

[zeripath]

So the only way I can see to make this configurable beyond just the recompiling and replacing the constant with MaxInt at link time using -X would be to infect the whole tree:

We would need the flags to be available at lines 66 and 67 here:

go-git/plumbing/format/packfile/fsobject.go

Lines 50 to 69 in fb6894b

	func (o *FSObject) Reader() (io.ReadCloser, error) {
	obj, ok := o.cache.Get(o.hash)
	if ok && obj != o {
	reader, err := obj.Reader()
	if err != nil {
	return nil, err
	}
	return reader, nil
	}
	f, err := o.fs.Open(o.path)
	if err != nil {
	return nil, err
	}
	p := NewPackfileWithCache(o.index, nil, f, o.cache)
	if o.size > LargeObjectThreshold {
	// We have a big object
	h, err := p.objectHeaderAtOffset(o.offset)

which would mean that the signature of Reader would need to change or (better) plumbing/format/packfile.FSObject would need to gain the flag(s) (and its NewFSObject fn).

If FSObject needed to gain the flag then - plumbing/format/packfile.Packfile would need to gain the flag(s), (Similarly NewPackfile and NewPackfileWithCache)

That would mean changes for the related functions in storage/filesystem.ObjectStorage and storage/filesystem.Options

We would also need to add changes to storage/filesystem/dotgit.DotGit too.

---

If that's acceptable I'll push up changes that do that.

[zeripath]

Proposed Patch

From 85feea76404121fd1570e04cda0563cb82b108da Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Mon, 28 Jun 2021 09:09:43 +0100
Subject: [PATCH] Make the LargeObjectThreshold optional

This commit moves the LargeObjectThreshold into an option set on the
ObjectStorage. Unfortunately this means that the LOT has to percolate
down the calling tree.

Signed-off-by: Andrew Thornton <art27@cantab.net>
---
 .../format/packfile/encoder_advanced_test.go  |  2 +-
 plumbing/format/packfile/encoder_test.go      |  2 +-
 plumbing/format/packfile/fsobject.go          | 41 ++++++------
 plumbing/format/packfile/packfile.go          | 24 ++++---
 plumbing/format/packfile/packfile_test.go     | 12 ++--
 storage/filesystem/object.go                  | 15 +++--
 storage/filesystem/object_test.go             | 63 +++++++++++++++++--
 storage/filesystem/storage.go                 |  3 +
 8 files changed, 112 insertions(+), 50 deletions(-)

diff --git a/plumbing/format/packfile/encoder_advanced_test.go b/plumbing/format/packfile/encoder_advanced_test.go
index 95db5c0..15c0fba 100644
--- a/plumbing/format/packfile/encoder_advanced_test.go
+++ b/plumbing/format/packfile/encoder_advanced_test.go
@@ -105,7 +105,7 @@ func (s *EncoderAdvancedSuite) testEncodeDecode(
 	_, err = f.Seek(0, io.SeekStart)
 	c.Assert(err, IsNil)
 
-	p := NewPackfile(index, fs, f)
+	p := NewPackfile(index, fs, f, 0)
 
 	decodeHash, err := p.ID()
 	c.Assert(err, IsNil)
diff --git a/plumbing/format/packfile/encoder_test.go b/plumbing/format/packfile/encoder_test.go
index d2db892..c9d49c3 100644
--- a/plumbing/format/packfile/encoder_test.go
+++ b/plumbing/format/packfile/encoder_test.go
@@ -318,7 +318,7 @@ func packfileFromReader(c *C, buf *bytes.Buffer) (*Packfile, func()) {
 	index, err := w.Index()
 	c.Assert(err, IsNil)
 
-	return NewPackfile(index, fs, file), func() {
+	return NewPackfile(index, fs, file, 0), func() {
 		c.Assert(file.Close(), IsNil)
 	}
 }
diff --git a/plumbing/format/packfile/fsobject.go b/plumbing/format/packfile/fsobject.go
index 4aa3c8e..a395d17 100644
--- a/plumbing/format/packfile/fsobject.go
+++ b/plumbing/format/packfile/fsobject.go
@@ -12,15 +12,16 @@ import (
 
 // FSObject is an object from the packfile on the filesystem.
 type FSObject struct {
-	hash   plumbing.Hash
-	h      *ObjectHeader
-	offset int64
-	size   int64
-	typ    plumbing.ObjectType
-	index  idxfile.Index
-	fs     billy.Filesystem
-	path   string
-	cache  cache.Object
+	hash                 plumbing.Hash
+	h                    *ObjectHeader
+	offset               int64
+	size                 int64
+	typ                  plumbing.ObjectType
+	index                idxfile.Index
+	fs                   billy.Filesystem
+	path                 string
+	cache                cache.Object
+	largeObjectThreshold int64
 }
 
 // NewFSObject creates a new filesystem object.
@@ -33,16 +34,18 @@ func NewFSObject(
 	fs billy.Filesystem,
 	path string,
 	cache cache.Object,
+	largeObjectThreshold int64,
 ) *FSObject {
 	return &FSObject{
-		hash:   hash,
-		offset: offset,
-		size:   contentSize,
-		typ:    finalType,
-		index:  index,
-		fs:     fs,
-		path:   path,
-		cache:  cache,
+		hash:                 hash,
+		offset:               offset,
+		size:                 contentSize,
+		typ:                  finalType,
+		index:                index,
+		fs:                   fs,
+		path:                 path,
+		cache:                cache,
+		largeObjectThreshold: largeObjectThreshold,
 	}
 }
 
@@ -63,8 +66,8 @@ func (o *FSObject) Reader() (io.ReadCloser, error) {
 		return nil, err
 	}
 
-	p := NewPackfileWithCache(o.index, nil, f, o.cache)
-	if o.size > LargeObjectThreshold {
+	p := NewPackfileWithCache(o.index, nil, f, o.cache, o.largeObjectThreshold)
+	if o.largeObjectThreshold > 0 && o.size > o.largeObjectThreshold {
 		// We have a big object
 		h, err := p.objectHeaderAtOffset(o.offset)
 		if err != nil {
diff --git a/plumbing/format/packfile/packfile.go b/plumbing/format/packfile/packfile.go
index 2bb0596..8dd6041 100644
--- a/plumbing/format/packfile/packfile.go
+++ b/plumbing/format/packfile/packfile.go
@@ -34,20 +34,15 @@ var (
 // wrapped in FSObject.
 const smallObjectThreshold = 16 * 1024
 
-// Conversely there are large objects that should not be cached and kept
-// in memory as they're too large to be reasonably cached. Objects larger
-// than this threshold are now always never read into memory to be stored
-// in the cache
-const LargeObjectThreshold = 1024 * 1024
-
 // Packfile allows retrieving information from inside a packfile.
 type Packfile struct {
 	idxfile.Index
-	fs             billy.Filesystem
-	file           billy.File
-	s              *Scanner
-	deltaBaseCache cache.Object
-	offsetToType   map[int64]plumbing.ObjectType
+	fs                   billy.Filesystem
+	file                 billy.File
+	s                    *Scanner
+	deltaBaseCache       cache.Object
+	offsetToType         map[int64]plumbing.ObjectType
+	largeObjectThreshold int64
 }
 
 // NewPackfileWithCache creates a new Packfile with the given object cache.
@@ -58,6 +53,7 @@ func NewPackfileWithCache(
 	fs billy.Filesystem,
 	file billy.File,
 	cache cache.Object,
+	largeObjectThreshold int64,
 ) *Packfile {
 	s := NewScanner(file)
 	return &Packfile{
@@ -67,6 +63,7 @@ func NewPackfileWithCache(
 		s,
 		cache,
 		make(map[int64]plumbing.ObjectType),
+		largeObjectThreshold,
 	}
 }
 
@@ -74,8 +71,8 @@ func NewPackfileWithCache(
 // and packfile idx.
 // If the filesystem is provided, the packfile will return FSObjects, otherwise
 // it will return MemoryObjects.
-func NewPackfile(index idxfile.Index, fs billy.Filesystem, file billy.File) *Packfile {
-	return NewPackfileWithCache(index, fs, file, cache.NewObjectLRUDefault())
+func NewPackfile(index idxfile.Index, fs billy.Filesystem, file billy.File, largeObjectThreshold int64) *Packfile {
+	return NewPackfileWithCache(index, fs, file, cache.NewObjectLRUDefault(), largeObjectThreshold)
 }
 
 // Get retrieves the encoded object in the packfile with the given hash.
@@ -271,6 +268,7 @@ func (p *Packfile) getNextObject(h *ObjectHeader, hash plumbing.Hash) (plumbing.
 		p.fs,
 		p.file.Name(),
 		p.deltaBaseCache,
+		p.largeObjectThreshold,
 	), nil
 }
 
diff --git a/plumbing/format/packfile/packfile_test.go b/plumbing/format/packfile/packfile_test.go
index 60c7c73..6af8817 100644
--- a/plumbing/format/packfile/packfile_test.go
+++ b/plumbing/format/packfile/packfile_test.go
@@ -111,7 +111,7 @@ func (s *PackfileSuite) SetUpTest(c *C) {
 	s.idx = idxfile.NewMemoryIndex()
 	c.Assert(idxfile.NewDecoder(s.f.Idx()).Decode(s.idx), IsNil)
 
-	s.p = packfile.NewPackfile(s.idx, fixtures.Filesystem, s.f.Packfile())
+	s.p = packfile.NewPackfile(s.idx, fixtures.Filesystem, s.f.Packfile(), 0)
 }
 
 func (s *PackfileSuite) TearDownTest(c *C) {
@@ -122,7 +122,7 @@ func (s *PackfileSuite) TestDecode(c *C) {
 	fixtures.Basic().ByTag("packfile").Test(c, func(f *fixtures.Fixture) {
 		index := getIndexFromIdxFile(f.Idx())
 
-		p := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile())
+		p := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile(), 0)
 		defer p.Close()
 
 		for _, h := range expectedHashes {
@@ -138,7 +138,7 @@ func (s *PackfileSuite) TestDecodeByTypeRefDelta(c *C) {
 
 	index := getIndexFromIdxFile(f.Idx())
 
-	packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile())
+	packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile(), 0)
 	defer packfile.Close()
 
 	iter, err := packfile.GetByType(plumbing.CommitObject)
@@ -171,7 +171,7 @@ func (s *PackfileSuite) TestDecodeByType(c *C) {
 		for _, t := range ts {
 			index := getIndexFromIdxFile(f.Idx())
 
-			packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile())
+			packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile(), 0)
 			defer packfile.Close()
 
 			iter, err := packfile.GetByType(t)
@@ -189,7 +189,7 @@ func (s *PackfileSuite) TestDecodeByTypeConstructor(c *C) {
 	f := fixtures.Basic().ByTag("packfile").One()
 	index := getIndexFromIdxFile(f.Idx())
 
-	packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile())
+	packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile(), 0)
 	defer packfile.Close()
 
 	_, err := packfile.GetByType(plumbing.OFSDeltaObject)
@@ -266,7 +266,7 @@ func (s *PackfileSuite) TestSize(c *C) {
 
 	index := getIndexFromIdxFile(f.Idx())
 
-	packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile())
+	packfile := packfile.NewPackfile(index, fixtures.Filesystem, f.Packfile(), 0)
 	defer packfile.Close()
 
 	// Get the size of binary.jpg, which is not delta-encoded.
diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go
index f4c2bae..5c91bcd 100644
--- a/storage/filesystem/object.go
+++ b/storage/filesystem/object.go
@@ -204,9 +204,9 @@ func (s *ObjectStorage) packfile(idx idxfile.Index, pack plumbing.Hash) (*packfi
 
 	var p *packfile.Packfile
 	if s.objectCache != nil {
-		p = packfile.NewPackfileWithCache(idx, s.dir.Fs(), f, s.objectCache)
+		p = packfile.NewPackfileWithCache(idx, s.dir.Fs(), f, s.objectCache, s.options.LargeObjectThreshold)
 	} else {
-		p = packfile.NewPackfile(idx, s.dir.Fs(), f)
+		p = packfile.NewPackfile(idx, s.dir.Fs(), f, s.options.LargeObjectThreshold)
 	}
 
 	return p, s.storePackfileInCache(pack, p)
@@ -401,7 +401,7 @@ func (s *ObjectStorage) getFromUnpacked(h plumbing.Hash) (obj plumbing.EncodedOb
 		return nil, err
 	}
 
-	if size > packfile.LargeObjectThreshold {
+	if s.options.LargeObjectThreshold > 0 && size > s.options.LargeObjectThreshold {
 		obj = dotgit.NewEncodedObject(s.dir, h, t, size)
 		return obj, nil
 	}
@@ -601,6 +601,7 @@ func (s *ObjectStorage) buildPackfileIters(
 			return newPackfileIter(
 				s.dir.Fs(), pack, t, seen, s.index[h],
 				s.objectCache, s.options.KeepDescriptors,
+				s.options.LargeObjectThreshold,
 			)
 		},
 	}, nil
@@ -690,6 +691,7 @@ func NewPackfileIter(
 	idxFile billy.File,
 	t plumbing.ObjectType,
 	keepPack bool,
+	largeObjectThreshold int64,
 ) (storer.EncodedObjectIter, error) {
 	idx := idxfile.NewMemoryIndex()
 	if err := idxfile.NewDecoder(idxFile).Decode(idx); err != nil {
@@ -701,7 +703,7 @@ func NewPackfileIter(
 	}
 
 	seen := make(map[plumbing.Hash]struct{})
-	return newPackfileIter(fs, f, t, seen, idx, nil, keepPack)
+	return newPackfileIter(fs, f, t, seen, idx, nil, keepPack, largeObjectThreshold)
 }
 
 func newPackfileIter(
@@ -712,12 +714,13 @@ func newPackfileIter(
 	index idxfile.Index,
 	cache cache.Object,
 	keepPack bool,
+	largeObjectThreshold int64,
 ) (storer.EncodedObjectIter, error) {
 	var p *packfile.Packfile
 	if cache != nil {
-		p = packfile.NewPackfileWithCache(index, fs, f, cache)
+		p = packfile.NewPackfileWithCache(index, fs, f, cache, largeObjectThreshold)
 	} else {
-		p = packfile.NewPackfile(index, fs, f)
+		p = packfile.NewPackfile(index, fs, f, largeObjectThreshold)
 	}
 
 	iter, err := p.GetByType(t)
diff --git a/storage/filesystem/object_test.go b/storage/filesystem/object_test.go
index 22f5b0c..59b40d3 100644
--- a/storage/filesystem/object_test.go
+++ b/storage/filesystem/object_test.go
@@ -107,6 +107,27 @@ func (s *FsSuite) TestGetFromPackfileMaxOpenDescriptors(c *C) {
 	c.Assert(err, IsNil)
 }
 
+func (s *FsSuite) TestGetFromPackfileMaxOpenDescriptorsLargeObjectThreshold(c *C) {
+	fs := fixtures.ByTag(".git").ByTag("multi-packfile").One().DotGit()
+	o := NewObjectStorageWithOptions(dotgit.New(fs), cache.NewObjectLRUDefault(), Options{
+		MaxOpenDescriptors:   1,
+		LargeObjectThreshold: 1,
+	})
+
+	expected := plumbing.NewHash("8d45a34641d73851e01d3754320b33bb5be3c4d3")
+	obj, err := o.getFromPackfile(expected, false)
+	c.Assert(err, IsNil)
+	c.Assert(obj.Hash(), Equals, expected)
+
+	expected = plumbing.NewHash("e9cfa4c9ca160546efd7e8582ec77952a27b17db")
+	obj, err = o.getFromPackfile(expected, false)
+	c.Assert(err, IsNil)
+	c.Assert(obj.Hash(), Equals, expected)
+
+	err = o.Close()
+	c.Assert(err, IsNil)
+}
+
 func (s *FsSuite) TestGetSizeOfObjectFile(c *C) {
 	fs := fixtures.ByTag(".git").ByTag("unpacked").One().DotGit()
 	o := NewObjectStorage(dotgit.New(fs), cache.NewObjectLRUDefault())
@@ -160,6 +181,21 @@ func (s *FsSuite) TestGetFromPackfileMultiplePackfiles(c *C) {
 	c.Assert(obj.Hash(), Equals, expected)
 }
 
+func (s *FsSuite) TestGetFromPackfileMultiplePackfilesLargeObjectThreshold(c *C) {
+	fs := fixtures.ByTag(".git").ByTag("multi-packfile").One().DotGit()
+	o := NewObjectStorageWithOptions(dotgit.New(fs), cache.NewObjectLRUDefault(), Options{LargeObjectThreshold: 1})
+
+	expected := plumbing.NewHash("8d45a34641d73851e01d3754320b33bb5be3c4d3")
+	obj, err := o.getFromPackfile(expected, false)
+	c.Assert(err, IsNil)
+	c.Assert(obj.Hash(), Equals, expected)
+
+	expected = plumbing.NewHash("e9cfa4c9ca160546efd7e8582ec77952a27b17db")
+	obj, err = o.getFromPackfile(expected, false)
+	c.Assert(err, IsNil)
+	c.Assert(obj.Hash(), Equals, expected)
+}
+
 func (s *FsSuite) TestIter(c *C) {
 	fixtures.ByTag(".git").ByTag("packfile").Test(c, func(f *fixtures.Fixture) {
 		fs := f.DotGit()
@@ -179,6 +215,25 @@ func (s *FsSuite) TestIter(c *C) {
 	})
 }
 
+func (s *FsSuite) TestIterLargeObjectThreshold(c *C) {
+	fixtures.ByTag(".git").ByTag("packfile").Test(c, func(f *fixtures.Fixture) {
+		fs := f.DotGit()
+		o := NewObjectStorageWithOptions(dotgit.New(fs), cache.NewObjectLRUDefault(), Options{LargeObjectThreshold: 1})
+
+		iter, err := o.IterEncodedObjects(plumbing.AnyObject)
+		c.Assert(err, IsNil)
+
+		var count int32
+		err = iter.ForEach(func(o plumbing.EncodedObject) error {
+			count++
+			return nil
+		})
+
+		c.Assert(err, IsNil)
+		c.Assert(count, Equals, f.ObjectsCount)
+	})
+}
+
 func (s *FsSuite) TestIterWithType(c *C) {
 	fixtures.ByTag(".git").Test(c, func(f *fixtures.Fixture) {
 		for _, t := range objectTypes {
@@ -215,7 +270,7 @@ func (s *FsSuite) TestPackfileIter(c *C) {
 				idxf, err := dg.ObjectPackIdx(h)
 				c.Assert(err, IsNil)
 
-				iter, err := NewPackfileIter(fs, f, idxf, t, false)
+				iter, err := NewPackfileIter(fs, f, idxf, t, false, 0)
 				c.Assert(err, IsNil)
 
 				err = iter.ForEach(func(o plumbing.EncodedObject) error {
@@ -298,7 +353,7 @@ func (s *FsSuite) TestPackfileIterKeepDescriptors(c *C) {
 				idxf, err := dg.ObjectPackIdx(h)
 				c.Assert(err, IsNil)
 
-				iter, err := NewPackfileIter(fs, f, idxf, t, true)
+				iter, err := NewPackfileIter(fs, f, idxf, t, true, 0)
 				c.Assert(err, IsNil)
 
 				err = iter.ForEach(func(o plumbing.EncodedObject) error {
@@ -377,7 +432,7 @@ func BenchmarkPackfileIter(b *testing.B) {
 							b.Fatal(err)
 						}
 
-						iter, err := NewPackfileIter(fs, f, idxf, t, false)
+						iter, err := NewPackfileIter(fs, f, idxf, t, false, 0)
 						if err != nil {
 							b.Fatal(err)
 						}
@@ -425,7 +480,7 @@ func BenchmarkPackfileIterReadContent(b *testing.B) {
 							b.Fatal(err)
 						}
 
-						iter, err := NewPackfileIter(fs, f, idxf, t, false)
+						iter, err := NewPackfileIter(fs, f, idxf, t, false, 0)
 						if err != nil {
 							b.Fatal(err)
 						}
diff --git a/storage/filesystem/storage.go b/storage/filesystem/storage.go
index 8b69b27..7e7a2c5 100644
--- a/storage/filesystem/storage.go
+++ b/storage/filesystem/storage.go
@@ -34,6 +34,9 @@ type Options struct {
 	// MaxOpenDescriptors is the max number of file descriptors to keep
 	// open. If KeepDescriptors is true, all file descriptors will remain open.
 	MaxOpenDescriptors int
+	// LargeObjectThreshold maximum object size (in bytes) that will be read in to memory.
+	// If left unset or set to 0 there is no limit
+	LargeObjectThreshold int64
 }
 
 // NewStorage returns a new Storage backed by a given `fs.Filesystem` and cache.
-- 
2.32.0

[zeripath]

I've pushed it up and changed the first comment to match the new changes.

[mcuadros]

I was thinking of an environment variable, to change the threshold. Maybe is the better option?

[hiddeco]

The downside of environment variables is that they may grant configuration access to end-users of the library implementation that you do not actually want them to have.

[mcuadros]

True, but in this case is something about performance, which I don't think is a bad thing.

[hiddeco]

That depends on how your application is utilizing this library, if it is like ours, the performance configuration you want to have is on a per repository basis while the runtime is shared.

[zeripath]

relying on environment variables seems like something that a downstream application should decide