From 35907790a5baedfc5d0f164ec8637571ff026aaf Mon Sep 17 00:00:00 2001 From: Ola Rozenfeld Date: Thu, 1 Feb 2024 16:45:33 -0500 Subject: [PATCH] Addressing comments --- go/pkg/diskcache/BUILD.bazel | 6 +- go/pkg/diskcache/diskcache.go | 79 ++++++++++--------- go/pkg/diskcache/diskcache_test.go | 9 +-- .../{atim_darwin.go => sys_darwin.go} | 1 + .../diskcache/{atim_linux.go => sys_linux.go} | 2 +- .../{atim_windows.go => sys_windows.go} | 2 +- 6 files changed, 52 insertions(+), 47 deletions(-) rename go/pkg/diskcache/{atim_darwin.go => sys_darwin.go} (83%) rename go/pkg/diskcache/{atim_linux.go => sys_linux.go} (80%) rename go/pkg/diskcache/{atim_windows.go => sys_windows.go} (88%) diff --git a/go/pkg/diskcache/BUILD.bazel b/go/pkg/diskcache/BUILD.bazel index 4dca4268d..718b923d6 100644 --- a/go/pkg/diskcache/BUILD.bazel +++ b/go/pkg/diskcache/BUILD.bazel @@ -3,10 +3,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "diskcache", srcs = [ - "atim_darwin.go", - "atim_linux.go", - "atim_windows.go", "diskcache.go", + "sys_darwin.go", + "sys_linux.go", + "sys_windows.go", ], importpath = "github.com/bazelbuild/remote-apis-sdks/go/pkg/diskcache", visibility = ["//visibility:public"], diff --git a/go/pkg/diskcache/diskcache.go b/go/pkg/diskcache/diskcache.go index 5e7924796..875372d20 100644 --- a/go/pkg/diskcache/diskcache.go +++ b/go/pkg/diskcache/diskcache.go @@ -118,43 +118,50 @@ func New(ctx context.Context, root string, maxCapacityBytes uint64) *DiskCache { _ = os.MkdirAll(root, os.ModePerm) // We use Git's directory/file naming structure as inspiration: // https://git-scm.com/book/en/v2/Git-Internals-Git-Objects#:~:text=The%20subdirectory%20is%20named%20with%20the%20first%202%20characters%20of%20the%20SHA%2D1%2C%20and%20the%20filename%20is%20the%20remaining%2038%20characters. + var wg sync.WaitGroup + wg.Add(256) for i := 0; i < 256; i++ { - _ = os.MkdirAll(filepath.Join(root, fmt.Sprintf("%02x", i)), os.ModePerm) - } - _ = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - // We log and continue on all errors, because cache read errors are not critical. - if err != nil { - log.Errorf("Error reading cache directory: %v", err) - return nil - } - if d.IsDir() { - return nil - } - subdir := filepath.Base(filepath.Dir(path)) - k, err := res.getKeyFromFileName(subdir + d.Name()) - if err != nil { - log.Errorf("Error parsing cached file name %s: %v", path, err) - return nil - } - atime, err := GetLastAccessTime(path) - if err != nil { - log.Errorf("Error getting last accessed time of %s: %v", path, err) - return nil - } - it := &qitem{ - key: k, - lat: atime, - } - size, err := res.getItemSize(k) - if err != nil { - log.Errorf("Error getting file size of %s: %v", path, err) - return nil - } - res.store.Store(k, it) - atomic.AddInt64(&res.sizeBytes, size) - heap.Push(res.queue, it) - return nil - }) + prefixDir := filepath.Join(root, fmt.Sprintf("%02x", i)) + go func() { + defer wg.Done() + _ = os.MkdirAll(prefixDir, os.ModePerm) + _ = filepath.WalkDir(prefixDir, func(path string, d fs.DirEntry, err error) error { + // We log and continue on all errors, because cache read errors are not critical. + if err != nil { + log.Errorf("Error reading cache directory: %v", err) + return nil + } + if d.IsDir() { + return nil + } + subdir := filepath.Base(filepath.Dir(path)) + k, err := res.getKeyFromFileName(subdir + d.Name()) + if err != nil { + log.Errorf("Error parsing cached file name %s: %v", path, err) + return nil + } + atime, err := GetLastAccessTime(path) + if err != nil { + log.Errorf("Error getting last accessed time of %s: %v", path, err) + return nil + } + it := &qitem{ + key: k, + lat: atime, + } + size, err := res.getItemSize(k) + if err != nil { + log.Errorf("Error getting file size of %s: %v", path, err) + return nil + } + res.store.Store(k, it) + atomic.AddInt64(&res.sizeBytes, size) + heap.Push(res.queue, it) + return nil + }) + }() + } + wg.Wait() go res.gc() return res } diff --git a/go/pkg/diskcache/diskcache_test.go b/go/pkg/diskcache/diskcache_test.go index c784535a3..6f27ba727 100644 --- a/go/pkg/diskcache/diskcache_test.go +++ b/go/pkg/diskcache/diskcache_test.go @@ -21,12 +21,9 @@ import ( // Test utility only. Assumes all modifications are done, and at least one GC is expected. func waitForGc(d *DiskCache) { - for { - select { - case t := <-d.testGcTicks: - if t == d.gcTick { - return - } + for t := range d.testGcTicks { + if t == d.gcTick { + return } } } diff --git a/go/pkg/diskcache/atim_darwin.go b/go/pkg/diskcache/sys_darwin.go similarity index 83% rename from go/pkg/diskcache/atim_darwin.go rename to go/pkg/diskcache/sys_darwin.go index 82509e751..4fa2a27cd 100644 --- a/go/pkg/diskcache/atim_darwin.go +++ b/go/pkg/diskcache/sys_darwin.go @@ -1,4 +1,5 @@ // Utility to get the last accessed time on Darwin. +// System utilities that differ between OS implementations. package diskcache import ( diff --git a/go/pkg/diskcache/atim_linux.go b/go/pkg/diskcache/sys_linux.go similarity index 80% rename from go/pkg/diskcache/atim_linux.go rename to go/pkg/diskcache/sys_linux.go index c34df5127..1c79836c6 100644 --- a/go/pkg/diskcache/atim_linux.go +++ b/go/pkg/diskcache/sys_linux.go @@ -1,4 +1,4 @@ -// Utility to get the last accessed time on Linux. +// System utilities that differ between OS implementations. package diskcache import ( diff --git a/go/pkg/diskcache/atim_windows.go b/go/pkg/diskcache/sys_windows.go similarity index 88% rename from go/pkg/diskcache/atim_windows.go rename to go/pkg/diskcache/sys_windows.go index c633346ac..92f35d4a6 100644 --- a/go/pkg/diskcache/atim_windows.go +++ b/go/pkg/diskcache/sys_windows.go @@ -1,4 +1,4 @@ -// Utility to get the last accessed time on Windows. +// System utilities that differ between OS implementations. package diskcache import (