Skip to content

Commit

Permalink
Document performance caveats of ExtractV1File and address comments
Browse files Browse the repository at this point in the history
Since `CopyFileRange` performance is OS-dependant, we cannot guarantee
that `ExtractV1File` will keep copies out of user-space. For example, on
Linux with sufficiently old Kernel the current behaviour will fall back
to user-space copy. Document this on the function so that it is made
clear.

Improve benchmarking determinism and error messages.

The benchmark numbers that Daniel obtained on his laptop,
running Linux 5.13 on an i5-8350U with /tmp being tmpfs,
are as follows.
The "old" results are BenchmarkExtractV1UsingReader,
and the "new" are BenchmarkExtractV1File.

	name         old time/op    new time/op    delta
	ExtractV1-8    1.33ms ± 1%    1.11ms ± 2%  -16.48%  (p=0.000 n=8+8)

	name         old speed      new speed      delta
	ExtractV1-8  7.88GB/s ± 1%  9.43GB/s ± 2%  +19.74%  (p=0.000 n=8+8)

	name         old alloc/op   new alloc/op   delta
	ExtractV1-8    34.0kB ± 0%     1.0kB ± 0%  -97.09%  (p=0.000 n=8+8)

	name         old allocs/op  new allocs/op  delta
	ExtractV1-8      26.0 ± 0%      23.0 ± 0%  -11.54%  (p=0.000 n=8+8)

So, at least in the case where the filesystem is very fast,
we can see that the benefit is around 10-20%,
as well as fewer allocs thanks to not needing a user-space buffer.
The performance benefit will likely be smaller on slower disks.

For the Linux syscall logic, see:
- https://cs.opensource.google/go/go/+/refs/tags/go1.16.7:src/internal/poll/copy_file_range_linux.go;drc=refs%2Ftags%2Fgo1.16.7;l=54


This commit was moved from ipld/go-car@c514a30
  • Loading branch information
masih committed Aug 11, 2021
1 parent 240282a commit 7fd2bd2
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 10 deletions.
10 changes: 5 additions & 5 deletions ipld/car/v2/bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import (
carv2 "github.com/ipld/go-car/v2"
)

var rng = rand.New(rand.NewSource(1413))

// BenchmarkReadBlocks instantiates a BlockReader, and iterates over all blocks.
// It essentially looks at the contents of any CARv1 or CARv2 file.
// Note that this also uses internal carv1.ReadHeader underneath.
Expand Down Expand Up @@ -59,7 +57,7 @@ func BenchmarkReadBlocks(b *testing.B) {
// BenchmarkExtractV1File extracts inner CARv1 payload from a sample CARv2 file using ExtractV1File.
func BenchmarkExtractV1File(b *testing.B) {
path := filepath.Join(b.TempDir(), "bench-large-v2.car")
generateRandomCarV2File(b, path, 10*1024*1024) // 10 MiB
generateRandomCarV2File(b, path, 10<<20) // 10 MiB
defer os.Remove(path)

info, err := os.Stat(path)
Expand Down Expand Up @@ -87,7 +85,7 @@ func BenchmarkExtractV1File(b *testing.B) {
// BenchmarkExtractV1File.
func BenchmarkExtractV1UsingReader(b *testing.B) {
path := filepath.Join(b.TempDir(), "bench-large-v2.car")
generateRandomCarV2File(b, path, 10*1024*1024) // 10 MiB
generateRandomCarV2File(b, path, 10<<20) // 10 MiB
defer os.Remove(path)

info, err := os.Stat(path)
Expand Down Expand Up @@ -121,6 +119,8 @@ func BenchmarkExtractV1UsingReader(b *testing.B) {
}

func generateRandomCarV2File(b *testing.B, path string, minTotalBlockSize int) {
// Use fixed RNG for determinism across benchmarks.
rng := rand.New(rand.NewSource(1413))
bs, err := blockstore.OpenReadWrite(path, []cid.Cid{})
defer func() {
if err := bs.Finalize(); err != nil {
Expand All @@ -130,7 +130,7 @@ func generateRandomCarV2File(b *testing.B, path string, minTotalBlockSize int) {
if err != nil {
b.Fatal(err)
}
buf := make([]byte, 1024)
buf := make([]byte, 32<<10) // 32 KiB
var totalBlockSize int
for totalBlockSize < minTotalBlockSize {
size, err := rng.Read(buf)
Expand Down
14 changes: 10 additions & 4 deletions ipld/car/v2/writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ func WrapV1(src io.ReadSeeker, dst io.Writer) error {
// Note that the destination path might still be created even if an error
// occurred.
// If srcPath and dstPath are the same, then the dstPath is converted, in-place, to CARv1.
//
// This function aims to extract the CARv1 payload as efficiently as possible.
// The method is best-effort and depends on your operating system;
// for example, it should use copy_file_range on recent Linux versions.
// This API should be preferred over copying directly via Reader.DataReader,
// as it should allow for better performance while always being at least as efficient.
func ExtractV1File(srcPath, dstPath string) (err error) {
src, err := os.Open(srcPath)
if err != nil {
Expand All @@ -110,7 +116,7 @@ func ExtractV1File(srcPath, dstPath string) (err error) {
return ErrAlreadyV1
}
if version != 2 {
return fmt.Errorf("invalid source version: %v", version)
return fmt.Errorf("source version must be 2; got: %d", version)
}

// Read CARv2 header to locate data payload.
Expand All @@ -123,11 +129,11 @@ func ExtractV1File(srcPath, dstPath string) (err error) {
// Validate header
dataOffset := int64(v2h.DataOffset)
if dataOffset < PragmaSize+HeaderSize {
return fmt.Errorf("invalid data payload offset: %v", dataOffset)
return fmt.Errorf("invalid data payload offset: %d", dataOffset)
}
dataSize := int64(v2h.DataSize)
if dataSize <= 0 {
return fmt.Errorf("invalid data payload size: %v", dataSize)
return fmt.Errorf("invalid data payload size: %d", dataSize)
}

// Seek to the point where the data payload starts
Expand Down Expand Up @@ -163,7 +169,7 @@ func ExtractV1File(srcPath, dstPath string) (err error) {
return err
}
if written != dataSize {
return fmt.Errorf("expected to write exactly %v but wrote %v", dataSize, written)
return fmt.Errorf("expected to write exactly %d but wrote %d", dataSize, written)
}

// Check that the size destination file matches expected size.
Expand Down
2 changes: 1 addition & 1 deletion ipld/car/v2/writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func TestExtractV1(t *testing.T) {
func TestExtractV1WithUnknownVersionIsError(t *testing.T) {
dstPath := filepath.Join(t.TempDir(), "extract-dst-file-test-v42.car")
err := ExtractV1File("testdata/sample-rootless-v42.car", dstPath)
require.EqualError(t, err, "invalid source version: 42")
require.EqualError(t, err, "source version must be 2; got: 42")
}

func TestExtractV1FromACarV1IsError(t *testing.T) {
Expand Down

0 comments on commit 7fd2bd2

Please sign in to comment.