From b2257682e4efd6c46fb4f0360b73c81069b3cd82 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Sun, 13 Mar 2016 11:11:40 -0700 Subject: [PATCH 1/2] pkg/fs: add benchmark for GetObject This commit adds a benchmark for GetObject. It doesn't leverage the I/O as much because it uses short text for data, just 58 chars. --- pkg/fs/fs-object_test.go | 73 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 pkg/fs/fs-object_test.go diff --git a/pkg/fs/fs-object_test.go b/pkg/fs/fs-object_test.go new file mode 100644 index 000000000..4c1de5f44 --- /dev/null +++ b/pkg/fs/fs-object_test.go @@ -0,0 +1,73 @@ +/* + * Minio Cloud Storage, (C) 2016 Minio, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package fs + +import ( + "bytes" + "crypto/md5" + "encoding/base64" + "io/ioutil" + "os" + "strconv" + "testing" +) + +func BenchmarkGetObject(b *testing.B) { + // Make a temporary directory to use as the filesystem. + directory, e := ioutil.TempDir("", "minio-benchmark-getobject") + if e != nil { + b.Fatal(e) + } + defer os.RemoveAll(directory) + + // Create the filesystem. + filesystem, err := New(directory, 0) + if err != nil { + b.Fatal(err) + } + + // Make a bucket and put in a few objects. + err = filesystem.MakeBucket("bucket") + if err != nil { + b.Fatal(err) + } + + text := "Jack and Jill went up the hill / To fetch a pail of water." + hasher := md5.New() + hasher.Write([]byte(text)) + sum := base64.StdEncoding.EncodeToString(hasher.Sum(nil)) + for i := 0; i < 10; i++ { + _, err = filesystem.CreateObject("bucket", "object"+strconv.Itoa(i), sum, int64(len(text)), bytes.NewBufferString(text), nil) + if err != nil { + b.Fatal(err) + } + } + + var w bytes.Buffer + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + n, err := filesystem.GetObject(&w, "bucket", "object"+strconv.Itoa(i%10), 0, 0) + if err != nil { + b.Error(err) + } + if n != int64(len(text)) { + b.Errorf("GetObject returned incorrect length %d (should be %d)\n", n, int64(len(text))) + } + } +} From 583e4ecff61a05ebccba7c2c3784459bd84ffc87 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Sun, 13 Mar 2016 00:58:07 -0800 Subject: [PATCH 2/2] pkg/fs: optimize GetObject syscalls for common case In the common case, GetObject is called on a bucket that exists and an object that exists and is not a directory. It should be optimized for this case, thus error-related syscalls are pushed back until they are necessary. This should not impact performance negatively in the uncommon case, and instead drops two otherwise unnecessary os.Stat's in the common case. The race conditions around a proper error being returned were present beforehand. It also renames 'err' to 'e'. --- pkg/fs/fs-object.go | 67 +++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/pkg/fs/fs-object.go b/pkg/fs/fs-object.go index dd45a90c3..748bcd621 100644 --- a/pkg/fs/fs-object.go +++ b/pkg/fs/fs-object.go @@ -54,50 +54,53 @@ func (fs Filesystem) GetObject(w io.Writer, bucket, object string, start, length // normalize buckets. bucket = fs.denormalizeBucket(bucket) - bucketPath := filepath.Join(fs.path, bucket) - if _, e := os.Stat(bucketPath); e != nil { + objectPath := filepath.Join(fs.path, bucket, object) + + file, e := os.Open(objectPath) + if e != nil { + // If the object doesn't exist, the bucket might not exist either. Stat for + // the bucket and give a better error message if that is true. if os.IsNotExist(e) { - return 0, probe.NewError(BucketNotFound{Bucket: bucket}) + _, e = os.Stat(filepath.Join(fs.path, bucket)) + if os.IsNotExist(e) { + return 0, probe.NewError(BucketNotFound{Bucket: bucket}) + } + + return 0, probe.NewError(ObjectNotFound{Bucket: bucket, Object: object}) } return 0, probe.NewError(e) } - - objectPath := filepath.Join(bucketPath, object) - filestat, err := os.Stat(objectPath) - switch err := err.(type) { - case nil: - if filestat.IsDir() { - return 0, probe.NewError(ObjectNotFound{Bucket: bucket, Object: object}) - } - default: - if os.IsNotExist(err) { - return 0, probe.NewError(ObjectNotFound{Bucket: bucket, Object: object}) - } - return 0, probe.NewError(err) - } - file, err := os.Open(objectPath) - if err != nil { - return 0, probe.NewError(err) - } defer file.Close() - _, err = file.Seek(start, os.SEEK_SET) - if err != nil { - return 0, probe.NewError(err) + _, e = file.Seek(start, os.SEEK_SET) + if e != nil { + // When the "handle is invalid", the file might be a directory on Windows. + if runtime.GOOS == "windows" && strings.Contains(e.Error(), "handle is invalid") { + return 0, probe.NewError(ObjectNotFound{Bucket: bucket, Object: object}) + } + + return 0, probe.NewError(e) } var count int64 + // Copy over the whole file if the length is non-positive. if length > 0 { - count, err = io.CopyN(w, file, length) - if err != nil { - return count, probe.NewError(err) - } + count, e = io.CopyN(w, file, length) } else { - count, err = io.Copy(w, file) - if err != nil { - return count, probe.NewError(err) - } + count, e = io.Copy(w, file) } + + if e != nil { + // This call will fail if the object is a directory. Stat the file to see if + // this is true, if so, return an ObjectNotFound error. + stat, e := os.Stat(objectPath) + if e == nil && stat.IsDir() { + return count, probe.NewError(ObjectNotFound{Bucket: bucket, Object: object}) + } + + return count, probe.NewError(e) + } + return count, nil }