perf(devbox): skip redundant runx, script and state-hash work per env compute

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
 	}

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)
+}

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
 	}

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)
+}

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.
 //

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")
+	}
+}

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 #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 #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 #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.
 
 ---