Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

plumbing: cache, enforce the use of cache in packfile decoder #698

Merged
merged 4 commits into from
Dec 20, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions plumbing/cache/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const (

type FileSize int64

const DefaultMaxSize FileSize = 96 * MiByte

// Object is an interface to a object cache.
type Object interface {
// Put puts the given object into the cache. Whether this object will
Expand Down
5 changes: 5 additions & 0 deletions plumbing/cache/object_lru.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ func NewObjectLRU(maxSize FileSize) *ObjectLRU {
return &ObjectLRU{MaxSize: maxSize}
}

// NewObjectLRUDefault creates a new ObjectLRU with the default cache size.
func NewObjectLRUDefault() *ObjectLRU {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tested function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now cache tests are done twice. One with the previous cache and another time with a cache created with NewObjectLRUDefault.

return &ObjectLRU{MaxSize: DefaultMaxSize}
}

// Put puts an object into the cache. If the object is already in the cache, it
// will be marked as used. Otherwise, it will be inserted. A single object might
// be evicted to make room for the new object.
Expand Down
16 changes: 11 additions & 5 deletions plumbing/format/packfile/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,20 @@ type Decoder struct {
// If the ObjectStorer implements storer.Transactioner, a transaction is created
// during the Decode execution. If anything fails, Rollback is called
func NewDecoder(s *Scanner, o storer.EncodedObjectStorer) (*Decoder, error) {
return NewDecoderForType(s, o, plumbing.AnyObject)
return NewDecoderForType(s, o, plumbing.AnyObject,
cache.NewObjectLRUDefault())
}

// NewDecoderForType returns a new Decoder but in this case for a specific object type.
// When an object is read using this Decoder instance and it is not of the same type of
// the specified one, nil will be returned. This is intended to avoid the content
// deserialization of all the objects
// deserialization of all the objects.
//
// cacheObject is an ObjectLRU that is used to speed up the process. If cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache instance can be any implementation of cache.Object

// is not needed you can pass nil. To create a cache object with the default
// size you an use the helper cache.ObjectLRUDefault().
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/an/can/

func NewDecoderForType(s *Scanner, o storer.EncodedObjectStorer,
t plumbing.ObjectType) (*Decoder, error) {
t plumbing.ObjectType, cacheObject cache.Object) (*Decoder, error) {

if t == plumbing.OFSDeltaObject ||
t == plumbing.REFDeltaObject ||
Expand All @@ -101,8 +106,9 @@ func NewDecoderForType(s *Scanner, o storer.EncodedObjectStorer,
}

return &Decoder{
s: s,
o: o,
s: s,
o: o,
DeltaBaseCache: cacheObject,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make DeltaBaseCache private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed these issues in the following commits.


idx: NewIndex(0),
offsetToType: make(map[int64]plumbing.ObjectType),
Expand Down
23 changes: 16 additions & 7 deletions plumbing/format/packfile/decoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"io"

"gopkg.in/src-d/go-git.v4/plumbing"
"gopkg.in/src-d/go-git.v4/plumbing/cache"
"gopkg.in/src-d/go-git.v4/plumbing/format/idxfile"
"gopkg.in/src-d/go-git.v4/plumbing/format/packfile"
"gopkg.in/src-d/go-git.v4/plumbing/storer"
Expand Down Expand Up @@ -51,7 +52,8 @@ func (s *ReaderSuite) TestDecodeByTypeRefDelta(c *C) {

storage := memory.NewStorage()
scanner := packfile.NewScanner(f.Packfile())
d, err := packfile.NewDecoderForType(scanner, storage, plumbing.CommitObject)
d, err := packfile.NewDecoderForType(scanner, storage, plumbing.CommitObject,
cache.NewObjectLRUDefault())
c.Assert(err, IsNil)

// Index required to decode by ref-delta.
Expand All @@ -77,7 +79,8 @@ func (s *ReaderSuite) TestDecodeByTypeRefDeltaError(c *C) {
fixtures.Basic().ByTag("ref-delta").Test(c, func(f *fixtures.Fixture) {
storage := memory.NewStorage()
scanner := packfile.NewScanner(f.Packfile())
d, err := packfile.NewDecoderForType(scanner, storage, plumbing.CommitObject)
d, err := packfile.NewDecoderForType(scanner, storage,
plumbing.CommitObject, cache.NewObjectLRUDefault())
c.Assert(err, IsNil)

defer d.Close()
Expand Down Expand Up @@ -111,7 +114,8 @@ func (s *ReaderSuite) TestDecodeByType(c *C) {
for _, t := range ts {
storage := memory.NewStorage()
scanner := packfile.NewScanner(f.Packfile())
d, err := packfile.NewDecoderForType(scanner, storage, t)
d, err := packfile.NewDecoderForType(scanner, storage, t,
cache.NewObjectLRUDefault())
c.Assert(err, IsNil)

// when the packfile is ref-delta based, the offsets are required
Expand Down Expand Up @@ -141,13 +145,17 @@ func (s *ReaderSuite) TestDecodeByTypeConstructor(c *C) {
storage := memory.NewStorage()
scanner := packfile.NewScanner(f.Packfile())

_, err := packfile.NewDecoderForType(scanner, storage, plumbing.OFSDeltaObject)
_, err := packfile.NewDecoderForType(scanner, storage,
plumbing.OFSDeltaObject, cache.NewObjectLRUDefault())
c.Assert(err, Equals, plumbing.ErrInvalidType)

_, err = packfile.NewDecoderForType(scanner, storage, plumbing.REFDeltaObject)
_, err = packfile.NewDecoderForType(scanner, storage,
plumbing.REFDeltaObject, cache.NewObjectLRUDefault())

c.Assert(err, Equals, plumbing.ErrInvalidType)

_, err = packfile.NewDecoderForType(scanner, storage, plumbing.InvalidObject)
_, err = packfile.NewDecoderForType(scanner, storage, plumbing.InvalidObject,
cache.NewObjectLRUDefault())
c.Assert(err, Equals, plumbing.ErrInvalidType)
}

Expand Down Expand Up @@ -313,7 +321,8 @@ func (s *ReaderSuite) TestDecodeObjectAt(c *C) {
func (s *ReaderSuite) TestDecodeObjectAtForType(c *C) {
f := fixtures.Basic().One()
scanner := packfile.NewScanner(f.Packfile())
d, err := packfile.NewDecoderForType(scanner, nil, plumbing.TreeObject)
d, err := packfile.NewDecoderForType(scanner, nil, plumbing.TreeObject,
cache.NewObjectLRUDefault())
c.Assert(err, IsNil)

// when the packfile is ref-delta based, the offsets are required
Expand Down
7 changes: 2 additions & 5 deletions storage/filesystem/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
"gopkg.in/src-d/go-billy.v4"
)

const DefaultMaxDeltaBaseCacheSize = 92 * cache.MiByte

type ObjectStorage struct {
// DeltaBaseCache is an object cache uses to cache delta's bases when
DeltaBaseCache cache.Object
Expand All @@ -30,7 +28,7 @@ type ObjectStorage struct {

func newObjectStorage(dir *dotgit.DotGit) (ObjectStorage, error) {
s := ObjectStorage{
DeltaBaseCache: cache.NewObjectLRU(DefaultMaxDeltaBaseCacheSize),
DeltaBaseCache: cache.NewObjectLRUDefault(),
dir: dir,
}

Expand Down Expand Up @@ -433,13 +431,12 @@ func newPackfileIter(f billy.File, t plumbing.ObjectType, seen map[plumbing.Hash
return nil, err
}

d, err := packfile.NewDecoderForType(s, memory.NewStorage(), t)
d, err := packfile.NewDecoderForType(s, memory.NewStorage(), t, cache)
if err != nil {
return nil, err
}

d.SetIndex(index)
d.DeltaBaseCache = cache

return &packfileIter{
f: f,
Expand Down