perf: reuse Conda history while parsing environments (Fixes #475) - #493
Conversation
Read conda-meta/history once per environment and share the snapshot across manager, package, and name parsing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Test Coverage Report (Linux)
Coverage increased! Great work! |
Performance Report (Linux) ✅
Legend
|
Performance Report (macOS)
Legend
|
Test Coverage Report (Windows)
Coverage increased! Great work! |
Performance Report (Windows) ➖
Legend
|
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant synchronous I/O during Conda environment discovery by reading conda-meta/history once per environment parse and reusing that snapshot for multiple parsing steps. This aligns with PET’s performance goals by minimizing filesystem reads in hot discovery paths.
Changes:
- Read
conda-meta/historyonce perCondaEnvironmentparse and thread the contents through conda-dir detection, env naming, and Python package metadata extraction. - Refactor
CondaPackageInfoto accept optional pre-read history content and optimize history scanning (use reverse search instead of collecting matches). - Add a regression test to ensure the environment-info path only invokes the history reader once.
Show a summary per file
| File | Description |
|---|---|
| crates/pet-conda/src/package.rs | Adds history-reuse entry points for package parsing and avoids extra allocations when selecting the most recent matching history entry. |
| crates/pet-conda/src/environments.rs | Reads history once per env parse, reuses the creation line for conda-dir and name derivation, and adds a “reads history once” regression test. |
Review details
Comments suppressed due to low confidence (2)
crates/pet-conda/src/environments.rs:410
- These tests now pass the entire history file contents into
get_conda_env_name'screation_lineparameter. That weakens the assertion becauseget_conda_env_name_from_creation_lineis intended to receive only the filtered# cmd: ... create ...line; passing full history could accidentally match-n <name>in unrelated commands. Extract the creation line viaget_conda_creation_line(&history)and passas_deref()instead.
let history = std::fs::read_to_string(&history_file).unwrap();
let name = get_conda_env_name(&env_path, &conda_dir, Some(&history));
crates/pet-conda/src/environments.rs:490
- These tests now pass the entire history file contents into
get_conda_env_name'screation_lineparameter. That weakens the assertion becauseget_conda_env_name_from_creation_lineis intended to receive only the filtered# cmd: ... create ...line; passing full history could accidentally match-n <name>in unrelated commands. Extract the creation line viaget_conda_creation_line(&history)and passas_deref()instead.
let history = std::fs::read_to_string(&history_file).unwrap();
let name = get_conda_env_name(&env_path, &conda_dir, Some(&history));
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
crates/pet-conda/src/environments.rs:55
- This function reads
conda-meta/historyonce for environment parsing, but the base environment can still cause a second read of the same file when manager discovery runs (e.g.,CondaManager::from(prefix)callsget_conda_installation_used_to_create_conda_env(path), which reads history again whenpathis the conda install/base env). This means the PR’s stated “reuse … for manager discovery” / “one read per env per find()” goal isn’t fully met for the base env case.
Consider either (a) avoiding history reads in CondaManager::from when path is already a conda install (is_conda_install(path) fast-path), or (b) plumbing the already-read history/creation line into manager detection (or a per-refresh history cache) so manager lookup can reuse it.
pub fn get_conda_environment_info(
env_path: &Path,
manager: &Option<CondaManager>,
) -> Option<CondaEnvironment> {
get_conda_environment_info_with_history_reader(env_path, manager, |env_path| {
std::fs::read_to_string(env_path.join("conda-meta").join("history")).ok()
})
crates/pet-conda/src/environments.rs:169
get_conda_creation_linelowercases each candidate line using full Unicode case-folding (to_lowercase()), which is relatively expensive for a hot-path parser. Since the history format is ASCII,to_ascii_lowercase()is sufficient and avoids the heavier Unicode mapping.
let line = history.lines().map(str::trim).find(|line| {
let line = line.to_lowercase();
line.starts_with("# cmd:") && line.contains(" create -")
})?;
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
Use ASCII-insensitive byte-stable matching and checked slicing for history command extraction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary - add a PET-wide Rust coding skill alongside locator-specific guidance - capture path identity/cache, Unicode-safe parsing, hot-path I/O/allocation, and cross-platform rules - require tests to prove claimed read-count, cache-hit, and event-count invariants - wire the recurring checks into the Reviewer agent for every Rust change ## Review retrospective Recent feedback clustered around: - normalized cache keys and caller-facing path preservation (#487, #490) - Unicode-safe byte indexing and ASCII format handling (#493) - proving optimization scope and read counts (#493) - duplicate side effects and precise pattern semantics (#495) - workflow assignment/merge postconditions, already addressed in #486 Fixes #496 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
conda-meta/historyonce per environment parseValidation
cargo test -p pet-conda.\scripts\rust-precommit.ps1Fixes #475