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%
```
This commit is contained in:
Harshavardhana 2023-08-31 17:58:48 -07:00 committed by GitHub
parent ea93643e6a
commit b1c1f02132
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 63 additions and 21 deletions

View File

@ -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 // instead of "copying" from source, we need the stream to be seekable
// to ensure that we can make fan-out calls concurrently. // to ensure that we can make fan-out calls concurrently.
buf := bytebufferpool.Get() buf := bytebufferpool.Get()
defer bytebufferpool.Put(buf) defer func() {
buf.Reset()
bytebufferpool.Put(buf)
}()
md5w := md5.New() md5w := md5.New()

View File

@ -18,7 +18,6 @@
package cmd package cmd
import ( import (
"bytes"
"context" "context"
"fmt" "fmt"
"io" "io"
@ -32,6 +31,7 @@ import (
xhttp "github.com/minio/minio/internal/http" xhttp "github.com/minio/minio/internal/http"
xioutil "github.com/minio/minio/internal/ioutil" xioutil "github.com/minio/minio/internal/ioutil"
"github.com/minio/minio/internal/logger" "github.com/minio/minio/internal/logger"
"github.com/valyala/bytebufferpool"
) )
// WalkDirOptions provides options for WalkDir operations. // 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 { scanDir = func(current string) error {
// Skip forward, if requested... // Skip forward, if requested...
var sb bytes.Buffer sb := bytebufferpool.Get()
defer func() {
sb.Reset()
bytebufferpool.Put(sb)
}()
forward := "" forward := ""
if len(opts.ForwardTo) > 0 && strings.HasPrefix(opts.ForwardTo, current) { if len(opts.ForwardTo) > 0 && strings.HasPrefix(opts.ForwardTo, current) {
forward = strings.TrimPrefix(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 { if s.walkReadMu != nil {
s.walkReadMu.Lock() 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 { if s.walkReadMu != nil {
s.walkReadMu.Unlock() 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(entry, xlStorageFormatFile)
meta.name = strings.TrimSuffix(meta.name, SlashSeparator) 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) meta.name = decodeDirObject(meta.name)
if err := send(meta); err != nil { if err := send(meta); err != nil {
return err return err
@ -254,7 +259,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ
// Check legacy. // Check legacy.
if HasSuffix(entry, xlStorageFormatFileV1) && legacy { if HasSuffix(entry, xlStorageFormatFileV1) && legacy {
var meta metaCacheEntry 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) diskHealthCheckOK(ctx, err)
if err != nil { if err != nil {
if !IsErrIgnored(err, io.EOF, io.ErrUnexpectedEOF) { 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(entry, xlStorageFormatFileV1)
meta.name = strings.TrimSuffix(meta.name, SlashSeparator) 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 { if err := send(meta); err != nil {
return err return err
} }
@ -297,7 +302,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ
if contextCanceled(ctx) { if contextCanceled(ctx) {
return ctx.Err() 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. // If directory entry on stack before this, pop it now.
for len(dirStack) > 0 && dirStack[len(dirStack)-1] < meta.name { 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 { if s.walkReadMu != nil {
s.walkReadMu.Lock() 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 { if s.walkReadMu != nil {
s.walkReadMu.Unlock() s.walkReadMu.Unlock()
} }
@ -339,7 +344,7 @@ func (s *xlStorage) WalkDir(ctx context.Context, opts WalkDirOptions, wr io.Writ
} }
case osIsNotExist(err), isSysErrIsDir(err): case osIsNotExist(err), isSysErrIsDir(err):
if legacy { 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) diskHealthCheckOK(ctx, err)
if err == nil { if err == nil {
// It was an object // 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) // NOT an object, append to stack (with slash)
// If dirObject, but no metadata (which is unexpected) we skip it. // If dirObject, but no metadata (which is unexpected) we skip it.
if !isDirObj { if !isDirObj {
if !isDirEmpty(pathJoinBuf(&sb, volumeDir, meta.name)) { if !isDirEmpty(pathJoinBuf(sb, volumeDir, meta.name)) {
dirStack = append(dirStack, meta.name+slashSeparator) dirStack = append(dirStack, meta.name+slashSeparator)
} }
} }

View File

@ -49,6 +49,7 @@ import (
"github.com/minio/minio/internal/logger" "github.com/minio/minio/internal/logger"
"github.com/minio/pkg/trie" "github.com/minio/pkg/trie"
"github.com/minio/pkg/wildcard" "github.com/minio/pkg/wildcard"
"github.com/valyala/bytebufferpool"
"golang.org/x/exp/slices" "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 // pathJoin - like path.Join() but retains trailing SlashSeparator of the last element
func pathJoin(elem ...string) string { func pathJoin(elem ...string) string {
trailingSlash := "" sb := bytebufferpool.Get()
if len(elem) > 0 { defer func() {
if hasSuffixByte(elem[len(elem)-1], SlashSeparatorChar) { sb.Reset()
trailingSlash = SlashSeparator bytebufferpool.Put(sb)
} }()
}
return path.Join(elem...) + trailingSlash return pathJoinBuf(sb, elem...)
} }
// pathJoinBuf - like path.Join() but retains trailing SlashSeparator of the last element. // pathJoinBuf - like path.Join() but retains trailing SlashSeparator of the last element.
// Provide a string builder to reduce allocation. // 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) trailingSlash := len(elem) > 0 && hasSuffixByte(elem[len(elem)-1], SlashSeparatorChar)
dst.Reset() dst.Reset()
added := 0 added := 0
for _, e := range elem { for _, e := range elem {
if added > 0 || e != "" { if added > 0 || e != "" {
if added > 0 { if added > 0 {
dst.WriteRune(SlashSeparatorChar) dst.WriteByte(SlashSeparatorChar)
} }
dst.WriteString(e) dst.WriteString(e)
added += len(e) added += len(e)
@ -266,7 +267,7 @@ func pathJoinBuf(dst *bytes.Buffer, elem ...string) string {
return s return s
} }
if trailingSlash { if trailingSlash {
dst.WriteRune(SlashSeparatorChar) dst.WriteByte(SlashSeparatorChar)
} }
return dst.String() return dst.String()
} }

View File

@ -24,6 +24,7 @@ import (
"io" "io"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"path"
"reflect" "reflect"
"runtime" "runtime"
"strconv" "strconv"
@ -36,6 +37,38 @@ import (
"github.com/minio/pkg/trie" "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 // Wrapper
func TestPathTraversalExploit(t *testing.T) { func TestPathTraversalExploit(t *testing.T) {
if runtime.GOOS != globalWindowsOSName { if runtime.GOOS != globalWindowsOSName {