memfs: Add thread-safety to Memory #110
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
memfs/memory.go
Outdated
| @@ -24,11 +22,25 @@ const separator = filepath.Separator | |||
| // Memory a very convenient filesystem based on memory files. | |||
| type Memory struct { | |||
| s *storage | |||
|
|
|||
| m mutex | |||
There was a problem hiding this comment.
Why not sync.RWMutext?
Suggested change
| m mutex | |
| m sync.RWMutext |
memfs/memory.go
Outdated
Comment on lines
31
to
34
| o := &options{ | ||
| // Enable thread-safety by default. | ||
| newMutex: newMutex, | ||
| } |
There was a problem hiding this comment.
I think we should always enable thread-safety
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 <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
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 <[email protected]>
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 <[email protected]>
memfs/memory.go
Outdated
Comment on lines
31
to
35
| func New(opts ...Option) billy.Filesystem { | ||
| o := &options{ | ||
| // Enable thread-safety by default. | ||
| newMutex: newMutex, | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we need opts ...Option and options?
There was a problem hiding this comment.
Yes, the plan is to align with the other osfs implementations in a future PR.
memfs/memory.go
Outdated
|
|
||
| fs := &Memory{ | ||
| s: newStorage(), | ||
| m: sync.RWMutex{}, |
There was a problem hiding this comment.
This can be omitted
Suggested change
| m: sync.RWMutex{}, |
There was a problem hiding this comment.
Good shout, let me amend that.
Comment on lines
+1
to
+6
| package memfs | ||
|
|
||
| type Option func(*options) | ||
|
|
||
| type options struct { | ||
| } |
There was a problem hiding this comment.
We can remove this altogether
Suggested change
| package memfs | |
| type Option func(*options) | |
| type options struct { | |
| } |
There was a problem hiding this comment.
I left this on purpose, the idea is to add additional options on a follow-up PR. Please note that the pattern aligns with both osfs implementations.
Comment on lines
+31
to
+32
| func New(opts ...Option) billy.Filesystem { | ||
| o := &options{} | ||
| for _, opt := range opts { | ||
| opt(o) | ||
| } |
There was a problem hiding this comment.
Suggested change
| func New(opts ...Option) billy.Filesystem { | |
| o := &options{} | |
| for _, opt := range opts { | |
| opt(o) | |
| } | |
| func New() billy.Filesystem { |
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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make
Memorythread-safe for operations such asCreate,RenameandDelete. When compared withMemorywithout a mutex, the cost per op is roughly50ns, as per benchmark below:Some minor changes to
Renamewere made onosfsimplementations, to ensure consistent behaviour across the built-inimplementations.
Additional thread-safety will be required for
memfs.file, which will be dealt with separately.Closes #40.
Relatest to go-git/go-git#773.
PS: The benchmark results were based before the removal of the Mutex option (last commit).