Skip to content

Conversation

mikeland73
Copy link
Contributor

Summary

This changes bin wrappers to use eval $(devbox shellenv --use-cached-print-dev-env)

Motivation:

For performance reasons we were printing out the shellenv results in the bin wrappers. The problem with this approach is that the env vars in the wrappers overwrite whatever the user sets. For example:

USER=foo node -e "console.log(process.env.USER);"

will print mike because export USER="mike" is in the wrapper.

Solution:

  • Revert to using eval $(devbox shellenv) in the wrapper.
  • For performance, add hash to environment and skip evaluation if the hash matches (this means shellenv has already been eval-ed).
  • For performance add new --use-cached-print-dev-env flag to shellenv that uses a cached copy of nix print-dev-env if it exists. The cache is regenerated every time computeNixEnv is called so it happens on shell, run, adding/removing package, when wrappers are created, and also when shellenv is called without the flag.
  • For performance added DO_NOT_TRACK to shellenv call in wrapper. This can be removed once segment moves to queuing instead of blocking.

Latency impact when hash matches: None, or maybe a bit < 1ms faster.
Latency impact when hash doesn't match: ~20ms slower

Potential additional optimization 1: skip launcher when calling devbox shellenv. This should reduce about ~10ms of latency.
Potential additional optimization 2: when hash matches, instead of saving a wrapper we could save a symlink. This would eliminate extra latency of wrapper.

How was it tested?

  • Tested by adding and removing packages while in shell.
  • Timed calls to hello
  • Existing unit tests that test shellenv, bin wrappers, etc

@mikeland73 mikeland73 requested review from gcurtis and ipince April 7, 2023 07:10
@mikeland73
Copy link
Contributor Author

@ipince @gcurtis PTAL when you get a chance


// minor optimization. If we've already computed the non-cache version, use
// that instead.
if nixEnvCache == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be:

Suggested change
if nixEnvCache == nil {
if nixEnvCache != nil {

Comment on lines 1032 to 1041
func addHashToEnv(env map[string]string) map[string]string {
data := []byte{}
for k, v := range env {
data = append(data, []byte(k)...)
data = append(data, []byte(v)...)
}
hash := md5.Sum(data)
env[devboxShellEnvHashVarName] = hex.EncodeToString(hash[:])
return env
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

data can be removed by hashing as you go:

Suggested change
func addHashToEnv(env map[string]string) map[string]string {
data := []byte{}
for k, v := range env {
data = append(data, []byte(k)...)
data = append(data, []byte(v)...)
}
hash := md5.Sum(data)
env[devboxShellEnvHashVarName] = hex.EncodeToString(hash[:])
return env
}
func addHashToEnv(env map[string]string) map[string]string {
h := md5.New()
for k, v := range env {
h.Write([]byte(k))
h.Write([]byte(v))
}
env[devboxShellEnvHashVarName] = hex.EncodeToString(md5.Sum(data))
return env
}

If you want, the return value can also be removed since the function is modifying the map directly.

Comment on lines 112 to 120
if _, err := os.Stat(args.PrintDevEnvCachePath); err == nil {
data, err = os.ReadFile(args.PrintDevEnvCachePath)
if err != nil {
return nil, errors.WithStack(err)
}
if err := json.Unmarshal(data, &out); err != nil {
return nil, errors.WithStack(err)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the os.Stat call and just trying reading:

Suggested change
if _, err := os.Stat(args.PrintDevEnvCachePath); err == nil {
data, err = os.ReadFile(args.PrintDevEnvCachePath)
if err != nil {
return nil, errors.WithStack(err)
}
if err := json.Unmarshal(data, &out); err != nil {
return nil, errors.WithStack(err)
}
}
cachedData, err := os.ReadFile(args.PrintDevEnvCachePath)
if err == nil {
if err := json.Unmarshal(data, &out); err != nil {
return nil, errors.WithStack(err)
}
cachedData = data
} else if !errors.Is(err, fs.ErrNotExist) {
return nil, errors.WithStack(err)
}

@mikeland73 mikeland73 merged commit 7d92683 into main Apr 10, 2023
@mikeland73 mikeland73 deleted the landau/use-shell-env-command branch April 10, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants