From c294666406f31c3b6c16d8f47ee7b30c39f6b992 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Sat, 25 Jan 2025 20:38:05 +0000 Subject: [PATCH 1/5] build: Add benchmark for open/read ops This benchmark aims to compare the time and space complexity across the different billy implementations, which will later be used to assess their trends over time. The initial figures shows that there is a substantial difference in cost for the Open operation amongst the os-based options, however the impact on Read operations is more subtle. As expected, in memory has the best figures for most cases. goos: linux goarch: amd64 pkg: github.com/go-git/go-billy/v6/test cpu: AMD Ryzen 7 PRO 8840HS w/ Radeon 780M Graphics BenchmarkOpenRead/stdlib_open-16 319782 3750 ns/op 152 B/op 3 allocs/op BenchmarkOpenRead/osfs.chrootOS_open-16 248833 4620 ns/op 360 B/op 9 allocs/op BenchmarkOpenRead/osfs.boundOS_open-16 182808 6370 ns/op 984 B/op 20 allocs/op BenchmarkOpenRead/memfs_open-16 1000000 1038 ns/op 224 B/op 10 allocs/op BenchmarkOpenRead/stdlib_read-16 165465 6791 ns/op 150.79 MB/s 0 B/op 0 allocs/op BenchmarkOpenRead/osfs.chrootOS_read-16 158107 7018 ns/op 145.90 MB/s 0 B/op 0 allocs/op BenchmarkOpenRead/osfs.boundOS_read-16 155649 6865 ns/op 149.15 MB/s 0 B/op 0 allocs/op BenchmarkOpenRead/memfs_read-16 705140 1703 ns/op 601.17 MB/s 0 B/op 0 allocs/op PASS ok github.com/go-git/go-billy/v6/test 34.080s Signed-off-by: Paulo Gomes --- test/bench_test.go | 149 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 test/bench_test.go diff --git a/test/bench_test.go b/test/bench_test.go new file mode 100644 index 0000000..49dd178 --- /dev/null +++ b/test/bench_test.go @@ -0,0 +1,149 @@ +package test + +import ( + "crypto/rand" + "io" + "os" + "testing" + + "github.com/go-git/go-billy/v6" + "github.com/go-git/go-billy/v6/memfs" + "github.com/go-git/go-billy/v6/osfs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + fn = "test" + contentSize = 1024 * 10 + bufSize = 1024 +) + +type test struct { + name string + fn string + sut billy.Filesystem + openF func(billy.Filesystem) func(string) (io.ReadSeekCloser, error) + createF func(*testing.B, billy.Filesystem, string) (io.WriteCloser, error) +} + +func BenchmarkOpenRead(b *testing.B) { + tests := []test{ + { + // provide baseline comparison against direct use of os. + name: "stdlib", + fn: filepath.Join(b.TempDir(), fn), + openF: stdlibOpen, + createF: stdlibCreate, + }, + { + name: "osfs.chrootOS", + fn: fn, + sut: osfs.New(b.TempDir(), osfs.WithChrootOS()), + openF: billyOpen, + createF: billyCreate, + }, + { + name: "osfs.boundOS", + fn: fn, + sut: osfs.New(b.TempDir(), osfs.WithBoundOS()), + openF: billyOpen, + createF: billyCreate, + }, + { + name: "memfs", + fn: fn, + sut: memfs.New(), + openF: billyOpen, + createF: billyCreate, + }, + } + + for _, tc := range tests { + f, err := tc.createF(b, tc.sut, tc.fn) + require.NoError(b, err) + assert.NotNil(b, f) + + prepFS(b, f) + b.Run(tc.name+"_open", open(tc.fn, tc.openF(tc.sut))) + } + + for _, tc := range tests { + b.Run(tc.name+"_read", read(tc.fn, tc.openF(tc.sut))) + } +} + +func open(n string, of func(string) (io.ReadSeekCloser, error)) func(b *testing.B) { + return func(b *testing.B) { + for i := 0; i < b.N; i++ { + f, err := of(n) + require.NoError(b, err) + assert.NotNil(b, f) + + b.StopTimer() + err = f.Close() + require.NoError(b, err) + b.StartTimer() + } + } +} + +func read(n string, of func(string) (io.ReadSeekCloser, error)) func(b *testing.B) { + return func(b *testing.B) { + b.StopTimer() + + buf := make([]byte, 1024) + f, err := of(n) + require.NoError(b, err) + assert.NotNil(b, f) + + b.StartTimer() + for i := 0; i < b.N; i++ { + _, err = f.Seek(0, io.SeekStart) + require.NoError(b, err) + + for { + n, err := f.Read(buf) + if n == 0 { + break + } + b.SetBytes(int64(n)) + require.NoError(b, err) + } + } + + err = f.Close() + require.NoError(b, err) + } +} + +func prepFS(b *testing.B, f io.WriteCloser) { + defer f.Close() + + content := make([]byte, contentSize) + _, err := rand.Read(content) + require.NoError(b, err, "failed to generate random content") + + _, err = f.Write(content) + require.NoError(b, err, "failed to write test file") +} + +func stdlibOpen(_ billy.Filesystem) func(n string) (io.ReadSeekCloser, error) { + return func(n string) (io.ReadSeekCloser, error) { + return os.Open(n) + } +} + +func billyOpen(fs billy.Filesystem) func(n string) (io.ReadSeekCloser, error) { + return func(n string) (io.ReadSeekCloser, error) { + return fs.Open(n) + } +} + +func stdlibCreate(_ *testing.B, _ billy.Filesystem, n string) (io.WriteCloser, error) { + return os.Create(n) +} + +func billyCreate(b *testing.B, fs billy.Filesystem, n string) (io.WriteCloser, error) { + return fs.Create(n) +} From 529dad71e5e0c9e1af30c318f2d4ab040207818e Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Sat, 25 Jan 2025 20:52:02 +0000 Subject: [PATCH 2/5] boundos: Remove redundant call to expandDot Signed-off-by: Paulo Gomes --- osfs/os_bound.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osfs/os_bound.go b/osfs/os_bound.go index 8cdade5..bac37e4 100644 --- a/osfs/os_bound.go +++ b/osfs/os_bound.go @@ -34,6 +34,8 @@ import ( var ( ErrBaseDirCannotBeRemoved = errors.New("base dir cannot be removed") ErrBaseDirCannotBeRenamed = errors.New("base dir cannot be renamed") + + dotPrefixes = []string{"./", ".\\"} ) // BoundOS is a fs implementation based on the OS filesystem which is bound to @@ -66,6 +68,7 @@ func (fs *BoundOS) OpenFile(filename string, flag int, perm fs.FileMode) (billy. if err != nil { return nil, err } + return openFile(fn, flag, perm, fs.createDir) } @@ -113,7 +116,6 @@ func (fs *BoundOS) MkdirAll(path string, perm fs.FileMode) error { } func (fs *BoundOS) Open(filename string) (billy.File, error) { - filename = fs.expandDot(filename) return fs.OpenFile(filename, os.O_RDONLY, 0) } @@ -187,7 +189,7 @@ func (fs *BoundOS) expandDot(p string) string { if p == "." { return fs.baseDir } - for _, prefix := range []string{"./", ".\\"} { + for _, prefix := range dotPrefixes { if strings.HasPrefix(p, prefix) { return filepath.Join(fs.baseDir, strings.TrimPrefix(p, prefix)) } From 12eb9b1238f59823e3ac73e28d248b47eeb634e7 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 27 Jan 2025 17:59:53 +0000 Subject: [PATCH 3/5] *: Expand tests covering fs.Rename Ahead of making changes to Rename, ensures that the behaviour across the built-in implementations are aligned by expanding tests, so that we avoid future regression. Signed-off-by: Paulo Gomes --- Makefile | 2 +- fs.go | 8 +-- osfs/os_bound.go | 18 ++++--- osfs/os_bound_test.go | 4 +- osfs/os_chroot.go | 7 ++- test/basic_test.go | 111 ++++++++++++++++++++++++++++++++++++------ 6 files changed, 121 insertions(+), 29 deletions(-) diff --git a/Makefile b/Makefile index 9178b97..e3a6480 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ $(GOLANGCI): .PHONY: test test: - $(GOTEST) -race ./... + $(GOTEST) -race -timeout 60s ./... test-coverage: echo "" > $(COVERAGE_REPORT); \ diff --git a/fs.go b/fs.go index 6907a6f..fe66099 100644 --- a/fs.go +++ b/fs.go @@ -8,9 +8,11 @@ import ( ) var ( - ErrReadOnly = errors.New("read-only filesystem") - ErrNotSupported = errors.New("feature not supported") - ErrCrossedBoundary = errors.New("chroot boundary crossed") + ErrReadOnly = errors.New("read-only filesystem") + ErrNotSupported = errors.New("feature not supported") + ErrCrossedBoundary = errors.New("chroot boundary crossed") + ErrBaseDirCannotBeRemoved = errors.New("base dir cannot be removed") + ErrBaseDirCannotBeRenamed = errors.New("base dir cannot be renamed") ) // Capability holds the supported features of a billy filesystem. This does diff --git a/osfs/os_bound.go b/osfs/os_bound.go index bac37e4..953cafc 100644 --- a/osfs/os_bound.go +++ b/osfs/os_bound.go @@ -20,7 +20,6 @@ package osfs import ( - "errors" "fmt" "io/fs" "os" @@ -32,9 +31,6 @@ import ( ) var ( - ErrBaseDirCannotBeRemoved = errors.New("base dir cannot be removed") - ErrBaseDirCannotBeRenamed = errors.New("base dir cannot be renamed") - dotPrefixes = []string{"./", ".\\"} ) @@ -84,15 +80,21 @@ func (fs *BoundOS) ReadDir(path string) ([]os.FileInfo, error) { func (fs *BoundOS) Rename(from, to string) error { if from == "." || from == fs.baseDir { - return ErrBaseDirCannotBeRenamed + return billy.ErrBaseDirCannotBeRenamed } from = fs.expandDot(from) - to = fs.expandDot(to) + _, err := fs.Lstat(from) + if err != nil { + return err + } + f, err := fs.abs(from) if err != nil { return err } + + to = fs.expandDot(to) t, err := fs.abs(to) if err != nil { return err @@ -130,7 +132,7 @@ func (fs *BoundOS) Stat(filename string) (os.FileInfo, error) { func (fs *BoundOS) Remove(filename string) error { if filename == "." || filename == fs.baseDir { - return ErrBaseDirCannotBeRemoved + return billy.ErrBaseDirCannotBeRemoved } fn, err := fs.abs(filename) @@ -161,7 +163,7 @@ func (fs *BoundOS) Join(elem ...string) string { func (fs *BoundOS) RemoveAll(path string) error { if path == "." || path == fs.baseDir { - return ErrBaseDirCannotBeRemoved + return billy.ErrBaseDirCannotBeRemoved } path = fs.expandDot(path) diff --git a/osfs/os_bound_test.go b/osfs/os_bound_test.go index 29539bd..78273d3 100644 --- a/osfs/os_bound_test.go +++ b/osfs/os_bound_test.go @@ -1293,10 +1293,10 @@ func TestRename(t *testing.T) { assert.NotNil(fi) err = fs.Rename(filepath.FromSlash("/tmp/outside/cwd/file1"), newFile) - require.ErrorContains(t, err, notFoundError()) + require.ErrorIs(t, err, os.ErrNotExist) err = fs.Rename(oldFile, filepath.FromSlash("/tmp/outside/cwd/file2")) - require.ErrorContains(t, err, notFoundError()) + require.ErrorIs(t, err, os.ErrNotExist) } func mustExist(filename string) { diff --git a/osfs/os_chroot.go b/osfs/os_chroot.go index 9545633..aefd341 100644 --- a/osfs/os_chroot.go +++ b/osfs/os_chroot.go @@ -52,7 +52,12 @@ func (fs *ChrootOS) ReadDir(dir string) ([]os.FileInfo, error) { } func (fs *ChrootOS) Rename(from, to string) error { - if err := fs.createDir(to); err != nil { + _, err := fs.Lstat(from) + if err != nil { + return err + } + + if err = fs.createDir(to); err != nil { return err } diff --git a/test/basic_test.go b/test/basic_test.go index 77b18ff..3db7831 100644 --- a/test/basic_test.go +++ b/test/basic_test.go @@ -4,11 +4,17 @@ import ( "bytes" "fmt" "io" + stdfs "io/fs" "os" "path/filepath" + "reflect" + "slices" + "strings" "testing" + "github.com/go-git/go-billy/v6" . "github.com/go-git/go-billy/v6" //nolint + "github.com/go-git/go-billy/v6/osfs" "github.com/go-git/go-billy/v6/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -469,24 +475,101 @@ func TestStatNonExistent(t *testing.T) { } func TestRename(t *testing.T) { - eachBasicFS(t, func(t *testing.T, fs Basic) { - t.Helper() - err := util.WriteFile(fs, "foo", nil, 0644) - require.NoError(t, err) - - err = fs.Rename("foo", "bar") - require.NoError(t, err) - - foo, err := fs.Stat("foo") - assert.Nil(t, foo) - assert.ErrorIs(t, err, os.ErrNotExist) + tests := []struct { + name string + before func(*testing.T, billy.Filesystem) + from string + to string + wantErr *error + wantFiles []string + }{ + { + name: "from not found", + from: "foo", + to: "bar", + wantErr: &os.ErrNotExist, + }, + { + name: "file rename", + before: func(t *testing.T, fs billy.Filesystem) { + root := fsRoot(fs) + _, err := fs.Create(fs.Join(root, "foo")) + require.NoError(t, err) + }, + from: "foo", + to: "bar", + wantFiles: []string{"/bar"}, + }, + { + name: "dir rename", + before: func(t *testing.T, fs billy.Filesystem) { + root := fsRoot(fs) + _, err := fs.Create(fs.Join(root, "foo", "bar1")) + require.NoError(t, err) + _, err = fs.Create(fs.Join(root, "foo", "bar2")) + require.NoError(t, err) + }, + from: "foo", + to: "bar", + wantFiles: []string{"/bar/bar1", "/bar/bar2"}, + }, + } - bar, err := fs.Stat("bar") - require.NoError(t, err) - assert.NotNil(t, bar) + eachFS(t, func(t *testing.T, fs Filesystem) { + t.Helper() + + root := fsRoot(fs) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if tc.before != nil { + tc.before(t, fs) + } + + err := fs.Rename(fs.Join(root, tc.from), fs.Join(root, tc.to)) + if tc.wantErr == nil { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, *tc.wantErr) + } + + err = util.Walk(fs, root, func(path string, fi stdfs.FileInfo, err error) error { + if err != nil { + return err + } + + if fi.IsDir() { + return nil + } + + if root != "/" { + path = strings.TrimPrefix(path, root) + } + if !slices.Contains(tc.wantFiles, path) { + assert.Fail(t, "file not found", "name", path) + } + + return nil + }) + require.NoError(t, err) + + fis, _ := fs.ReadDir(root) + for _, fi := range fis { + cpath := fs.Join(root, fi.Name()) + err := util.RemoveAll(fs, cpath) + require.NoError(t, err) + } + }) + } }) } +func fsRoot(fs billy.Filesystem) string { + if reflect.TypeOf(fs) == reflect.TypeOf(&osfs.BoundOS{}) { + return fs.Root() + } + return "/" +} + func TestOpenAndWrite(t *testing.T) { eachBasicFS(t, func(t *testing.T, fs Basic) { t.Helper() From a989507d72628328f203c648c6637dbb7f4bddf8 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Mon, 27 Jan 2025 22:26:21 +0000 Subject: [PATCH 4/5] memfs: Add thread-safety to Memory Make Memory thread safe for operations such as Create, Rename and Delete. When compared with Memory without a mutex, the cost per op is roughly 50ns, as per benchmark below: goos: linux goarch: amd64 pkg: github.com/go-git/go-billy/v6/test cpu: AMD Ryzen 7 PRO 8840HS w/ Radeon 780M Graphics BenchmarkCompare/stdlib_open-16 341277 3303 ns/op 152 B/op 3 allocs/op BenchmarkCompare/osfs.chrootOS_open-16 286580 3806 ns/op 360 B/op 9 allocs/op BenchmarkCompare/osfs.boundOS_open-16 209929 5311 ns/op 984 B/op 20 allocs/op BenchmarkCompare/memfs_open-16 1503024 967.4 ns/op 224 B/op 10 allocs/op BenchmarkCompare/memfs_mutex_open-16 1272890 911.6 ns/op 224 B/op 10 allocs/op BenchmarkCompare/stdlib_read-16 184956 5957 ns/op 171.88 MB/s 0 B/op 0 allocs/op BenchmarkCompare/osfs.chrootOS_read-16 180552 6207 ns/op 164.97 MB/s 0 B/op 0 allocs/op BenchmarkCompare/osfs.boundOS_read-16 179314 6131 ns/op 167.02 MB/s 0 B/op 0 allocs/op BenchmarkCompare/memfs_read-16 792040 1568 ns/op 653.02 MB/s 0 B/op 0 allocs/op BenchmarkCompare/memfs_mutex_read-16 783422 1609 ns/op 636.27 MB/s 0 B/op 0 allocs/op BenchmarkCompare/stdlib_create-16 216717 5232 ns/op 152 B/op 3 allocs/op BenchmarkCompare/osfs.chrootOS_create-16 157268 7017 ns/op 616 B/op 11 allocs/op BenchmarkCompare/osfs.boundOS_create-16 123038 9242 ns/op 1335 B/op 22 allocs/op BenchmarkCompare/memfs_create-16 880813 1954 ns/op 693 B/op 12 allocs/op BenchmarkCompare/memfs_mutex_create-16 559534 1901 ns/op 631 B/op 12 allocs/op PASS ok github.com/go-git/go-billy/v6/test 118.368s This change enables users to opt-out (memfs.WithoutMutex), in case speed on a single thread is more important than being thread-safe. Additional thread-safety will be required for memfs.file. Signed-off-by: Paulo Gomes --- .github/workflows/test.yml | 1 - Makefile | 2 +- memfs/file.go | 243 +++++++++++++++++++++++++++++++++++++ memfs/memory.go | 186 +++------------------------- memfs/memory_option.go | 15 +++ memfs/memory_test.go | 39 ++++++ memfs/mutex.go | 33 +++++ memfs/storage.go | 152 ++++++++++------------- test/basic_test.go | 23 ++-- test/bench_test.go | 42 ++++++- 10 files changed, 463 insertions(+), 273 deletions(-) create mode 100644 memfs/file.go create mode 100644 memfs/memory_option.go create mode 100644 memfs/mutex.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5e46d34..64202e0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -2,7 +2,6 @@ name: Test on: push: - branches: [ "master", "main" ] pull_request: permissions: {} diff --git a/Makefile b/Makefile index e3a6480..a8460af 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ $(GOLANGCI): .PHONY: test test: - $(GOTEST) -race -timeout 60s ./... + $(GOTEST) -race -timeout 300s ./... test-coverage: echo "" > $(COVERAGE_REPORT); \ diff --git a/memfs/file.go b/memfs/file.go new file mode 100644 index 0000000..7ebe76b --- /dev/null +++ b/memfs/file.go @@ -0,0 +1,243 @@ +package memfs + +import ( + "errors" + "io" + "io/fs" + "os" + "sync" + "time" + + "github.com/go-git/go-billy/v6" +) + +type file struct { + name string + content *content + position int64 + flag int + mode os.FileMode + modTime time.Time + + isClosed bool +} + +func (f *file) Name() string { + return f.name +} + +func (f *file) Read(b []byte) (int, error) { + n, err := f.ReadAt(b, f.position) + f.position += int64(n) + + if errors.Is(err, io.EOF) && n != 0 { + err = nil + } + + return n, err +} + +func (f *file) ReadAt(b []byte, off int64) (int, error) { + if f.isClosed { + return 0, os.ErrClosed + } + + if !isReadAndWrite(f.flag) && !isReadOnly(f.flag) { + return 0, errors.New("read not supported") + } + + n, err := f.content.ReadAt(b, off) + + return n, err +} + +func (f *file) Seek(offset int64, whence int) (int64, error) { + if f.isClosed { + return 0, os.ErrClosed + } + + switch whence { + case io.SeekCurrent: + f.position += offset + case io.SeekStart: + f.position = offset + case io.SeekEnd: + f.position = int64(f.content.Len()) + offset + } + + return f.position, nil +} + +func (f *file) Write(p []byte) (int, error) { + return f.WriteAt(p, f.position) +} + +func (f *file) WriteAt(p []byte, off int64) (int, error) { + if f.isClosed { + return 0, os.ErrClosed + } + + if !isReadAndWrite(f.flag) && !isWriteOnly(f.flag) { + return 0, errors.New("write not supported") + } + + f.modTime = time.Now() + n, err := f.content.WriteAt(p, off) + f.position = off + int64(n) + + return n, err +} + +func (f *file) Close() error { + if f.isClosed { + return os.ErrClosed + } + + f.isClosed = true + return nil +} + +func (f *file) Truncate(size int64) error { + if size < int64(len(f.content.bytes)) { + f.content.bytes = f.content.bytes[:size] + } else if more := int(size) - len(f.content.bytes); more > 0 { + f.content.bytes = append(f.content.bytes, make([]byte, more)...) + } + + return nil +} + +func (f *file) Duplicate(filename string, mode fs.FileMode, flag int) billy.File { + n := &file{ + name: filename, + content: f.content, + mode: mode, + flag: flag, + modTime: f.modTime, + } + + if isTruncate(flag) { + n.content.Truncate() + } + + if isAppend(flag) { + n.position = int64(n.content.Len()) + } + + return n +} + +func (f *file) Stat() (os.FileInfo, error) { + return &fileInfo{ + name: f.Name(), + mode: f.mode, + size: f.content.Len(), + modTime: f.modTime, + }, nil +} + +// Lock is a no-op in memfs. +func (f *file) Lock() error { + return nil +} + +// Unlock is a no-op in memfs. +func (f *file) Unlock() error { + return nil +} + +type fileInfo struct { + name string + size int + mode os.FileMode + modTime time.Time +} + +func (fi *fileInfo) Name() string { + return fi.name +} + +func (fi *fileInfo) Size() int64 { + return int64(fi.size) +} + +func (fi *fileInfo) Mode() fs.FileMode { + return fi.mode +} + +func (fi *fileInfo) ModTime() time.Time { + return fi.modTime +} + +func (fi *fileInfo) IsDir() bool { + return fi.mode.IsDir() +} + +func (*fileInfo) Sys() interface{} { + return nil +} + +type content struct { + name string + bytes []byte + + m sync.RWMutex +} + +func (c *content) WriteAt(p []byte, off int64) (int, error) { + if off < 0 { + return 0, &os.PathError{ + Op: "writeat", + Path: c.name, + Err: errors.New("negative offset"), + } + } + + c.m.Lock() + prev := len(c.bytes) + + diff := int(off) - prev + if diff > 0 { + c.bytes = append(c.bytes, make([]byte, diff)...) + } + + c.bytes = append(c.bytes[:off], p...) + if len(c.bytes) < prev { + c.bytes = c.bytes[:prev] + } + c.m.Unlock() + + return len(p), nil +} + +func (c *content) ReadAt(b []byte, off int64) (n int, err error) { + if off < 0 { + return 0, &os.PathError{ + Op: "readat", + Path: c.name, + Err: errors.New("negative offset"), + } + } + + c.m.RLock() + size := int64(len(c.bytes)) + if off >= size { + c.m.RUnlock() + return 0, io.EOF + } + + l := int64(len(b)) + if off+l > size { + l = size - off + } + + btr := c.bytes[off : off+l] + n = copy(b, btr) + + if len(btr) < len(b) { + err = io.EOF + } + c.m.RUnlock() + + return +} diff --git a/memfs/memory.go b/memfs/memory.go index 2f70e96..c69eeed 100644 --- a/memfs/memory.go +++ b/memfs/memory.go @@ -4,7 +4,6 @@ package memfs // import "github.com/go-git/go-billy/v6/memfs" import ( "errors" "fmt" - "io" "io/fs" "log" "os" @@ -12,7 +11,6 @@ import ( "sort" "strings" "syscall" - "time" "github.com/go-git/go-billy/v6" "github.com/go-git/go-billy/v6/helper/chroot" @@ -24,11 +22,25 @@ const separator = filepath.Separator // Memory a very convenient filesystem based on memory files. type Memory struct { s *storage + + m mutex } // New returns a new Memory filesystem. -func New() billy.Filesystem { - fs := &Memory{s: newStorage()} +func New(opts ...Option) billy.Filesystem { + o := &options{ + // Enable thread-safety by default. + newMutex: newMutex, + } + + for _, opt := range opts { + opt(o) + } + + fs := &Memory{ + s: newStorage(o.newMutex), + m: o.newMutex(), + } _, err := fs.s.New("/", 0755|os.ModeDir, 0) if err != nil { log.Printf("failed to create root dir: %v", err) @@ -217,172 +229,6 @@ func (fs *Memory) Capabilities() billy.Capability { billy.TruncateCapability } -type file struct { - name string - content *content - position int64 - flag int - mode os.FileMode - modTime time.Time - - isClosed bool -} - -func (f *file) Name() string { - return f.name -} - -func (f *file) Read(b []byte) (int, error) { - n, err := f.ReadAt(b, f.position) - f.position += int64(n) - - if errors.Is(err, io.EOF) && n != 0 { - err = nil - } - - return n, err -} - -func (f *file) ReadAt(b []byte, off int64) (int, error) { - if f.isClosed { - return 0, os.ErrClosed - } - - if !isReadAndWrite(f.flag) && !isReadOnly(f.flag) { - return 0, errors.New("read not supported") - } - - n, err := f.content.ReadAt(b, off) - - return n, err -} - -func (f *file) Seek(offset int64, whence int) (int64, error) { - if f.isClosed { - return 0, os.ErrClosed - } - - switch whence { - case io.SeekCurrent: - f.position += offset - case io.SeekStart: - f.position = offset - case io.SeekEnd: - f.position = int64(f.content.Len()) + offset - } - - return f.position, nil -} - -func (f *file) Write(p []byte) (int, error) { - return f.WriteAt(p, f.position) -} - -func (f *file) WriteAt(p []byte, off int64) (int, error) { - if f.isClosed { - return 0, os.ErrClosed - } - - if !isReadAndWrite(f.flag) && !isWriteOnly(f.flag) { - return 0, errors.New("write not supported") - } - - f.modTime = time.Now() - n, err := f.content.WriteAt(p, off) - f.position = off + int64(n) - - return n, err -} - -func (f *file) Close() error { - if f.isClosed { - return os.ErrClosed - } - - f.isClosed = true - return nil -} - -func (f *file) Truncate(size int64) error { - if size < int64(len(f.content.bytes)) { - f.content.bytes = f.content.bytes[:size] - } else if more := int(size) - len(f.content.bytes); more > 0 { - f.content.bytes = append(f.content.bytes, make([]byte, more)...) - } - - return nil -} - -func (f *file) Duplicate(filename string, mode fs.FileMode, flag int) billy.File { - nf := &file{ - name: filename, - content: f.content, - mode: mode, - flag: flag, - modTime: f.modTime, - } - - if isTruncate(flag) { - nf.content.Truncate() - } - - if isAppend(flag) { - nf.position = int64(nf.content.Len()) - } - - return nf -} - -func (f *file) Stat() (os.FileInfo, error) { - return &fileInfo{ - name: f.Name(), - mode: f.mode, - size: f.content.Len(), - modTime: f.modTime, - }, nil -} - -// Lock is a no-op in memfs. -func (f *file) Lock() error { - return nil -} - -// Unlock is a no-op in memfs. -func (f *file) Unlock() error { - return nil -} - -type fileInfo struct { - name string - size int - mode os.FileMode - modTime time.Time -} - -func (fi *fileInfo) Name() string { - return fi.name -} - -func (fi *fileInfo) Size() int64 { - return int64(fi.size) -} - -func (fi *fileInfo) Mode() fs.FileMode { - return fi.mode -} - -func (fi *fileInfo) ModTime() time.Time { - return fi.modTime -} - -func (fi *fileInfo) IsDir() bool { - return fi.mode.IsDir() -} - -func (*fileInfo) Sys() interface{} { - return nil -} - func (c *content) Truncate() { c.bytes = make([]byte, 0) } diff --git a/memfs/memory_option.go b/memfs/memory_option.go new file mode 100644 index 0000000..3d7170a --- /dev/null +++ b/memfs/memory_option.go @@ -0,0 +1,15 @@ +package memfs + +type Option func(*options) + +type options struct { + newMutex func() mutex +} + +// WithoutMutex disables thread safety. This is a temporary option +// for users to opt-out from the default setting. +func WithoutMutex() Option { + return func(o *options) { + o.newMutex = newNoOpMutex + } +} diff --git a/memfs/memory_test.go b/memfs/memory_test.go index 75157ab..96391c7 100644 --- a/memfs/memory_test.go +++ b/memfs/memory_test.go @@ -6,6 +6,7 @@ import ( "io/fs" "os" "runtime" + "sync" "testing" "time" @@ -357,3 +358,41 @@ func TestSymlink(t *testing.T) { require.NoError(t, err) assert.Nil(t, fi) } + +func TestThreadSafety(t *testing.T) { + fs := New() + + var wg sync.WaitGroup + files := 100 + + fnc := func(n int, s string, remove bool) { + fn := fmt.Sprintf("/file_%d%s", n, s) + f, err := fs.Create(fn) + require.NoError(t, err) + require.NotNil(t, f) + + err = fs.Rename(fn, fn+"2") + require.NoError(t, err) + + if remove { + err = fs.Remove(fn + "2") + require.NoError(t, err) + } + wg.Done() + } + + for i := 0; i < files; i++ { + wg.Add(4) + + go fnc(i, "a", false) + go fnc(i, "b", false) + go fnc(i, "c", true) + go fnc(i, "d", true) + } + + wg.Wait() + + fi, err := fs.ReadDir("/") + require.NoError(t, err) + assert.Len(t, fi, files*2) +} diff --git a/memfs/mutex.go b/memfs/mutex.go new file mode 100644 index 0000000..4b84da2 --- /dev/null +++ b/memfs/mutex.go @@ -0,0 +1,33 @@ +package memfs + +import "sync" + +func newMutex() mutex { + return &sync.RWMutex{} +} + +func newNoOpMutex() mutex { + return &sync.RWMutex{} +} + +type mutex interface { + Lock() + Unlock() + RLock() + RUnlock() +} + +type noOpMutex struct { //nolint:unused +} + +func (noOpMutex) Lock() { //nolint:unused +} + +func (noOpMutex) Unlock() { //nolint:unused +} + +func (noOpMutex) RLock() { //nolint:unused +} + +func (noOpMutex) RUnlock() { //nolint:unused +} diff --git a/memfs/storage.go b/memfs/storage.go index c0830e5..868d525 100644 --- a/memfs/storage.go +++ b/memfs/storage.go @@ -1,40 +1,42 @@ package memfs import ( - "errors" "fmt" - "io" "io/fs" "os" "path/filepath" "strings" - "sync" "time" + + "github.com/go-git/go-billy/v6" ) type storage struct { files map[string]*file children map[string]map[string]*file + + mf mutex + mc mutex } -func newStorage() *storage { +func newStorage(newMutex func() mutex) *storage { return &storage{ files: make(map[string]*file, 0), children: make(map[string]map[string]*file, 0), + mc: newMutex(), + mf: newMutex(), } } func (s *storage) Has(path string) bool { - path = clean(path) - - _, ok := s.files[path] + _, ok := s.get(path) return ok } func (s *storage) New(path string, mode fs.FileMode, flag int) (*file, error) { path = clean(path) - if s.Has(path) { - if !s.MustGet(path).mode.IsDir() { + if f, ok := s.get(path); ok { + if !f.mode.IsDir() { return nil, fmt.Errorf("file already exists %q", path) } @@ -42,7 +44,6 @@ func (s *storage) New(path string, mode fs.FileMode, flag int) (*file, error) { } name := filepath.Base(path) - f := &file{ name: name, content: &content{name: name}, @@ -51,7 +52,10 @@ func (s *storage) New(path string, mode fs.FileMode, flag int) (*file, error) { modTime: time.Now(), } + s.mf.Lock() s.files[path] = f + s.mf.Unlock() + err := s.createParent(path, mode, f) if err != nil { return nil, fmt.Errorf("failed to create parent: %w", err) @@ -71,27 +75,32 @@ func (s *storage) createParent(path string, mode fs.FileMode, f *file) error { return err } + s.mc.Lock() if _, ok := s.children[base]; !ok { s.children[base] = make(map[string]*file, 0) } s.children[base][f.Name()] = f + s.mc.Unlock() + return nil } func (s *storage) Children(path string) []*file { path = clean(path) - l := make([]*file, 0) + s.mc.RLock() + l := make([]*file, 0, len(s.children)) for _, f := range s.children[path] { l = append(l, f) } + s.mc.RUnlock() return l } func (s *storage) MustGet(path string) *file { - f, ok := s.Get(path) + f, ok := s.get(path) if !ok { panic(fmt.Errorf("couldn't find %q", path)) } @@ -100,12 +109,19 @@ func (s *storage) MustGet(path string) *file { } func (s *storage) Get(path string) (*file, bool) { + return s.get(path) +} + +func (s *storage) get(path string) (*file, bool) { path = clean(path) - if !s.Has(path) { + + s.mf.RLock() + file, ok := s.files[path] + s.mf.RUnlock() + if !ok { return nil, false } - file, ok := s.files[path] return file, ok } @@ -113,12 +129,16 @@ func (s *storage) Rename(from, to string) error { from = clean(from) to = clean(to) + if from == "/" || from == "." { + return billy.ErrBaseDirCannotBeRenamed + } + if !s.Has(from) { return os.ErrNotExist } move := [][2]string{{from, to}} - + s.mf.RLock() for pathFrom := range s.files { if pathFrom == from || !strings.HasPrefix(pathFrom, from) { continue @@ -129,6 +149,7 @@ func (s *storage) Rename(from, to string) error { move = append(move, [2]string{pathFrom, pathTo}) } + s.mf.RUnlock() for _, ops := range move { from := ops[0] @@ -143,104 +164,63 @@ func (s *storage) Rename(from, to string) error { } func (s *storage) move(from, to string) error { + s.mf.Lock() s.files[to] = s.files[from] s.files[to].name = filepath.Base(to) + file := s.files[to] + s.mf.Unlock() + + s.mc.Lock() s.children[to] = s.children[from] + s.mc.Unlock() defer func() { - delete(s.children, from) + s.mf.Lock() delete(s.files, from) + s.mf.Unlock() + + s.mc.Lock() + delete(s.children, from) delete(s.children[filepath.Dir(from)], filepath.Base(from)) + s.mc.Unlock() }() - return s.createParent(to, 0644, s.files[to]) + return s.createParent(to, 0644, file) } func (s *storage) Remove(path string) error { path = clean(path) + if path == "/" || path == "." { + return billy.ErrBaseDirCannotBeRemoved + } - f, has := s.Get(path) + f, has := s.get(path) if !has { return os.ErrNotExist } - if f.mode.IsDir() && len(s.children[path]) != 0 { - return fmt.Errorf("dir: %s contains files", path) + if f.mode.IsDir() { + s.mc.RLock() + if len(s.children[path]) != 0 { + s.mc.RUnlock() + return fmt.Errorf("dir: %s contains files", path) + } + s.mc.RUnlock() } base, file := filepath.Split(path) base = filepath.Clean(base) - delete(s.children[base], file) + s.mf.Lock() delete(s.files, path) + s.mf.Unlock() + + s.mc.Lock() + delete(s.children[base], file) + s.mc.Unlock() return nil } func clean(path string) string { return filepath.Clean(filepath.FromSlash(path)) } - -type content struct { - name string - bytes []byte - - m sync.RWMutex -} - -func (c *content) WriteAt(p []byte, off int64) (int, error) { - if off < 0 { - return 0, &os.PathError{ - Op: "writeat", - Path: c.name, - Err: errors.New("negative offset"), - } - } - - c.m.Lock() - prev := len(c.bytes) - - diff := int(off) - prev - if diff > 0 { - c.bytes = append(c.bytes, make([]byte, diff)...) - } - - c.bytes = append(c.bytes[:off], p...) - if len(c.bytes) < prev { - c.bytes = c.bytes[:prev] - } - c.m.Unlock() - - return len(p), nil -} - -func (c *content) ReadAt(b []byte, off int64) (n int, err error) { - if off < 0 { - return 0, &os.PathError{ - Op: "readat", - Path: c.name, - Err: errors.New("negative offset"), - } - } - - c.m.RLock() - size := int64(len(c.bytes)) - if off >= size { - c.m.RUnlock() - return 0, io.EOF - } - - l := int64(len(b)) - if off+l > size { - l = size - off - } - - btr := c.bytes[off : off+l] - n = copy(b, btr) - - if len(btr) < len(b) { - err = io.EOF - } - c.m.RUnlock() - - return -} diff --git a/test/basic_test.go b/test/basic_test.go index 3db7831..5b72b7a 100644 --- a/test/basic_test.go +++ b/test/basic_test.go @@ -493,25 +493,30 @@ func TestRename(t *testing.T) { name: "file rename", before: func(t *testing.T, fs billy.Filesystem) { root := fsRoot(fs) - _, err := fs.Create(fs.Join(root, "foo")) + f, err := fs.Create(fs.Join(root, "foo")) require.NoError(t, err) + require.NoError(t, f.Close()) }, from: "foo", to: "bar", - wantFiles: []string{"/bar"}, + wantFiles: []string{filepath.FromSlash("/bar")}, }, { name: "dir rename", before: func(t *testing.T, fs billy.Filesystem) { root := fsRoot(fs) - _, err := fs.Create(fs.Join(root, "foo", "bar1")) + f, err := fs.Create(fs.Join(root, "foo", "bar1")) require.NoError(t, err) - _, err = fs.Create(fs.Join(root, "foo", "bar2")) + require.NoError(t, f.Close()) + f, err = fs.Create(fs.Join(root, "foo", "bar2")) require.NoError(t, err) + require.NoError(t, f.Close()) }, - from: "foo", - to: "bar", - wantFiles: []string{"/bar/bar1", "/bar/bar2"}, + from: "foo", + to: "bar", + wantFiles: []string{ + filepath.FromSlash("/bar/bar1"), + filepath.FromSlash("/bar/bar2")}, }, } @@ -541,7 +546,7 @@ func TestRename(t *testing.T) { return nil } - if root != "/" { + if filepath.Dir(root) == "" { path = strings.TrimPrefix(path, root) } if !slices.Contains(tc.wantFiles, path) { @@ -567,7 +572,7 @@ func fsRoot(fs billy.Filesystem) string { if reflect.TypeOf(fs) == reflect.TypeOf(&osfs.BoundOS{}) { return fs.Root() } - return "/" + return string(filepath.Separator) } func TestOpenAndWrite(t *testing.T) { diff --git a/test/bench_test.go b/test/bench_test.go index 49dd178..53c561f 100644 --- a/test/bench_test.go +++ b/test/bench_test.go @@ -2,8 +2,10 @@ package test import ( "crypto/rand" + "fmt" "io" "os" + "path/filepath" "testing" "github.com/go-git/go-billy/v6" @@ -24,10 +26,10 @@ type test struct { fn string sut billy.Filesystem openF func(billy.Filesystem) func(string) (io.ReadSeekCloser, error) - createF func(*testing.B, billy.Filesystem, string) (io.WriteCloser, error) + createF func(billy.Filesystem, string) (io.WriteCloser, error) } -func BenchmarkOpenRead(b *testing.B) { +func BenchmarkCompare(b *testing.B) { tests := []test{ { // provide baseline comparison against direct use of os. @@ -53,6 +55,13 @@ func BenchmarkOpenRead(b *testing.B) { { name: "memfs", fn: fn, + sut: memfs.New(memfs.WithoutMutex()), + openF: billyOpen, + createF: billyCreate, + }, + { + name: "memfs_mutex", + fn: fn, sut: memfs.New(), openF: billyOpen, createF: billyCreate, @@ -60,7 +69,7 @@ func BenchmarkOpenRead(b *testing.B) { } for _, tc := range tests { - f, err := tc.createF(b, tc.sut, tc.fn) + f, err := tc.createF(tc.sut, tc.fn) require.NoError(b, err) assert.NotNil(b, f) @@ -71,6 +80,27 @@ func BenchmarkOpenRead(b *testing.B) { for _, tc := range tests { b.Run(tc.name+"_read", read(tc.fn, tc.openF(tc.sut))) } + + for _, tc := range tests { + b.Run(tc.name+"_create", create(tc.sut, tc.fn, tc.createF)) + } +} + +func create(fs billy.Filesystem, n string, nf func(billy.Filesystem, string) (io.WriteCloser, error)) func(b *testing.B) { + return func(b *testing.B) { + for i := 0; i < b.N; i++ { + fn := fmt.Sprintf("%s_%d", n, i) + b.StartTimer() + f, err := nf(fs, fn) + b.StopTimer() + + require.NoError(b, err) + assert.NotNil(b, f) + + err = f.Close() + require.NoError(b, err) + } + } } func open(n string, of func(string) (io.ReadSeekCloser, error)) func(b *testing.B) { @@ -130,7 +160,7 @@ func prepFS(b *testing.B, f io.WriteCloser) { func stdlibOpen(_ billy.Filesystem) func(n string) (io.ReadSeekCloser, error) { return func(n string) (io.ReadSeekCloser, error) { - return os.Open(n) + return os.OpenFile(n, os.O_RDONLY, 0o444) } } @@ -140,10 +170,10 @@ func billyOpen(fs billy.Filesystem) func(n string) (io.ReadSeekCloser, error) { } } -func stdlibCreate(_ *testing.B, _ billy.Filesystem, n string) (io.WriteCloser, error) { +func stdlibCreate(_ billy.Filesystem, n string) (io.WriteCloser, error) { return os.Create(n) } -func billyCreate(b *testing.B, fs billy.Filesystem, n string) (io.WriteCloser, error) { +func billyCreate(fs billy.Filesystem, n string) (io.WriteCloser, error) { return fs.Create(n) } From 7c5ae60cd3a87900f82dbbf00da825b4169abd6c Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 28 Jan 2025 21:32:53 +0000 Subject: [PATCH 5/5] memfs: Remove memfs.WithoutMutex Ensure memfs is thread-safe at all times. In case the single thread performance is more important (probably outside of go-git) that can be fixed with future optimisation. In the context of go-git, this should not be a concern as there are other bottlenecks (e.g. sha1cd). Signed-off-by: Paulo Gomes --- memfs/memory.go | 11 ++--------- memfs/memory_option.go | 9 --------- memfs/mutex.go | 33 --------------------------------- memfs/storage.go | 9 ++++----- test/bench_test.go | 7 ------- 5 files changed, 6 insertions(+), 63 deletions(-) delete mode 100644 memfs/mutex.go diff --git a/memfs/memory.go b/memfs/memory.go index c69eeed..fbf6c29 100644 --- a/memfs/memory.go +++ b/memfs/memory.go @@ -22,24 +22,17 @@ const separator = filepath.Separator // Memory a very convenient filesystem based on memory files. type Memory struct { s *storage - - m mutex } // New returns a new Memory filesystem. func New(opts ...Option) billy.Filesystem { - o := &options{ - // Enable thread-safety by default. - newMutex: newMutex, - } - + o := &options{} for _, opt := range opts { opt(o) } fs := &Memory{ - s: newStorage(o.newMutex), - m: o.newMutex(), + s: newStorage(), } _, err := fs.s.New("/", 0755|os.ModeDir, 0) if err != nil { diff --git a/memfs/memory_option.go b/memfs/memory_option.go index 3d7170a..bbdd22d 100644 --- a/memfs/memory_option.go +++ b/memfs/memory_option.go @@ -3,13 +3,4 @@ package memfs type Option func(*options) type options struct { - newMutex func() mutex -} - -// WithoutMutex disables thread safety. This is a temporary option -// for users to opt-out from the default setting. -func WithoutMutex() Option { - return func(o *options) { - o.newMutex = newNoOpMutex - } } diff --git a/memfs/mutex.go b/memfs/mutex.go deleted file mode 100644 index 4b84da2..0000000 --- a/memfs/mutex.go +++ /dev/null @@ -1,33 +0,0 @@ -package memfs - -import "sync" - -func newMutex() mutex { - return &sync.RWMutex{} -} - -func newNoOpMutex() mutex { - return &sync.RWMutex{} -} - -type mutex interface { - Lock() - Unlock() - RLock() - RUnlock() -} - -type noOpMutex struct { //nolint:unused -} - -func (noOpMutex) Lock() { //nolint:unused -} - -func (noOpMutex) Unlock() { //nolint:unused -} - -func (noOpMutex) RLock() { //nolint:unused -} - -func (noOpMutex) RUnlock() { //nolint:unused -} diff --git a/memfs/storage.go b/memfs/storage.go index 868d525..f939e56 100644 --- a/memfs/storage.go +++ b/memfs/storage.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" "github.com/go-git/go-billy/v6" @@ -15,16 +16,14 @@ type storage struct { files map[string]*file children map[string]map[string]*file - mf mutex - mc mutex + mf sync.RWMutex + mc sync.RWMutex } -func newStorage(newMutex func() mutex) *storage { +func newStorage() *storage { return &storage{ files: make(map[string]*file, 0), children: make(map[string]map[string]*file, 0), - mc: newMutex(), - mf: newMutex(), } } diff --git a/test/bench_test.go b/test/bench_test.go index 53c561f..6e33511 100644 --- a/test/bench_test.go +++ b/test/bench_test.go @@ -55,13 +55,6 @@ func BenchmarkCompare(b *testing.B) { { name: "memfs", fn: fn, - sut: memfs.New(memfs.WithoutMutex()), - openF: billyOpen, - createF: billyCreate, - }, - { - name: "memfs_mutex", - fn: fn, sut: memfs.New(), openF: billyOpen, createF: billyCreate,