perf: opt-in blob sizes, single-parse load, parallel stats/report - #14
Merged
Conversation
Three independent performance changes plus their docs/tests. Extract — blob-size resolution is now opt-in (--blob-sizes, default off): git cat-file --batch-check no longer runs by default and old_size/new_size (omitempty) drop from the JSONL. No stat consumes those fields, and the LRU cache had already made the lookup cheap, so this is a dead-code / IO / memory cleanup, not a speedup — extract is bound by the git log stream. Load — each JSONL line is type-detected from its prefix and unmarshalled once instead of twice (peekType, with a full-parse fallback for foreign or reordered lines). Roughly halves the load phase's parse work. Compute — report (Generate) and the stats --format json path now run their independent metric passes concurrently; FileHotspots is computed once and shared by the table and the repo tree instead of 3x. Impact (12-core, isolated): stats/report -27% to -49% vs v2.11.0 (e.g. WordPress report 3.23s -> 1.76s; Linux 1m29s -> 49s). Output is byte-identical to v2.11.0 across Pi-hole, WordPress, Kubernetes, Praat and self (stats JSON + report HTML), and load counts match exactly on all six fixtures incl. Linux (1,438,634 commits). Tests: peekType (+fallback), blob-size gating, and parallel-determinism guards for both report and stats-JSON (12x byte-identical, under -race). Docs: PERF.md tables refreshed and the incorrect "blocks on the cat-file pipe" claim corrected with a control measurement; README perf table and the new --blob-sizes flag documented. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53ce88f9fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
old_size/new_size were scalar int64 with omitempty, so a legitimately resolved 0 (an empty blob — e.g. an added empty file) was omitted exactly like the default no-lookup path. A --blob-sizes consumer could not tell a 0-byte blob from a missing size, breaking the flag's contract. Make them *int64: nil omits the field (not resolved, null hash, or blob sizes disabled), a non-nil pointer emits the value including 0. emitCommit sets the pointer only for hashes the resolver actually returned (two-value map lookup), so a null/unresolved hash stays absent while a real empty blob emits "...":0. Default-path output is unchanged (no size fields). Verified on pi-hole --blob-sizes: test/__init__.py (git's empty-blob hash e69de29) now emits "new_size":0; added files keep old_size absent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
1 participant
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.
Three independent performance changes plus their docs/tests.
Extract — blob-size resolution is now opt-in (--blob-sizes, default off): git cat-file --batch-check no longer runs by default and old_size/new_size (omitempty) drop from the JSONL. No stat consumes those fields, and the LRU cache had already made the lookup cheap, so this is a dead-code / IO / memory cleanup, not a speedup — extract is bound by the git log stream.
Load — each JSONL line is type-detected from its prefix and unmarshalled once instead of twice (peekType, with a full-parse fallback for foreign or reordered lines). Roughly halves the load phase's parse work.
Compute — report (Generate) and the stats --format json path now run their independent metric passes concurrently; FileHotspots is computed once and shared by the table and the repo tree instead of 3x.
Impact (12-core, isolated): stats/report -27% to -49% vs v2.11.0 (e.g. WordPress report 3.23s -> 1.76s; Linux 1m29s -> 49s). Output is byte-identical to v2.11.0 across Pi-hole, WordPress, Kubernetes, Praat and self (stats JSON + report HTML), and load counts match exactly on all six fixtures incl. Linux (1,438,634 commits).
Tests: peekType (+fallback), blob-size gating, and parallel-determinism guards for both report and stats-JSON (12x byte-identical, under -race). Docs: PERF.md tables refreshed and the incorrect "blocks on the cat-file pipe" claim corrected with a control measurement; README perf table and the new --blob-sizes flag documented.