From 3b45d30f666ce91f7c853a2e75b3ac9220ed2e94 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:28:40 -0800 Subject: [PATCH 1/2] perf(devbox): avoid redundant work on every env compute and devbox run - RunXPaths: don't touch .devbox/virtenv/runx/bin when the project has no runx packages and no leftover links. - Scripts: skip rewriting script files whose contents and mode already match. Keeps the temp-file + rename write (not overwriteFileIfChanged) so the concurrency/ETXTBSY fixes from #3032 and #3037 still hold. - Memoize lockfile.StateStatus on the Devbox for the duration of one ensureStateIsUpToDate[AndComputeEnv] call instead of computing it twice per shellenv. The cache is dropped when the call returns, after the non-Nix refresh, and before any config-changing (non-ensure) mode. Spec item 17 (smaller cleanups), env-compute/run bullets. Co-Authored-By: Claude Opus 5.5 --- internal/devbox/devbox.go | 36 ++++++++++++++++++- internal/devbox/devbox_test.go | 20 +++++++++++ internal/devbox/packages.go | 8 ++++- internal/devbox/statehash_test.go | 27 +++++++++++++++ internal/shellgen/scripts.go | 19 +++++++++++ internal/shellgen/scripts_test.go | 57 +++++++++++++++++++++++++++++++ specs/perf-improvements.md | 22 +++++++----- 7 files changed, 178 insertions(+), 11 deletions(-) create mode 100644 internal/shellgen/scripts_test.go diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 1856551f4df..09b641ee29b 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -72,6 +72,12 @@ type Devbox struct { // packagesBeingUpdated tracks which packages are being updated so that // installNixPackagesToStore only refreshes those, not all packages. packagesBeingUpdated []*devpkg.Package + + // cachedStateStatus memoizes lockfile.StateStatus for the duration of a + // single ensureStateIsUpToDate[AndComputeEnv] call, which would otherwise + // compute it (hashing the lockfile, config, print-dev-env cache and nix + // profile) more than once. See stateStatus. + cachedStateStatus *lock.StateStatus } var legacyPackagesWarningHasBeenShown = false @@ -903,8 +909,10 @@ func (d *Devbox) ensureStateIsUpToDateAndComputeEnv( envOpts devopt.EnvOptions, ) (map[string]string, error) { defer debug.FunctionTimer().End() + // The status computed here is reused by ensureStateIsUpToDate below. + defer d.invalidateStateStatus() - status, err := d.lockfile.StateStatus(isFishShell()) + status, err := d.stateStatus() if err != nil { return nil, err } @@ -914,6 +922,7 @@ func (d *Devbox) ensureStateIsUpToDateAndComputeEnv( if err := d.refreshNonNixState(ctx); err != nil { return nil, err } + d.invalidateStateStatus() status = lock.StateUpToDate } if status != lock.StateUpToDate { @@ -950,6 +959,26 @@ func (d *Devbox) ensureStateIsUpToDateAndComputeEnv( return d.computeEnv(ctx, true /*usePrintDevEnvCache*/, envOpts) } +// stateStatus returns lockfile.StateStatus, memoized until the next +// invalidateStateStatus. Callers that populate the cache must invalidate it +// when they return, so that the cached value never outlives a change to the +// config, lockfile or state. +func (d *Devbox) stateStatus() (lock.StateStatus, error) { + if d.cachedStateStatus != nil { + return *d.cachedStateStatus, nil + } + status, err := d.lockfile.StateStatus(isFishShell()) + if err != nil { + return status, err + } + d.cachedStateStatus = &status + return status, nil +} + +func (d *Devbox) invalidateStateStatus() { + d.cachedStateStatus = nil +} + func (d *Devbox) nixPrintDevEnvCachePath() string { return filepath.Join(d.projectDir, ".devbox/.nix-print-dev-env-cache") } @@ -1262,6 +1291,11 @@ func (d *Devbox) RunXPaths(ctx context.Context) (string, error) { } } } + // Most projects have no runx packages. Don't touch the filesystem for + // them unless a previous run left links behind that need removing. + if len(links) == 0 && !fileutil.Exists(runxBinPath) { + return runxBinPath, nil + } if err := syncSymlinks(runxBinPath, links); err != nil { return "", err } diff --git a/internal/devbox/devbox_test.go b/internal/devbox/devbox_test.go index dc0f65fc18c..8f16471c497 100644 --- a/internal/devbox/devbox_test.go +++ b/internal/devbox/devbox_test.go @@ -187,3 +187,23 @@ func TestSyncSymlinksConcurrent(t *testing.T) { require.NoError(t, err) assert.Len(t, entries, len(links)) } + +func TestRunXPathsWithoutRunXPackages(t *testing.T) { + dir := t.TempDir() + box := openStateTestProject(t, dir, stateTestConfig) + binDir := filepath.Join(dir, ".devbox", "virtenv", "runx", "bin") + + got, err := box.RunXPaths(t.Context()) + require.NoError(t, err) + assert.Equal(t, binDir, got) + assert.NoDirExists(t, binDir, "runx bin dir should not be created without runx packages") + + // Links left behind by a removed runx package are cleaned up. + require.NoError(t, os.MkdirAll(binDir, 0o755)) + require.NoError(t, os.Symlink("/nonexistent", filepath.Join(binDir, "stale"))) + _, err = box.RunXPaths(t.Context()) + require.NoError(t, err) + entries, err := os.ReadDir(binDir) + require.NoError(t, err) + assert.Empty(t, entries) +} diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 3cc331e55d2..af1ef263a83 100644 --- a/internal/devbox/packages.go +++ b/internal/devbox/packages.go @@ -358,7 +358,13 @@ func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) er defer trace.StartRegion(ctx, "devboxEnsureStateIsUpToDate").End() defer debug.FunctionTimer().End() - status, err := d.lockfile.StateStatus(isFishShell()) + if mode != ensure { + // The caller changed the config, so a cached status is stale. + d.invalidateStateStatus() + } + // This function changes the state, so drop the status when it returns. + defer d.invalidateStateStatus() + status, err := d.stateStatus() if err != nil { return err } diff --git a/internal/devbox/statehash_test.go b/internal/devbox/statehash_test.go index ec891dd13f5..1d7b9db79a1 100644 --- a/internal/devbox/statehash_test.go +++ b/internal/devbox/statehash_test.go @@ -162,3 +162,30 @@ func TestStateStatusNixChanges(t *testing.T) { }) } } + +func TestStateStatusCacheDoesNotOutliveCall(t *testing.T) { + dir := t.TempDir() + box := openStateTestProject(t, dir, stateTestConfig) + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".devbox"), 0o755)) + require.NoError(t, box.saveStateHashFile()) + + box = openStateTestProject(t, dir, `{ + "packages": ["github:NixOS/nixpkgs/nixpkgs-unstable#hello"], + "env": {"MY_VAR": "new"} +}`) + box.nix = &cacheOnlyNix{} + + _, err := box.ensureStateIsUpToDateAndComputeEnv(t.Context(), devopt.EnvOptions{}) + require.NoError(t, err) + assert.Nil(t, box.cachedStateStatus, "cached status must be dropped when the call returns") + status, err := box.stateStatus() + require.NoError(t, err) + assert.Equal(t, lock.StateUpToDate, status) + box.invalidateStateStatus() + + // A stale cached value must not survive a config-changing call. + stale := lock.StateUpToDate + box.cachedStateStatus = &stale + require.NoError(t, box.ensureStateIsUpToDate(t.Context(), noInstall)) + assert.Nil(t, box.cachedStateStatus) +} diff --git a/internal/shellgen/scripts.go b/internal/shellgen/scripts.go index d99f61ffca2..aeba6b6eed0 100644 --- a/internal/shellgen/scripts.go +++ b/internal/shellgen/scripts.go @@ -114,6 +114,13 @@ func WriteScriptFile(devbox devboxer, name, body string) error { // scripts, so writing in place could let one of them execute an empty, // partial, or not yet executable file. func writeScriptFileAtomic(path, body string) error { + // Scripts rarely change between runs, so skip the write when the file is + // already up to date. This deliberately doesn't use + // overwriteFileIfChanged: that writes in place, and another devbox + // process may be executing the file (see writeTempFile). + if scriptFileIsUpToDate(path, body) { + return nil + } tmp, err := writeTempFile(filepath.Dir(path), "."+filepath.Base(path)+"-*", body) if tmp != "" { defer os.Remove(tmp) // no-op after a successful rename @@ -127,6 +134,18 @@ func writeScriptFileAtomic(path, body string) error { return errors.WithStack(os.Rename(tmp, path)) } +// scriptFileIsUpToDate reports whether path is a regular file with mode 0755 +// and contents body. +func scriptFileIsUpToDate(path, body string) bool { + fi, err := os.Lstat(path) + if err != nil || !fi.Mode().IsRegular() || fi.Mode().Perm() != 0o755 || + fi.Size() != int64(len(body)) { + return false + } + data, err := os.ReadFile(path) + return err == nil && string(data) == body +} + // writeTempFile creates a temp file in dir, writes body to it, and closes it. // It returns the file's name whenever the file was created, even on error. // diff --git a/internal/shellgen/scripts_test.go b/internal/shellgen/scripts_test.go new file mode 100644 index 00000000000..92a84964ac7 --- /dev/null +++ b/internal/shellgen/scripts_test.go @@ -0,0 +1,57 @@ +// Copyright 2024 Jetify Inc. and contributors. All rights reserved. +// Use of this source code is governed by the license in the LICENSE file. + +package shellgen + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestWriteScriptFileAtomicSkipsUnchanged(t *testing.T) { + path := filepath.Join(t.TempDir(), "script.sh") + if err := writeScriptFileAtomic(path, "echo hi\n"); err != nil { + t.Fatal(err) + } + + // Backdate the file so that a rewrite is visible in its mtime. + old := time.Now().Add(-time.Hour).Truncate(time.Second) + if err := os.Chtimes(path, old, old); err != nil { + t.Fatal(err) + } + if err := writeScriptFileAtomic(path, "echo hi\n"); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if !fi.ModTime().Equal(old) { + t.Errorf("unchanged script was rewritten: mtime = %v, want %v", fi.ModTime(), old) + } + + // Wrong permissions force a rewrite. + if err := os.Chmod(path, 0o644); err != nil { + t.Fatal(err) + } + if err := writeScriptFileAtomic(path, "echo hi\n"); err != nil { + t.Fatal(err) + } + if fi, err := os.Stat(path); err != nil || fi.Mode().Perm() != 0o755 { + t.Errorf("got mode %v (err %v), want 0755", fi.Mode().Perm(), err) + } + + // Changed contents are written. + if err := writeScriptFileAtomic(path, "echo bye\n"); err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != "echo bye\n" { + t.Errorf("got %q, want %q", got, "echo bye\n") + } +} diff --git a/specs/perf-improvements.md b/specs/perf-improvements.md index 939a43e944d..7ab1b7bd419 100644 --- a/specs/perf-improvements.md +++ b/specs/perf-improvements.md @@ -261,15 +261,19 @@ Each ~5–20 LOC: - Run `nix flake prefetch` once instead of twice (`internal/nix/nixpkgs.go:39-78`): use `--json` on the first call and save the location from its output. -- `RunXPaths` (`internal/devbox/devbox.go:1196`) deletes and recreates - `.devbox/virtenv/runx/bin` on every env compute. Skip it when there are no - runx packages, and otherwise only rewrite changed links. -- `WriteScriptsToFiles` (`internal/shellgen/scripts.go:41`) rewrites all - scripts on every `devbox run`. Use the existing `overwriteFileIfChanged` - (`internal/shellgen/generate.go:106`). -- `IsUpToDateAndInstalled` runs 2–3 times per shellenv - (`internal/devbox/packages.go:310`, `internal/devbox/devbox.go:877`). - Memoize it per `Devbox` instance. +- (done in #PRNUM) `RunXPaths` (`internal/devbox/devbox.go`) deleted and + recreated `.devbox/virtenv/runx/bin` on every env compute. #3019 already made + it rewrite only changed links; it now also skips the filesystem entirely + when there are no runx packages and no leftover links. +- (done in #PRNUM) `WriteScriptsToFiles` (`internal/shellgen/scripts.go`) + rewrote all scripts on every `devbox run`. It now skips scripts whose + contents and mode already match. It doesn't use `overwriteFileIfChanged`, + because that writes in place and would bring back the races fixed in #3032 + and #3037. +- (done in #PRNUM) `StateStatus` (formerly `IsUpToDateAndInstalled`) ran twice + per shellenv (`ensureStateIsUpToDateAndComputeEnv` and + `ensureStateIsUpToDate`). It's now memoized on the `Devbox` for the duration + of one such call and dropped when the call returns or changes state. --- From cce9ad8871cd08ddb6e2e66e6bded0cb88573f4f Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:31:10 -0800 Subject: [PATCH 2/2] docs(spec): mark item 17 env-compute cleanups done in #3056 Co-Authored-By: Claude Opus 5.5 --- specs/perf-improvements.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/specs/perf-improvements.md b/specs/perf-improvements.md index 7ab1b7bd419..e2f1ec55b68 100644 --- a/specs/perf-improvements.md +++ b/specs/perf-improvements.md @@ -261,16 +261,16 @@ Each ~5–20 LOC: - Run `nix flake prefetch` once instead of twice (`internal/nix/nixpkgs.go:39-78`): use `--json` on the first call and save the location from its output. -- (done in #PRNUM) `RunXPaths` (`internal/devbox/devbox.go`) deleted and +- (done in #3056) `RunXPaths` (`internal/devbox/devbox.go`) deleted and recreated `.devbox/virtenv/runx/bin` on every env compute. #3019 already made it rewrite only changed links; it now also skips the filesystem entirely when there are no runx packages and no leftover links. -- (done in #PRNUM) `WriteScriptsToFiles` (`internal/shellgen/scripts.go`) +- (done in #3056) `WriteScriptsToFiles` (`internal/shellgen/scripts.go`) rewrote all scripts on every `devbox run`. It now skips scripts whose contents and mode already match. It doesn't use `overwriteFileIfChanged`, because that writes in place and would bring back the races fixed in #3032 and #3037. -- (done in #PRNUM) `StateStatus` (formerly `IsUpToDateAndInstalled`) ran twice +- (done in #3056) `StateStatus` (formerly `IsUpToDateAndInstalled`) ran twice per shellenv (`ensureStateIsUpToDateAndComputeEnv` and `ensureStateIsUpToDate`). It's now memoized on the `Devbox` for the duration of one such call and dropped when the call returns or changes state.