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: Experimental environment activation cache #2367

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ruben-arts
Copy link
Contributor

@ruben-arts ruben-arts commented Oct 29, 2024

Disclaimer: Most of this code is copied from #1832 build by @borchero, but the merge conflicts made me copy it onto main by hand, I did do some small changes so it's not a one-on-one. But the "work" was done by @borchero.

This PR will add a .pixi/activation-env-v0 folder after activation, in which it can store the environment activation cache.

The cache

The cache consists of an EnvironmentHash this is the same hash as the one used in the task_cache. It's build by hasing the activation scripts, the environment data from the lockfile including all package.url's.
Next to the hash it contains the full set of environment variables, that are the result of the initial activation.

The invalidation

This cache is currently not used when:

  • If no lock_file is given to the activator it will skip checking for a cache
  • If it can't fine the cache file
  • If the cached EnvironmentHash is not similar to the EnvironmentHash of the activation. (Generated on the go)
    • The package urls of that environment in the lockfile are hashed
    • The state of the environment variables that are overwritten by the activation are hashed
    • The [activation] table is hashed

Enable the feature

This is an experimental feature (a first for pixi)
You can test it by:

# Setting experimental in config
# For all your projects
pixi config set experimental.use-environment-activation-cache true --global

# For a specific project
pixi config set experimental.use-environment-activation-cache true --local

To disable it you can force it to always activate, which only makes sense if you turn on experimental

pixi run --force-activate
pixi shell --force-activate
pixi shell-hook --force-activate

BREAKING task_cache

I've had to add activation.env to the EnvironmentHash, which will break the existing environment hashes on people projects. To me this is the right thing to do. It will not update if you don't have [activation.env] so it is not all project being effected..

TODO

  • Document this behavior
  • Test
  • Allow for ignore of the cache through cli and global setting
  • Allow for pixi cleaning only this cache
  • Make it experimental opt-in
  • Remove cli flag experimental

Most of this code is copied from prefix-dev#1832 build by @borchero but the merge conflicts made me copy it onto main by hand.
@ruben-arts ruben-arts changed the title feat: add environment variable cache to the activation logic. perf: cache environment activations Oct 29, 2024
@wolfv
Copy link
Member

wolfv commented Oct 29, 2024

I think your profiling showed that a major bottleneck of pixi run is checking wether the environment is up-to-date or not, and specifically reading the prefix records.

We could relatively easily switch to a different format for the data (e.g. a binary format). I also think that reading the PrefixRecord is probably mostly slow because you have a lot of path data in the prefix records. I think it would be nice to quantify the slowness and make a case for speeding that up! Together with the activation cache pixi run should then be blazing fast :)

@ruben-arts
Copy link
Contributor Author

ruben-arts commented Oct 29, 2024

❯ hyperfine "old-pixi run echo hello" "pixi run --experimental echo hello" "pixi run --experimental --force-activate echo hello" -r 20
Benchmark 1: old-pixi run echo hello
  Time (mean ± σ):      1.085 s ±  0.073 s    [User: 0.525 s, System: 0.768 s]
  Range (min … max):    0.950 s …  1.233 s    20 runs
 
Benchmark 2: pixi run --experimental echo hello
  Time (mean ± σ):     635.0 ms ±  44.5 ms    [User: 334.1 ms, System: 431.7 ms]
  Range (min … max):   500.0 ms … 710.5 ms    20 runs
 
Benchmark 3: pixi run --experimental --force-activate echo hello
  Time (mean ± σ):      1.057 s ±  0.138 s    [User: 0.522 s, System: 0.733 s]
  Range (min … max):    0.731 s …  1.262 s    20 runs
 
Summary
  pixi run --experimental echo hello ran
    1.66 ± 0.25 times faster than pixi run --experimental --force-activate echo hello
    1.71 ± 0.17 times faster than old-pixi run echo hello

Profiled, with and without cache and compared to old pixi (main)

@ruben-arts ruben-arts changed the title perf: cache environment activations perf: Experimental environment activation cache Nov 5, 2024
Hofer-Julian added a commit that referenced this pull request Nov 6, 2024
This pr will add a hash of the lock file to the `EnvironmentFile`
(`.pixi/envs/default/conda-meta/pixi`)

It will look like this:
```json
{
  "manifest_path": "/home/rarts/dev/pixi/pixi.toml",
  "environment_name": "default",
  "pixi_version": "0.34.0",
  "environment_lock_file_hash": "4f36ee620f10329d"
}
```
And that hash will be compared with the current lockfile on `pixi run`
`pixi shell` `pixi shell-hook`.

### Profile result

```shell
❯ hyperfine "pixi run echo" "old-pixi run echo"
Benchmark 1: pixi run echo
  Time (mean ± σ):     381.6 ms ±  22.1 ms    [User: 193.8 ms, System: 246.3 ms]
  Range (min … max):   344.5 ms … 414.3 ms    10 runs
 
Benchmark 2: old-pixi run echo
  Time (mean ± σ):     868.2 ms ±  58.2 ms    [User: 480.0 ms, System: 557.0 ms]
  Range (min … max):   791.1 ms … 950.8 ms    10 runs
 
Summary
  pixi run echo ran
    2.28 ± 0.20 times faster than old-pixi run echo
```
> [!NOTE]  
> The remaining `381ms` is the activation which is fixed by
#2367

### UX
- It's turned on by default
- You can request a re-validate on `pixi run/shell/shell-hook` with
`--revalidate`
- All commands designed to update the lock file or `pixi install` will
always re-validate.

### TODO:

- [x] : Add tests: chosen python integration tests as I was to fed up
with the extreme amount of time spent on writing tests in Rust
- [x] : use Enum instead of booleans. Using `UpdateMode::QuickValidate`
and `UpdateMode::Revalidate`
- [x] : Document behavior
- [x] : Extend logic to cli. `--revalidate`

---------

Co-authored-by: Hofer-Julian <[email protected]>
Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Looks great, awesome improvement @ruben-arts!
I only had small comments.

I could also reproduce the speedup:

nu ❯ hyperfine "pixi run echo hello" "pixi-activation-cache run echo hello" "pixi-activation-cache run --force-activate echo hello" -r 20 --warmup 1
Benchmark 1: pixi run echo hello
  Time (mean ± σ):     946.2 ms ±   6.4 ms    [User: 458.8 ms, System: 800.2 ms]
  Range (min … max):   934.6 ms … 961.9 ms    20 runs
 
Benchmark 2: pixi-activation-cache run echo hello
  Time (mean ± σ):     538.6 ms ±   4.4 ms    [User: 265.5 ms, System: 393.9 ms]
  Range (min … max):   529.8 ms … 545.1 ms    20 runs
 
Benchmark 3: pixi-activation-cache run --force-activate echo hello
  Time (mean ± σ):     906.2 ms ±   7.6 ms    [User: 419.1 ms, System: 672.2 ms]
  Range (min … max):   894.5 ms … 922.2 ms    20 runs
 
Summary
  pixi-activation-cache run echo hello ran
    1.68 ± 0.02 times faster than pixi-activation-cache run --force-activate echo hello
    1.76 ± 0.02 times faster than pixi run echo hello

names
.into_iter()
.map(|name| {
let value = std::env::var(name).unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need to cache environment variables that are not on the system?

If yes, an None would be nicer than an empty string

/// If it can get the cache, it will validate it with the lock file and the current environment.
/// If the cache is valid, it will return the environment variables from the cache.
async fn try_get_valid_activation_cache(
lock_file: Option<&LockFile>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lock_file: Option<&LockFile>,
lock_file: &LockFile,

This makes the expectation of the function clearer

Comment on lines +209 to +212
if cache.hash == hash {
return Some(cache.environment_variables);
}
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if cache.hash == hash {
return Some(cache.environment_variables);
}
None
if cache.hash == hash {
Some(cache.environment_variables)
} else {
None
}

Comment on lines +107 to +112
with cache_path.open("r+") as f:
contents = f.read()
new_contents = contents.replace("test123", "test456")
f.seek(0) # Move pointer to start of the file
f.write(new_contents)
f.truncate() # Remove any remaining original content
Copy link
Contributor

Choose a reason for hiding this comment

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

Deserializing that to a dict, doing the transformations there and serializing it again would make this more readable I think :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants