From 57195a5ae472878c19dae4b6a65bc37a76e992e4 Mon Sep 17 00:00:00 2001 From: feistel <6742251-feistel@users.noreply.gitlab.com> Date: Sat, 25 Jun 2022 22:09:09 +0200 Subject: Open files lazily when serving content Changelog: performance --- internal/serving/disk/lazy.go | 80 +++++++++++++++++++++++++++++++ internal/serving/disk/reader.go | 26 +++++----- internal/serving/disk/zip/serving_test.go | 33 ++++++++++++- 3 files changed, 125 insertions(+), 14 deletions(-) create mode 100644 internal/serving/disk/lazy.go (limited to 'internal/serving') diff --git a/internal/serving/disk/lazy.go b/internal/serving/disk/lazy.go new file mode 100644 index 00000000..9b58457c --- /dev/null +++ b/internal/serving/disk/lazy.go @@ -0,0 +1,80 @@ +package disk + +import ( + "context" + "errors" + "fmt" + "io" + + "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" +) + +var ( + // Make sure lazyFile is a vfs.File to support vfs.ServeCompressedFile + // if the file is compressed. + // This should always be satisfied because root.Open always returns + // a vfs.File. + _ vfs.File = &lazyFile{} + + // Make sure lazyFile is a ReadSeeker to support http.ServeContent + // if the file is not compressed. + // Note: lazyFile.Seek only works if the underlying root.Open returns + // a vfs.SeekableFile which is the case if the file is not compressed. + _ io.ReadSeeker = &lazyFile{} + + // ErrInvalidSeeker is returned if lazyFile.Seek is called and the + // underlying file is not seekable + ErrInvalidSeeker = errors.New("file is not seekable") +) + +type lazyFile struct { + f vfs.File + err error + load func() (vfs.File, error) +} + +func lazyOpen(ctx context.Context, root vfs.Root, fullPath string) lazyFile { + lf := lazyFile{ + load: func() (vfs.File, error) { + return root.Open(ctx, fullPath) + }, + } + + return lf +} + +func (lf lazyFile) Read(p []byte) (int, error) { + if lf.f == nil && lf.err == nil { + lf.f, lf.err = lf.load() + } + + if lf.err != nil { + return 0, lf.err + } + + return lf.f.Read(p) +} + +func (lf lazyFile) Close() error { + if lf.f != nil { + return lf.f.Close() + } + + return nil +} + +func (lf lazyFile) Seek(offset int64, whence int) (int64, error) { + if lf.f == nil && lf.err == nil { + lf.f, lf.err = lf.load() + } + + if lf.err != nil { + return 0, lf.err + } + + if sf, ok := lf.f.(io.ReadSeeker); ok { + return sf.Seek(offset, whence) + } + + return 0, fmt.Errorf("unable to seek from %T: %w", lf.f, ErrInvalidSeeker) +} diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index f250a0d6..f804c92c 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -199,14 +199,6 @@ func (reader *Reader) resolvePath(ctx context.Context, root vfs.Root, subPath .. func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath, sha string, accessControl bool) bool { fullPath := reader.handleContentEncoding(ctx, w, r, root, origPath) - file, err := root.Open(ctx, fullPath) - if err != nil { - httperrors.Serve500WithRequest(w, r, "root.Open", err) - return true - } - - defer file.Close() - fi, err := root.Lstat(ctx, fullPath) if err != nil { httperrors.Serve500WithRequest(w, r, "root.Lstat", err) @@ -232,12 +224,20 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h reader.fileSizeMetric.WithLabelValues(reader.vfs.Name()).Observe(float64(fi.Size())) - // Support vfs.SeekableFile if available (uncompressed files) - if rs, ok := file.(vfs.SeekableFile); ok { - http.ServeContent(w, r, origPath, fi.ModTime(), rs) - } else { + compressed, err := root.IsCompressed(fullPath) + if err != nil { + httperrors.Serve500WithRequest(w, r, "root.IsCompressed", err) + return true + } + + lf := lazyOpen(ctx, root, fullPath) + defer lf.Close() + + if compressed { w.Header().Set("Content-Length", strconv.FormatInt(fi.Size(), 10)) - vfsServing.ServeCompressedFile(w, r, fi.ModTime(), file) + vfsServing.ServeCompressedFile(w, r, fi.ModTime(), lf) + } else { + http.ServeContent(w, r, origPath, fi.ModTime(), lf) } return true diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go index c3b18f2b..88db51aa 100644 --- a/internal/serving/disk/zip/serving_test.go +++ b/internal/serving/disk/zip/serving_test.go @@ -11,11 +11,13 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/config" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) func TestZip_ServeFileHTTP(t *testing.T) { @@ -33,25 +35,32 @@ func TestZip_ServeFileHTTP(t *testing.T) { path string expectedStatus int expectedBody string - extraHeaders http.Header + // Used to assert there are no zip requests if we don't + // need to open or read from the file, making sure the + // lazyFile is working as intended. + expectedOpen bool + extraHeaders http.Header }{ "accessing /index.html": { vfsPath: httpURL, path: "/index.html", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", + expectedOpen: true, }, "accessing /index.html from disk": { vfsPath: fileURL, path: "/index.html", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", + expectedOpen: true, }, "accessing /": { vfsPath: httpURL, path: "/", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", + expectedOpen: true, }, "accessing / If-Modified-Since": { vfsPath: httpURL, @@ -66,6 +75,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { path: "/", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", + expectedOpen: true, extraHeaders: http.Header{ "If-Modified-Since": {time.Now().AddDate(-10, 0, 0).Format(http.TimeFormat)}, }, @@ -82,6 +92,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { vfsPath: httpURL, path: "/", expectedStatus: http.StatusOK, + expectedOpen: true, expectedBody: "zip.gitlab.io/project/index.html\n", extraHeaders: http.Header{ "If-Unmodified-Since": {time.Now().Format(http.TimeFormat)}, @@ -91,6 +102,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { vfsPath: fileURL, path: "/", expectedStatus: http.StatusOK, + expectedOpen: true, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing without /": { @@ -133,6 +145,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { vfsPath: httpURL, path: "/", expectedStatus: http.StatusOK, + expectedOpen: true, expectedBody: "zip.gitlab.io/project/index.html\n", extraHeaders: http.Header{ "If-None-Match": {fmt.Sprintf("%q", "badetag")}, @@ -142,6 +155,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { vfsPath: httpURL, path: "/", expectedStatus: http.StatusOK, + expectedOpen: true, expectedBody: "zip.gitlab.io/project/index.html\n", extraHeaders: http.Header{ "If-Match": {fmt.Sprintf("%q", sha(httpURL))}, @@ -189,6 +203,8 @@ func TestZip_ServeFileHTTP(t *testing.T) { r.Header = test.extraHeaders } + zipRequestsCountBefore := zipCacheRequests() + handler := serving.Handler{ Writer: w, Request: r, @@ -223,10 +239,25 @@ func TestZip_ServeFileHTTP(t *testing.T) { if test.expectedStatus != http.StatusInternalServerError { require.Equal(t, test.expectedBody, string(body)) } + + zipRequestsCountAfter := zipCacheRequests() + + if !test.expectedOpen { + require.Equal(t, zipRequestsCountBefore, zipRequestsCountAfter) + } else { + require.NotEqual(t, zipRequestsCountBefore, zipRequestsCountAfter) + } }) } } +func zipCacheRequests() float64 { + zipMetricHit := metrics.ZipCacheRequests.WithLabelValues("data-offset", "hit") + zipMetricMiss := metrics.ZipCacheRequests.WithLabelValues("data-offset", "miss") + + return testutil.ToFloat64(zipMetricHit) + testutil.ToFloat64(zipMetricMiss) +} + func sha(path string) string { sha := sha256.Sum256([]byte(path)) s := hex.EncodeToString(sha[:]) -- cgit v1.2.3