[perf] in syncPackagesToProfile, only removeExtraItemsFromProfile is mode is not install

[savil]

Summary

Doing devbox global add <package> was taking 9 seconds for me. Most of this time
was spent in removeExtraPackagesFromProfile. This operation takes longer
because it calls devpkg.NormalizedPackageAttributePath which calls nix.Search
which is slow.

Instead, I advocate for us to not do this step when adding packages.

How was it tested?

devbox global add <package> is now snappy

[savil]

Current dependencies on/for this PR:

• main
  • PR [perf] skip forcing printDevEnv when add/rm/update outside shellenv #1540
    • PR [perf] in syncPackagesToProfile, only removeExtraItemsFromProfile is mode is not install #1541 👈

This comment was auto-generated by Graphite.

[mikeland73]

What if we reintroduce the heuristic where we only remove extra packages if the counts don't match. Or better yet, only if profile has more than devbox.json

[savil]

🤔 I think but need to verify that'll break the update scenario where the same package needs to be removed and added from profile

[savil]

@mikeland73 looking at this again, I do think the update scenario where the "resolved" store-path of the package has changed will not work if we apply the heuristic.

The old heuristic only worked b/c we did add-then-remove, but we now do remove-then-add:

if len(packages) == len(profileItems) {
	// Optimization: skip comparison if number of packages are the same. This only works
	// because we assume that all packages in `devbox.json` have just been added to the
	// profile.
	return nil, nil
}

Strictly speaking this also affected the update scenario, but it depends on which store-path got higher priority...

I think skipping removeExtraPackages for install is okay. We already skip addPackages for uninstall.

That said, I want to make this faster for ensure mode. In particular, I'm thinking we could have a local file cache map of "resolved-nixpkgs-commit-hash" to "NormalizedPackageAttributePath". IIUC this doesn't change and is a one-time calculation for that specific particular resolution of the nix package. EDIT: see #1546

[mikeland73]

@savil do you think this is still needed after #1546 ?

This is OK (which is why I'm approving) but it has 2 downsides:

• We no longer fully sync packages on add so if a package is removed manually from devbox.json this won't remove it from profile on add.
• This adds complexity depending on mode which makes ensure and sync harder to reason about.

If after landing #1546 this is still an improvement, lets go for it. But my impression is that removeExtraItemsFromProfile is probably slow due to search.

Thoughts?

[mikeland73]

remove

[savil]

Likely not needed as much. I agree it adds some complexity so avoidable. That said, the complexity is already there for uninstall. Will keep it in abeyance.

[savil]

Since #1546 needs more work and/or exploration, I'm going to land this.

The scenario of a package being added manually in devbox.json, but then not being added via devbox add is fairly remote, and I think we can live with it until the next release. Once #1546 or its successor lands, then we can remove the special-casing we have for install and uninstall skipping work in this syncPackagesToProfile.