Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[perf] Improve install and ensure perf #2076

Merged
merged 5 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions internal/devbox/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ func (d *Devbox) packagesToInstallInStore(ctx context.Context, mode installMode)

// Second, check which packages are not in the nix store
packagesToInstall := []*devpkg.Package{}
storePathsForPackage := map[*devpkg.Package][]string{}
for _, pkg := range packages {
installables, err := pkg.Installables()
if err != nil {
Expand All @@ -549,16 +550,35 @@ func (d *Devbox) packagesToInstallInStore(ctx context.Context, mode installMode)
packagesToInstall = append(packagesToInstall, pkg)
continue
}
storePaths, err := nix.StorePathsFromInstallable(ctx, installable, pkg.HasAllowInsecure())

resolvedStorePaths, err := pkg.GetResolvedStorePaths()
if err != nil {
return nil, packageInstallErrorHandler(err, pkg, installable)
return nil, err
}
if resolvedStorePaths != nil {
storePathsForPackage[pkg] = append(storePathsForPackage[pkg], resolvedStorePaths...)
continue
}
isInStore, err := nix.StorePathsAreInStore(ctx, storePaths)

storePathsForPackage[pkg], err = nix.StorePathsFromInstallable(
ctx, installable, pkg.HasAllowInsecure())
if err != nil {
return nil, err
return nil, packageInstallErrorHandler(err, pkg, installable)
}
if !isInStore {
}
}

// Batch this for perf
storePathMap, err := nix.StorePathsAreInStore(ctx, lo.Flatten(lo.Values(storePathsForPackage)))
if err != nil {
return nil, err
}

for pkg, storePaths := range storePathsForPackage {
for _, storePath := range storePaths {
if !storePathMap[storePath] {
packagesToInstall = append(packagesToInstall, pkg)
break
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions internal/devpkg/narinfo_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,13 @@ func FillNarInfoCache(ctx context.Context, packages ...*Package) error {
group, _ := errgroup.WithContext(ctx)
for _, p := range eligiblePackages {
pkg := p // copy the loop variable since its used in a closure below
outputs, err := pkg.GetOutputs()
outputNames, err := p.outputs.GetNames(p)
if err != nil {
return err
}

for _, output := range outputs {
name := output.Name
for _, outputName := range outputNames {
name := outputName
group.Go(func() error {
_, err := pkg.fetchNarInfoStatusOnce(name)
return err
Expand Down Expand Up @@ -132,7 +132,6 @@ func (p *Package) areExpectedOutputsInCacheOnce(outputName string) (bool, error)
func (p *Package) fetchNarInfoStatusOnce(
outputName string,
) (map[string]string, error) {
defer debug.FunctionTimer().End()
ctx := context.TODO()

outputToCache := map[string]string{}
Expand Down
21 changes: 19 additions & 2 deletions internal/devpkg/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/samber/lo"
"go.jetpack.io/devbox/internal/boxcli/usererr"
"go.jetpack.io/devbox/internal/cachehash"
"go.jetpack.io/devbox/internal/debug"
"go.jetpack.io/devbox/internal/devbox/devopt"
"go.jetpack.io/devbox/internal/devconfig/configfile"
"go.jetpack.io/devbox/internal/devpkg/pkgtype"
Expand Down Expand Up @@ -679,7 +678,6 @@ func (p *Package) DocsURL() string {
// specified in devbox.json package fields or as part of the flake reference.
// If they exist in a cache, the cache URI is non-empty.
func (p *Package) GetOutputs() ([]Output, error) {
defer debug.FunctionTimer().End()
if p.IsRunX() {
return nil, nil
}
Expand Down Expand Up @@ -708,3 +706,22 @@ func (p *Package) GetOutputs() ([]Output, error) {
}
return outputs, nil
}

// GetResolvedStorePaths returns the store paths that are resolved (in lockfile)
func (p *Package) GetResolvedStorePaths() ([]string, error) {
names, err := p.outputs.GetNames(p)
if err != nil {
return nil, err
}
storePaths := []string{}
for _, name := range names {
outputs, err := p.outputsForOutputName(name)
if err != nil {
return nil, err
}
for _, output := range outputs {
storePaths = append(storePaths, output.Path)
}
}
return storePaths, nil
}
76 changes: 52 additions & 24 deletions internal/nix/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,49 +48,77 @@ func StorePathsFromInstallable(ctx context.Context, installable string, allowIns

return nil, err
}
return parseStorePathFromInstallableOutput(installable, resultBytes)

validPaths, err := parseStorePathFromInstallableOutput(resultBytes)
if err != nil {
return nil, fmt.Errorf("failed to parse path-info for %s: %w", installable, err)
}

return maps.Keys(validPaths), nil
}

// StorePathsAreInStore returns true if the store path is in the store
// It relies on `nix store ls` to check if the store path is in the store
func StorePathsAreInStore(ctx context.Context, storePaths []string) (bool, error) {
// StorePathsAreInStore a map of store paths to whether they are in the store.
func StorePathsAreInStore(ctx context.Context, storePaths []string) (map[string]bool, error) {
defer debug.FunctionTimer().End()
if len(storePaths) == 0 {
return map[string]bool{}, nil
}
args := append([]string{"path-info", "--offline", "--json"}, storePaths...)
cmd := commandContext(ctx, args...)
debug.Log("Running cmd %s", cmd)
output, err := cmd.Output()
if err != nil {
return nil, err
}

validPaths, err := parseStorePathFromInstallableOutput(output)
if err != nil {
return nil, err
}

result := map[string]bool{}
for _, storePath := range storePaths {
cmd := commandContext(ctx, "store", "ls", storePath)
debug.Log("Running cmd %s", cmd)
if err := cmd.Run(); err != nil {
if exitErr := (&exec.ExitError{}); errors.As(err, &exitErr) {
return false, nil
}
return false, err
}
_, ok := validPaths[storePath]
result[storePath] = ok
}
return true, nil

return result, nil
}

// Older nix versions (like 2.17) are an array of objects that contain path and valid fields
type pathInfoLegacy struct {
Path string `json:"path"`
Valid bool `json:"valid"`
}

// parseStorePathFromInstallableOutput parses the output of `nix store path-from-installable --json`
// This function is decomposed out of StorePathFromInstallable to make it testable.
func parseStorePathFromInstallableOutput(installable string, output []byte) ([]string, error) {
// Newer nix versions (like 2.20)
func parseStorePathFromInstallableOutput(output []byte) (map[string]any, error) {
// Newer nix versions (like 2.20) have output of the form
// {"<store-path>": {}}
// if a store path is used as an installable, the keys will be present even if invalid but
// the values will be null.
Comment on lines +99 to +100
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh good find!

var out1 map[string]any
if err := json.Unmarshal(output, &out1); err == nil {
return maps.Keys(out1), nil
maps.DeleteFunc(out1, func(k string, v any) bool {
return v == nil
})
return out1, nil
}

// Older nix versions (like 2.17)
var out2 []struct {
Path string `json:"path"`
Valid bool `json:"valid"`
}
var out2 []pathInfoLegacy

if err := json.Unmarshal(output, &out2); err == nil {
res := []string{}
res := map[string]any{}
for _, outValue := range out2 {
res = append(res, outValue.Path)
if outValue.Valid {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

res[outValue.Path] = true
}
}
return res, nil
}

return nil, fmt.Errorf("failed to parse store path from installable (%s) output: %s", installable, output)
return nil, fmt.Errorf("failed to parse path-info output: %s", output)
}

// DaemonError reports an unsuccessful attempt to connect to the Nix daemon.
Expand Down
8 changes: 5 additions & 3 deletions internal/nix/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package nix
import (
"slices"
"testing"

"golang.org/x/exp/maps"
)

func TestParseStorePathFromInstallableOutput(t *testing.T) {
Expand All @@ -19,18 +21,18 @@ func TestParseStorePathFromInstallableOutput(t *testing.T) {
},
{
name: "go-basic-nix-2-17-0",
input: `[{"path":"/nix/store/fgkl3qk8p5hnd07b0dhzfky3ys5gxjmq-go-1.22.0","valid":false}]`,
input: `[{"path":"/nix/store/fgkl3qk8p5hnd07b0dhzfky3ys5gxjmq-go-1.22.0","valid":true}]`,
expected: []string{"/nix/store/fgkl3qk8p5hnd07b0dhzfky3ys5gxjmq-go-1.22.0"},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual, err := parseStorePathFromInstallableOutput(tc.name, []byte(tc.input))
actual, err := parseStorePathFromInstallableOutput([]byte(tc.input))
if err != nil {
t.Errorf("Expected no error but got error: %s", err)
}
if !slices.Equal(tc.expected, actual) {
if !slices.Equal(tc.expected, maps.Keys(actual)) {
t.Errorf("Expected store path %s but got %s", tc.expected, actual)
}
})
Expand Down