diff --git a/ChangeLog b/ChangeLog index 8eb6e718a..81ada4a45 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,7 +5,7 @@ Version 17.34.0 * Subinclude cycle detection (#3615) * Fix for `plz export --notrim` in some cases (#3621) * `plz init plugin go` now writes a filegroup for `go.mod` and sets `modfile` (#3587) - * Return the subrepo config error from every Initialise call (#3637) + * Return the subrepo config error from every Initialise call (#3637) Version 17.33.0 --------------- diff --git a/go.mod b/go.mod index 18303d482..02c79c81e 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,6 @@ require ( github.com/cespare/xxhash/v2 v2.3.0 github.com/coreos/go-semver v0.3.1 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc - github.com/djherbis/atime v1.1.0 github.com/dustin/go-humanize v1.0.1 github.com/fsnotify/fsnotify v1.10.1 github.com/golang/protobuf v1.5.4 diff --git a/go.sum b/go.sum index a9158e6b9..9a50f4982 100644 --- a/go.sum +++ b/go.sum @@ -51,8 +51,6 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/djherbis/atime v1.1.0 h1:rgwVbP/5by8BvvjBNrbh64Qz33idKT3pSnMSJsxhi0g= -github.com/djherbis/atime v1.1.0/go.mod h1:28OF6Y8s3NQWwacXc5eZTsEsiMzp7LF8MbXE+XJPdBE= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= diff --git a/src/cache/BUILD b/src/cache/BUILD index 72d59791b..dd11cd56a 100644 --- a/src/cache/BUILD +++ b/src/cache/BUILD @@ -7,7 +7,6 @@ go_library( pgo_file = "//:pgo", visibility = ["PUBLIC"], deps = [ - "///third_party/go/github.com_djherbis_atime//:atime", "///third_party/go/github.com_dustin_go-humanize//:go-humanize", "///third_party/go/github.com_hashicorp_go-retryablehttp//:go-retryablehttp", "//src/clean", diff --git a/src/cache/dir_cache.go b/src/cache/dir_cache.go index 02a367b8d..533aebd4a 100644 --- a/src/cache/dir_cache.go +++ b/src/cache/dir_cache.go @@ -15,9 +15,9 @@ import ( "slices" "strings" "sync" + "syscall" "time" - "github.com/djherbis/atime" "github.com/dustin/go-humanize" "github.com/thought-machine/please/src/clean" @@ -182,8 +182,7 @@ func (cache *dirCache) storeFile(target *core.BuildTarget, out, cacheDir string) } // TODO(peterebden): This is a little inefficient, it would be better to track the size in // RecursiveCopy rather than walking again. - size, _ := findSize(cachedFile) - return size + return unsharedSize(cachedFile) } func (cache *dirCache) Retrieve(target *core.BuildTarget, key []byte, outs []string) bool { @@ -210,11 +209,15 @@ func (cache *dirCache) retrieve(target *core.BuildTarget, key []byte, suffix str } func (cache *dirCache) retrieveFiles(target *core.BuildTarget, cacheDir string, outs []string) (bool, error) { - if !core.PathExists(cacheDir) { + info, err := os.Stat(cacheDir) + if os.IsNotExist(err) { log.Debug("%s: %s doesn't exist in dir cache", target.Label, cacheDir) return false, nil + } else if err != nil { + return false, err } cache.markDir(cacheDir, 0) + cache.touch(cacheDir, info) if len(outs) == 0 { return true, nil } @@ -379,91 +382,157 @@ func newDirCache(config *core.Configuration) *dirCache { return cache } -// Period of time in seconds between which two artifacts are considered to have the same atime. +// Period of time in seconds between which two artifacts are considered to have been used at the same time. const accessTimeGracePeriod = 600 // Ten minutes -// A cacheEntry represents a single file entry in the cache. +// touchInterval is the minimum time between updates of an entry's modification time when it's retrieved. +// Updating it on every retrieval would cause a lot of unnecessary writes. +const touchInterval = time.Hour + +// A cacheEntry represents a single entry in the cache (i.e. one stored target). type cacheEntry struct { Path string - Size uint64 - Atime int64 + Size uint64 // Total size of the files in this entry that are not linked from outside the cache + Mtime int64 + Files []fileID } -func findSize(path string) (uint64, error) { - var totalSize uint64 +// A fileID identifies a file on disk independently of the path(s) it's linked at. +type fileID struct { + Dev, Ino uint64 +} + +// A cacheFile is a single file in the cache, which may be linked into multiple entries +// and also from outside the cache (typically into plz-out, when it's stored or retrieved). +type cacheFile struct { + Size uint64 + Links uint64 // Total number of hard links to this file + Refs uint64 // Number of those links that are inside the cache + Unshared bool // True if this file is only linked from inside the cache. +} + +// fileInfo returns the identifier and number of hard links for a file. +func fileInfo(info os.FileInfo) (fileID, uint64) { + st := info.Sys().(*syscall.Stat_t) + // The types of these fields vary between platforms, so the conversions are needed on some of them. + return fileID{Dev: uint64(st.Dev), Ino: uint64(st.Ino)}, uint64(st.Nlink) //nolint:unconvert +} + +// unsharedSize returns the total size of the files under the given path that are not hard linked from anywhere else. +func unsharedSize(path string) uint64 { + var size uint64 + filepath.Walk(path, func(path string, info os.FileInfo, err error) error { + if err == nil && !info.IsDir() { + if _, links := fileInfo(info); links <= 1 { + size += uint64(info.Size()) + } + } + return nil + }) + return size +} + +// readEntry reads a single entry in the cache, recording all the files in it. +func readEntry(path string, files map[fileID]*cacheFile) (*cacheEntry, error) { + info, err := os.Lstat(path) + if err != nil { + return nil, err + } + entry := &cacheEntry{Path: path, Mtime: info.ModTime().Unix()} if err := filepath.Walk(path, func(path string, info os.FileInfo, err error) error { if err != nil { return err + } else if info.IsDir() { + return nil + } + id, links := fileInfo(info) + if _, present := files[id]; !present { + files[id] = &cacheFile{Size: uint64(info.Size()), Links: links} } - totalSize += uint64(info.Size()) + files[id].Refs++ + entry.Files = append(entry.Files, id) return nil }); err != nil { - return 0, err + return nil, err } - return totalSize, nil + return entry, nil } // clean runs background cleaning of this cache until the process exits. // Returns the total size of the cache after it's finished. +// Sizes only count files that are not hard linked from outside the cache; those are typically also in someone's +// plz-out, and so removing them would not free any space (and would make it less likely for that file to be shared +// with the next repo that builds it). func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 { - entries := []cacheEntry{} + entries := []*cacheEntry{} + files := map[fileID]*cacheFile{} var totalSize uint64 if err := fs.Walk(cache.Dir, func(path string, isDir bool) error { - name := filepath.Base(path) - if cache.shouldClean(name, isDir) { - if size, marked := cache.isMarked(path); marked { - totalSize += size - if !cache.Compress { - return filepath.SkipDir // Already handled - } - return nil // Need to keep walking if we are dealing with compressed files - } - size, err := findSize(path) - if err != nil { - return err - } - info, err := os.Stat(path) - if err != nil { - return err - } - entries = append(entries, cacheEntry{ - Path: path, - Size: size, - Atime: atime.Get(info).Unix(), - }) + if !cache.shouldClean(filepath.Base(path), isDir) { + return nil // nothing particularly to do for other entries + } + if size, marked := cache.isMarked(path); marked { totalSize += size - if !cache.Compress { - return filepath.SkipDir - } + } else if entry, err := readEntry(path, files); err != nil { + // This can happen if another process is cleaning concurrently; it's not fatal to us. + log.Warning("Failed to read cache entry %s: %s", path, err) + } else { + entries = append(entries, entry) + } + if !cache.Compress { + return filepath.SkipDir // Already handled } - return nil // nothing particularly to do for other entries + return nil // Need to keep walking if we are dealing with compressed files }); err != nil { log.Error("error walking cache directory: %s\n", err) return totalSize } + for _, f := range files { + if f.Unshared = f.Links <= f.Refs; f.Unshared { + totalSize += f.Size + } + } + // Entries with no unshared files would free nothing if removed, so are never candidates for cleaning. + candidates := entries[:0] + for _, entry := range entries { + for _, id := range entry.Files { + if f := files[id]; f.Unshared { + entry.Size += f.Size + } + } + if entry.Size > 0 { + candidates = append(candidates, entry) + } + } log.Info("Total cache size: %s", humanize.Bytes(totalSize)) if totalSize < highWaterMark { return totalSize // Nothing to do, cache is small enough. } // OK, we need to slim it down a bit. We implement a simple LRU algorithm. - slices.SortFunc(entries, func(a, b cacheEntry) int { - diff := a.Atime - b.Atime - if diff > -accessTimeGracePeriod && diff < accessTimeGracePeriod { + slices.SortFunc(candidates, func(a, b *cacheEntry) int { + if diff := a.Mtime - b.Mtime; diff > -accessTimeGracePeriod && diff < accessTimeGracePeriod { return cmp.Compare(b.Size, a.Size) } - return cmp.Compare(a.Atime, b.Atime) + return cmp.Compare(a.Mtime, b.Mtime) }) - for _, entry := range entries { + for _, entry := range candidates { if _, marked := cache.isMarked(entry.Path); marked { continue } - log.Debug("Cleaning %s, accessed %s, saves %s", entry.Path, humanize.Time(time.Unix(entry.Atime, 0)), humanize.Bytes(entry.Size)) + log.Debug("Cleaning %s, used %s, saves up to %s", entry.Path, humanize.Time(time.Unix(entry.Mtime, 0)), humanize.Bytes(entry.Size)) if err := cache.cleanPath(entry.Path); err != nil { log.Warning("Error while cleaning cache: %s", err) continue } - totalSize -= entry.Size + // A file is only freed once the last entry referring to it is removed. + for _, id := range entry.Files { + f := files[id] + f.Refs-- + if f.Refs == 0 && f.Unshared { + totalSize -= f.Size + } + } if totalSize < lowWaterMark { break } @@ -471,6 +540,17 @@ func (cache *dirCache) clean(highWaterMark, lowWaterMark uint64) uint64 { return totalSize } +// touch updates the modification time of a cache entry to mark it as recently used. +func (cache *dirCache) touch(path string, info os.FileInfo) { + if time.Since(info.ModTime()) < touchInterval { + return // Updated recently enough already + } + now := time.Now() + if err := os.Chtimes(path, now, now); err != nil { + log.Debug("Failed to update modification time of %s: %s", path, err) + } +} + func (cache *dirCache) cleanPath(path string) error { lockFile, err := core.AcquireExclusiveFileLock(path + ".lock") if err != nil { diff --git a/src/cache/dir_cache_test.go b/src/cache/dir_cache_test.go index c3c39df98..582844d9e 100644 --- a/src/cache/dir_cache_test.go +++ b/src/cache/dir_cache_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" @@ -60,11 +61,9 @@ func TestStoreAndRetrieve(t *testing.T) { func TestCleanNoop(t *testing.T) { cache := makeCache(".plz-cache-test2", false) target1 := makeTarget2("//test2:target1", 2000) - cache.Store(target1, hash, target1.Outputs(nil)) - assert.True(t, inCache(target1)) + writeFile(cachePath(target1, false), 2000) target2 := makeTarget2("//test2:target2", 2000) - cache.Store(target2, hash, target2.Outputs(nil)) - assert.True(t, inCache(target2)) + writeFile(cachePath(target2, false), 2000) // Doesn't clean anything this time because the high water mark is sufficiently high totalSize := cache.clean(20000, 1000) assert.EqualValues(t, 12000, totalSize) @@ -80,10 +79,10 @@ func TestCleanNoop2(t *testing.T) { target2 := makeTarget2("//test3:target2", 2000) cache.Store(target2, hash, target2.Outputs(nil)) assert.True(t, inCache(target2)) - // Doesn't clean anything this time, the high water mark is lower but both targets have - // just been built. - totalSize := cache.clean(10000, 1000) - assert.EqualValues(t, 12000, totalSize) + // Doesn't clean anything this time; both targets have just been built, and are hard linked + // into plz-out so they don't count towards the size anyway. + totalSize := cache.clean(1, 0) + assert.EqualValues(t, 0, totalSize) assert.True(t, inCache(target1)) assert.True(t, inCache(target2)) } @@ -96,9 +95,9 @@ func TestCleanForReal(t *testing.T) { target2 := makeTarget2("//test4:target2", 2000) writeFile(cachePath(target2, false), 2000) assert.True(t, inCache(target2)) - // This time it should clean target2, because target1 has just been stored - totalSize := cache.clean(10000, 1000) - assert.EqualValues(t, 6000, totalSize) + // This time it should clean target2, because target1 is hard linked into plz-out so removing it wouldn't free anything + totalSize := cache.clean(5000, 1000) + assert.EqualValues(t, 0, totalSize) assert.True(t, inCache(target1)) assert.False(t, inCache(target2)) } @@ -111,13 +110,97 @@ func TestCleanForReal2(t *testing.T) { target2 := makeTarget2("//test5:target2", 2000) cache.Store(target2, hash, target2.Outputs(nil)) assert.True(t, inCache(target2)) - // This time it should clean target1, because target2 has just been stored - totalSize := cache.clean(10000, 1000) - assert.EqualValues(t, 6000, totalSize) + // This time it should clean target1, because target2 is hard linked into plz-out so removing it wouldn't free anything + totalSize := cache.clean(5000, 1000) + assert.EqualValues(t, 0, totalSize) assert.False(t, inCache(target1)) assert.True(t, inCache(target2)) } +func TestCleanSkipsLinkedEntries(t *testing.T) { + makeCache(".plz-cache-test8", false).Store(makeTarget2("//test8:target1", 2000), hash, []string{"test.go"}) + target1 := makeTarget2("//test8:target1", 2000) + cache := makeCache(".plz-cache-test8", false) // A new instance doesn't know what the previous one stored. + // Hardlink the file in again, as a second build would do on retrieving it. + out := filepath.Join("plz-out/gen", target1.Label.PackageName, "test.go") + assert.NoError(t, os.Remove(out)) + assert.NoError(t, os.Link(cachePath(target1, false), out)) + // The entry is still linked into plz-out so cleaning it won't save anything. + assert.EqualValues(t, 0, cache.clean(1, 0)) + assert.True(t, inCache(target1)) + // Once that link is gone (e.g. the repo is deleted), it does count and can be cleaned. + assert.NoError(t, os.Remove(out)) + assert.EqualValues(t, 0, cache.clean(1000, 0)) + assert.False(t, inCache(target1)) +} + +func TestCleanSharedWithinCache(t *testing.T) { + cache := makeCache(".plz-cache-test9", false) + target1 := makeTarget2("//test9:target1", 2000) + writeFile(cachePath(target1, false), 2000) + target2 := makeTarget2("//test9:target2", 2000) + assert.NoError(t, os.MkdirAll(filepath.Dir(cachePath(target2, false)), core.DirPermissions)) + assert.NoError(t, os.Link(cachePath(target1, false), cachePath(target2, false))) + setMtime(t, cachePath(target1, false), 2*time.Hour) + // The file is only linked within the cache, so it counts but only once. + assert.EqualValues(t, 6000, cache.clean(10000, 0)) + // Removing the first entry doesn't free anything, so it has to remove the second too. + assert.EqualValues(t, 0, cache.clean(5000, 1000)) + assert.False(t, inCache(target1)) + assert.False(t, inCache(target2)) +} + +func TestCleanLeastRecentlyUsed(t *testing.T) { + cache := makeCache(".plz-cache-test10", false) + target1 := makeTarget2("//test10:target1", 2000) + writeFile(cachePath(target1, false), 2000) + setMtime(t, cachePath(target1, false), time.Hour) + target2 := makeTarget2("//test10:target2", 2000) + writeFile(cachePath(target2, false), 2000) + setMtime(t, cachePath(target2, false), 3*time.Hour) + target3 := makeTarget2("//test10:target3", 2000) + writeFile(cachePath(target3, false), 2000) + setMtime(t, cachePath(target3, false), 2*time.Hour) + // Should remove the least recently used entry first, which is target2. + assert.EqualValues(t, 12000, cache.clean(15000, 13000)) + assert.True(t, inCache(target1)) + assert.False(t, inCache(target2)) + assert.True(t, inCache(target3)) +} + +func TestCleanLargestFirstWithinGracePeriod(t *testing.T) { + cache := makeCache(".plz-cache-test12", false) + target1 := makeTarget2("//test12:target1", 1000) + writeFile(cachePath(target1, false), 1000) + setMtime(t, cachePath(target1, false), 5*time.Minute) + target2 := makeTarget2("//test12:target2", 3000) + writeFile(cachePath(target2, false), 3000) + target3 := makeTarget2("//test12:target3", 2000) + writeFile(cachePath(target3, false), 2000) + setMtime(t, cachePath(target3, false), 4*time.Minute) + // These were all used within the grace period of each other, so it should remove the largest + // entry first (target2), even though it's the most recently used. + assert.EqualValues(t, 9000, cache.clean(15000, 13000)) + assert.True(t, inCache(target1)) + assert.False(t, inCache(target2)) + assert.True(t, inCache(target3)) +} + +func TestRetrieveUpdatesMtime(t *testing.T) { + cache := makeCache(".plz-cache-test11", false) + target := makeTarget2("//test11:target1", 20) + cache.Store(target, hash, target.Outputs(nil)) + entry := filepath.Dir(cachePath(target, false)) + // Not updated if it was recently modified + setMtime(t, cachePath(target, false), 30*time.Minute) + assert.True(t, cache.Retrieve(target, hash, target.Outputs(nil))) + assert.True(t, time.Since(getMtime(t, entry)) > 20*time.Minute) + // But it is updated if it's older + setMtime(t, cachePath(target, false), 2*time.Hour) + assert.True(t, cache.Retrieve(target, hash, target.Outputs(nil))) + assert.True(t, time.Since(getMtime(t, entry)) < time.Minute) +} + func TestStoreAndRetrieveCompressed(t *testing.T) { cache := makeCache(".plz-cache-test6", true) target := makeTarget2("//test6:target6", 20) @@ -147,6 +230,20 @@ func TestCleanCompressed(t *testing.T) { assert.True(t, inCompressedCache(target2)) } +// setMtime sets the modification time of the cache entry containing the given file to the given time ago. +func setMtime(t *testing.T, filename string, ago time.Duration) { + t.Helper() + then := time.Now().Add(-ago) + assert.NoError(t, os.Chtimes(filepath.Dir(filename), then, then)) +} + +func getMtime(t *testing.T, filename string) time.Time { + t.Helper() + info, err := os.Stat(filename) + assert.NoError(t, err) + return info.ModTime() +} + func makeCache(dir string, compress bool) *dirCache { config := core.DefaultConfiguration() config.Cache.Dir = dir diff --git a/third_party/go/BUILD b/third_party/go/BUILD index 213a932a0..5a8034aed 100644 --- a/third_party/go/BUILD +++ b/third_party/go/BUILD @@ -164,11 +164,6 @@ go_repo( version = "v0.0.0-20260706201446-f0a921348800", ) -go_repo( - module = "github.com/djherbis/atime", - version = "v1.1.0", -) - go_repo( module = "github.com/hashicorp/go-retryablehttp", version = "v0.7.8",