Remove or avoid redundant work to speed up zkg#243
Open
bbannier wants to merge 5 commits into
Open
Conversation
On every `Manager` construction, `Source.__init__` was unconditionally running `git_checkout` on each source clone, spawning three Git subprocesses (`checkout`, `submodule sync --recursive`, `submodule update --recursive --init`) even when the working tree was already at the target ref. Only call `git_checkout` when the current branch/SHA differs from the target.
`test()` calls `self.info(pkg_path)` and then `validate_dependencies()` calls `self.info(name)` again for the same package with identical arguments. For git-backed packages this means two scratch clones of the same repo. Add `_info_cache` on `Manager` and extract `_info_lookup()` so `info()` returns a cached `PackageInfo` on repeated calls with the same arguments.
`zeek --build-info` ran on every `Manager` construction with no cross-run persistence. Cache the result to `state_dir/zeek_build_info_cache.json`, keyed on the `zeek` binary path, mtime, and size, so subsequent runs with an unchanged binary skip the subprocess entirely. This does not show up in btest benchmarks because each test runs with a fresh `state_dir`, so the cache is always cold. The benefit is real for users: the subprocess is paid on every `zkg` command and is free on all subsequent ones once the binary is unchanged.
`_info()` clones a package into `scratch_dir` solely to read its metadata (`zkg.meta`), version tags, and default branch. Neither submodule content nor a fully initialised submodule tree is needed for this. The code called `_clone_package()` with `recursive=True` (fetching all submodules at clone time) and `git_checkout()` with `update_submodules=True` (running `git submodule sync --recursive` and `git submodule update --recursive --init`), causing up to four redundant network roundtrips per submodule for packages that would later be cloned again into the test or install staging directory. Hardcode `recursive=False` and `update_submodules=False` in `_info()` since the scratch clone is throwaway and metadata-only. Remove the now-unused `update_submodules` parameter from `_info()`, `_info_lookup()`, and `info()`.
The scratch clone exists only to read `zkg.meta`, version tags, and the default branch; none of which require full history. `_clone_package()` used a deep clone whenever a specific version was requested (to safely `git checkout` arbitrary historical commits), but that conservatism is only needed for the operational clone used during install/test staging. Inline `delete_path` + `git_clone(..., shallow=True, recursive=False)` directly in `_info()` so the scratch clone is always a `--depth 1 --no-single-branch` clone regardless of the requested version. This does not show up in btest benchmarks because test packages are local repos created with 1-2 commits -- there is no history to truncate. The benefit is real for users running `zkg info` or `zkg install` against remote packages with non-trivial history, where a shallow clone avoids transferring the full object graph.
bbannier
marked this pull request as ready for review
July 22, 2026 10:32
timwoj
approved these changes
Jul 22, 2026
timwoj
left a comment
timwoj
left a comment
Member
There was a problem hiding this comment.
Looks great to me. I had one question that was the resolved in a subsequent commit. Much better 👍🏼
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
All code changes in this branch were generated with Claude Sonnet 4.6. I reviewed all changes and am accountable.
This PR contains a number of unrelated cleanups which make zkg more snappy. Since I used timing of the BTest suite as a benchmark there might be more lingering improvements which are not covered by tests, or do not show up clearly. The changes fall into two categories:
On my machine I see the following runtime changes of the BTest test suite:
Like mentioned, I'd expect the commits without improvement to simply be not captured by the test suite (the increase at Disk-persist zeek --build-info seems consistent with noise).
Even though all code here was generated by an LLM, the code changes seem pretty straightforward. Some new unit tests tend to go pretty heavy on mocking, but if we agree that they are still readable I think this can be justified since they are meant to capture regressions where we accidentally introduce calls of expensive operations again.