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

zeripath · 2021-06-02T18:52:32Z

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.)

This PR adds code to prevent large objects from being read into memory from packfiles or the filesystem. Objects greater than 1Mb are now no longer directly stored in the cache or read completely into memory. This PR differs and improves the previous broken go-git#323 by fixing several bugs in the reader and transparently wrapping ReaderAt as a Reader. Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2021-06-02T18:53:27Z

@hiddeco This should fix the problems!

zeripath · 2021-06-02T18:55:19Z

plumbing/format/packfile/packfile.go

+		}
+		return r, nil
+	case plumbing.OFSDeltaObject:
+		deltaRc, err := asyncReader(p)


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.

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

zeripath · 2021-06-02T18:57:14Z

plumbing/format/packfile/patch_delta.go

+				}
+
+				discard := offset - basePos
+				if basePos > offset {


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 · 2021-06-02T18:59:45Z

@radeksimko would you be able to test?

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

radeksimko · 2021-06-03T14:35:25Z

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

zeripath · 2021-06-03T14:46:39Z

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

hiddeco · 2021-06-03T14:55:36Z

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 · 2021-06-08T17:07:48Z

@hiddeco have you noticed any problems with this?

zeripath · 2021-06-14T16:39:27Z

Anyone found any problems?

hiddeco · 2021-06-14T17:22:11Z

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

xkeyC · 2021-06-19T15:19:29Z

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

storage/filesystem/object.go

jfontan · 2021-06-22T20:25:48Z

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

zeripath · 2021-06-25T23:19:04Z

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

Should I make it configurable somehow?

mcuadros · 2021-06-28T07:21:09Z

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 · 2021-06-28T07:45:01Z

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 t 8000 o add changes to storage/filesystem/dotgit.DotGit too.

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

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>

zeripath · 2021-06-28T08:25:31Z

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 obje
6D40
ct
 		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 · 2021-06-28T08:33:09Z

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

mcuadros · 2021-06-29T09:35:58Z

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

hiddeco · 2021-06-29T10:54:44Z

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 · 2021-06-29T13:07:48Z

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

hiddeco · 2021-06-29T13:16:22Z

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 · 2021-06-29T14:11:56Z

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

Following the merging of go-git/go-git#330 we can now add a setting to avoid go-git reading and caching large objects. Signed-off-by: Andrew Thornton <art27@cantab.net>

* master: Update github.com/xanzy/ssh-agent to v0.3.1 Document the push refspec format Add support to push commits per hashes better tests plumbing: gitignore, Read .git/info/exclude file too. plumbing: packp, Add encoding for push-options. Fixes go-git#268. go-git: Add field `Options` to `PushOptions`, wire functionality. git: add --follow-tags option for pushes add tests examples: add find-if-any-tag-point-head Add RemoteURL to {Fetch,Pull,Push}Options plumbing/storer/object: improve grammar Go Doc (go-git#350) plumbing: format/packfile, prevent large objects from being read into memory completely (go-git#330)

… memory completely (go-git#330) This PR adds code to prevent large objects from being read into memory from packfiles or the filesystem. Objects greater than 1Mb are now no longer directly stored in the cache or read completely into memory. This PR differs and improves the previous broken go-git#323 by fixing several bugs in the reader and transparently wrapping ReaderAt as a Reader. Signed-off-by: Andrew Thornton <art27@cantab.net>

… memory completely (go-git#330) This PR adds code to prevent large objects from being read into memory from packfiles or the filesystem. Objects greater than 1Mb are now no longer directly stored in the cache or read completely into memory. This PR differs and improves the previous broken go-git#323 by fixing several bugs in the reader and transparently wrapping ReaderAt as a Reader. Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath commented Jun 2, 2021

View reviewed changes

add tests

22550b6

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

zeripath force-pushed the fix-323-fix-broken-large-ofs-delta branch from 1a104f5 to 22550b6 Compare June 2, 2021 20:15

zeripath mentioned this pull request Jun 2, 2021

Slow repository browsing in 1.14.x go-gitea/gitea#15707

Closed

6 tasks

prod CI

a1fc3fc

hiddeco mentioned this pull request Jun 3, 2021

Test go-git patch fluxcd/source-controller#373

Closed

mcuadros requested a review from jfontan June 8, 2021 17:20

jfontan reviewed Jun 22, 2021

View reviewed changes

storage/filesystem/object.go Outdated Show resolved Hide resolved

zeripath added 2 commits June 24, 2021 19:28

Don't add to the object cache

1d722fd

renudge CI

fb6894b

Make the LargeObjectThreshold optional

85feea7

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>

mcuadros changed the title ~~Prevent large objects from being read into memory completely (take 2)~~ plumbing: format/packfile, prevent large objects from being read into memory completely Jun 30, 2021

mcuadros merged commit b4368b2 into go-git:master Jun 30, 2021

zeripath mentioned this pull request Jun 30, 2021

Update Go-Git to take advantage of LargeObjectThreshold go-gitea/gitea#16316

Merged

zeripath deleted the fix-323-fix-broken-large-ofs-delta branch June 30, 2021 20:07

zeripath mentioned this pull request Jan 27, 2024

Getting object file Reader will load content of even large blobs into memory #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment