From b1c1f021328326013d3ec178ae548541f5a3e093 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 31 Aug 2023 17:58:48 -0700 Subject: [PATCH] use buffers for pathJoin, to re-use buffers. (#17960) ``` benchmark old ns/op new ns/op delta BenchmarkPathJoin/PathJoin-8 79.6 55.3 -30.53% benchmark old allocs new allocs delta BenchmarkPathJoin/PathJoin-8 2 1 -50.00% benchmark old bytes new bytes delta BenchmarkPathJoin/PathJoin-8 48 24 -50.00% ``` --- cmd/bucket-handlers.go | 5 ++++- cmd/metacache-walk.go | 25 +++++++++++++++---------- cmd/object-api-utils.go | 21 +++++++++++---------- cmd/object-api-utils_test.go | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 21 deletions(-) diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index 4c455bc65..9e891ce3b 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -1290,7 +1290,10 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h // instead of "copying" from source, we need the stream to be seekable // to ensure that we can make fan-out calls concurrently. buf := bytebufferpool.Get() - defer bytebufferpool.Put(buf) + defer func() { + buf.Reset() + bytebufferpool.Put(buf) + }() md5w := md5.New() diff --git a/cmd/metacache-walk.go b/cmd/metacache-walk.go index bbef359a1..795ede414 100644 --- a/cmd/metacache-walk.go +++ b/cmd/metacache-walk.go @@ -18,7 +18,6 @@ package cmd import ( - "bytes" "context" "fmt" "io" @@ -32,6 +31,7 @@ import ( xhttp "github.com/minio/minio/internal/http" xioutil "github.com/minio/minio/internal/ioutil" "github.com/minio/minio/internal/logger" + "github.com/valyala/bytebufferpool" ) // WalkDirOptions provides options for WalkDir operations. @@ -144,7 +144,12 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ scanDir = func(current string) error { // Skip forward, if requested... - var sb bytes.Buffer + sb := bytebufferpool.Get() + defer func() { + sb.Reset() + bytebufferpool.Put(sb) + }() + forward := "" if len(opts.ForwardTo) > 0 && strings.HasPrefix(opts.ForwardTo, current) { forward = strings.TrimPrefix(opts.ForwardTo, current) @@ -228,7 +233,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ if s.walkReadMu != nil { s.walkReadMu.Lock() } - meta.metadata, err = s.readMetadata(ctx, pathJoinBuf(&sb, volumeDir, current, entry)) + meta.metadata, err = s.readMetadata(ctx, pathJoinBuf(sb, volumeDir, current, entry)) if s.walkReadMu != nil { s.walkReadMu.Unlock() } @@ -244,7 +249,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ } meta.name = strings.TrimSuffix(entry, xlStorageFormatFile) meta.name = strings.TrimSuffix(meta.name, SlashSeparator) - meta.name = pathJoinBuf(&sb, current, meta.name) + meta.name = pathJoinBuf(sb, current, meta.name) meta.name = decodeDirObject(meta.name) if err := send(meta); err != nil { return err @@ -254,7 +259,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ // Check legacy. if HasSuffix(entry, xlStorageFormatFileV1) && legacy { var meta metaCacheEntry - meta.metadata, err = xioutil.ReadFile(pathJoinBuf(&sb, volumeDir, current, entry)) + meta.metadata, err = xioutil.ReadFile(pathJoinBuf(sb, volumeDir, current, entry)) diskHealthCheckOK(ctx, err) if err != nil { if !IsErrIgnored(err, io.EOF, io.ErrUnexpectedEOF) { @@ -264,7 +269,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ } meta.name = strings.TrimSuffix(entry, xlStorageFormatFileV1) meta.name = strings.TrimSuffix(meta.name, SlashSeparator) - meta.name = pathJoinBuf(&sb, current, meta.name) + meta.name = pathJoinBuf(sb, current, meta.name) if err := send(meta); err != nil { return err } @@ -297,7 +302,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ if contextCanceled(ctx) { return ctx.Err() } - meta := metaCacheEntry{name: pathJoinBuf(&sb, current, entry)} + meta := metaCacheEntry{name: pathJoinBuf(sb, current, entry)} // If directory entry on stack before this, pop it now. for len(dirStack) > 0 && dirStack[len(dirStack)-1] < meta.name { @@ -323,7 +328,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ if s.walkReadMu != nil { s.walkReadMu.Lock() } - meta.metadata, err = s.readMetadata(ctx, pathJoinBuf(&sb, volumeDir, meta.name, xlStorageFormatFile)) + meta.metadata, err = s.readMetadata(ctx, pathJoinBuf(sb, volumeDir, meta.name, xlStorageFormatFile)) if s.walkReadMu != nil { s.walkReadMu.Unlock() } @@ -339,7 +344,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ } case osIsNotExist(err), isSysErrIsDir(err): if legacy { - meta.metadata, err = xioutil.ReadFile(pathJoinBuf(&sb, volumeDir, meta.name, xlStorageFormatFileV1)) + meta.metadata, err = xioutil.ReadFile(pathJoinBuf(sb, volumeDir, meta.name, xlStorageFormatFileV1)) diskHealthCheckOK(ctx, err) if err == nil { // It was an object @@ -353,7 +358,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ // NOT an object, append to stack (with slash) // If dirObject, but no metadata (which is unexpected) we skip it. if !isDirObj { - if !isDirEmpty(pathJoinBuf(&sb, volumeDir, meta.name)) { + if !isDirEmpty(pathJoinBuf(sb, volumeDir, meta.name)) { dirStack = append(dirStack, meta.name+slashSeparator) } } diff --git a/cmd/object-api-utils.go b/cmd/object-api-utils.go index e88a647c5..ab6292e78 100644 --- a/cmd/object-api-utils.go +++ b/cmd/object-api-utils.go @@ -49,6 +49,7 @@ import ( "github.com/minio/minio/internal/logger" "github.com/minio/pkg/trie" "github.com/minio/pkg/wildcard" + "github.com/valyala/bytebufferpool" "golang.org/x/exp/slices" ) @@ -233,25 +234,25 @@ func pathsJoinPrefix(prefix string, elem ...string) (paths []string) { // pathJoin - like path.Join() but retains trailing SlashSeparator of the last element func pathJoin(elem ...string) string { - trailingSlash := "" - if len(elem) > 0 { - if hasSuffixByte(elem[len(elem)-1], SlashSeparatorChar) { - trailingSlash = SlashSeparator - } - } - return path.Join(elem...) + trailingSlash + sb := bytebufferpool.Get() + defer func() { + sb.Reset() + bytebufferpool.Put(sb) + }() + + return pathJoinBuf(sb, elem...) } // pathJoinBuf - like path.Join() but retains trailing SlashSeparator of the last element. // Provide a string builder to reduce allocation. -func pathJoinBuf(dst *bytes.Buffer, elem ...string) string { +func pathJoinBuf(dst *bytebufferpool.ByteBuffer, elem ...string) string { trailingSlash := len(elem) > 0 && hasSuffixByte(elem[len(elem)-1], SlashSeparatorChar) dst.Reset() added := 0 for _, e := range elem { if added > 0 || e != "" { if added > 0 { - dst.WriteRune(SlashSeparatorChar) + dst.WriteByte(SlashSeparatorChar) } dst.WriteString(e) added += len(e) @@ -266,7 +267,7 @@ func pathJoinBuf(dst *bytes.Buffer, elem ...string) string { return s } if trailingSlash { - dst.WriteRune(SlashSeparatorChar) + dst.WriteByte(SlashSeparatorChar) } return dst.String() } diff --git a/cmd/object-api-utils_test.go b/cmd/object-api-utils_test.go index c2a0f4610..6f6530a1e 100644 --- a/cmd/object-api-utils_test.go +++ b/cmd/object-api-utils_test.go @@ -24,6 +24,7 @@ import ( "io" "net/http" "net/http/httptest" + "path" "reflect" "runtime" "strconv" @@ -36,6 +37,38 @@ import ( "github.com/minio/pkg/trie" ) +func pathJoinOld(elem ...string) string { + trailingSlash := "" + if len(elem) > 0 { + if hasSuffixByte(elem[len(elem)-1], SlashSeparatorChar) { + trailingSlash = SlashSeparator + } + } + return path.Join(elem...) + trailingSlash +} + +func BenchmarkPathJoinOld(b *testing.B) { + b.Run("PathJoin", func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + pathJoinOld("volume", "path/path/path") + } + }) +} + +func BenchmarkPathJoin(b *testing.B) { + b.Run("PathJoin", func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + pathJoin("volume", "path/path/path") + } + }) +} + // Wrapper func TestPathTraversalExploit(t *testing.T) { if runtime.GOOS != globalWindowsOSName {