core: cache turn diff rendering - #27489
Conversation
At a high level, the old tracker stored file contents and regenerated every file’s diff whenever 1. Contents Get Revisions struct TrackedContent {
content: String,
revision: u64,
}Previously, The revision is a cheap content identity. Instead of hashing or comparing multi-megabyte strings to decide whether a cached diff is current, the tracker compares integers. Revisions are allocated here: fn tracked_content(&mut self, content: &str) -> TrackedContent {
let revision = self.next_revision;
self.next_revision += 1;
TrackedContent {
content: content.to_string(),
revision,
}
}Every recorded mutation gets a new revision, even if its text happens to equal an earlier version. That conservatively invalidates the touched file’s cache entry. 2. Per-File Cache Keys struct DiffCacheKey {
left_path: TrackedPath,
left_revision: Option<u64>,
right_path: TrackedPath,
right_revision: Option<u64>,
}A rendered file diff depends on four facts:
Both paths are necessary because moves and renames can produce a diff from The revisions are optional because one side may not exist:
Including 3. New Tracker State The important additions are: next_revision: u64,
rendered_diffs: HashMap<DiffCacheKey, Option<String>>,
unified_diff: Option<String>,
4. Updates Maintain the Cache pub fn track_delta(&mut self, environment_id: &str, delta: &AppliedPatchDelta) {
if !self.valid {
return;
}
if !delta.is_exact() {
self.invalidate();
return;
}
for change in delta.changes() {
self.apply_change(environment_id, change);
}
self.refresh_unified_diff();
}Previously, Now it:
This moves the expensive work to the mutation boundary. Any number of later reads, including turn completion, reuse that result. The early 5. Invalidation Clears Observable State pub fn invalidate(&mut self) {
self.valid = false;
self.rendered_diffs.clear();
self.unified_diff = None;
}Previously, invalidation only set Now readers consume cached state directly, so invalidation must clear both caches. Setting The per-file cache is cleared because none of its entries are useful once the tracker can no longer produce an authoritative result. 6. Reading Is Cheap pub fn get_unified_diff(&self) -> Option<String> {
self.unified_diff.clone()
}
pub(crate) fn has_unified_diff(&self) -> bool {
self.unified_diff.is_some()
}This is the central behavioral change. Before,
Now it clones an already-computed result.
One nuance: because the public method returns an owned 7. Incremental Refresh The beginning is intentionally similar to the old renderer:
The cache logic begins here: let mut previous_diffs = std::mem::take(&mut self.rendered_diffs);
let mut rendered_diffs = HashMap::new();
For each logical file, it creates a cache key from paths and revisions: let key = DiffCacheKey {
left_path: left_path.clone(),
left_revision: left_content.map(|content| content.revision),
right_path: right_path.clone(),
right_revision: right_content.map(|content| content.revision),
};Then it either reuses the old result or renders: let rendered = previous_diffs.remove(&key).unwrap_or_else(|| {
self.render_diff(...)
});Suppose a turn has already changed
The refreshed entries are inserted into a new map. Any old entries that no longer correspond to current state are naturally discarded. Finally: self.rendered_diffs = rendered_diffs;
self.unified_diff = (!aggregated.is_empty()).then_some(aggregated);The aggregate is still rebuilt after each committed patch because existing event semantics require the complete updated turn diff. The expensive file comparison and hashing are reused; aggregate concatenation remains linear in the current output size. This also handles net-zero state correctly. If every file’s baseline equals its final content, 8. Rendering Is Time-Bounded Old code: similar::TextDiff::from_lines(old, new)New code: let mut config = similar::TextDiff::configure();
config.timeout(DIFF_TIMEOUT);
let unified = config.diff_lines(old, new);The timeout is defined as 100 ms: Myers can become extremely slow for large, mostly rewritten files. The observed 48,000-line case spent roughly 10.6 seconds inside the algorithm. With the timeout, “Coarser” only means fewer intelligently matched unchanged regions. It does not mean approximate content: applying the resulting unified diff still produces the exact final file. The regression test verifies that through the repository’s patch implementation. The deadline is per newly rendered file, not per entire delta. A patch changing several pathological files could consume roughly 100 ms per file. 9. Removed Rendering Wrappers The old They merely selected baseline/current contents before calling Resulting Execution Flow After this branch, one patch follows this path: The key improvement is that work scales primarily with files changed by the latest patch, rather than repeatedly running Myers over every file changed during the entire turn. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56421478f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .await; | ||
| } | ||
|
|
||
| #[tokio::test] |
There was a problem hiding this comment.
Adding these tests inline keeps growing events.rs after it crosses ~800 LoC; repo guidance says oversized Rust modules should get new code in a new module absent a documented reason. Please move this test coverage to a sibling *_tests.rs file or another scoped module. guidance
Useful? React with 👍 / 👎.
|
|
||
| rx_event.recv().await.expect("item completed event"); | ||
| let unified_diff = loop { | ||
| let event = rx_event.recv().await.expect("turn diff event"); |
There was a problem hiding this comment.
If a regression causes emit_patch_end to skip TurnDiff, this loop never terminates because the session side of the channel is still held by the test, so recv().await won't return None; the test will hang until the whole test runner times out. Wrap the wait in a timeout or assert after a bounded number of events.
Useful? React with 👍 / 👎.
| #[cfg(test)] | ||
| fn rendered_diff_count(&self) -> usize { | ||
| self.rendered_diff_count.get() | ||
| } |
There was a problem hiding this comment.
This #[cfg(test)] accessor exists only so tests can inspect private state, leaving test-only API in the main implementation despite repo guidance to keep unit tests in dedicated *_tests.rs files and avoid test-only functions there. Prefer public behavior assertions or test-file scaffolding. guidance
Useful? React with 👍 / 👎.
| .expect("complete rewrite should produce a diff"); | ||
|
|
||
| assert!( | ||
| started.elapsed() < Duration::from_secs(2), |
There was a problem hiding this comment.
On slower CI machines or under load, this hard 2-second wall-clock assertion can fail even when the timeout behavior is functionally correct, since the test still builds and renders a 48k-line rewrite before checking elapsed time. That turns a performance guard into a flaky codex-core test; prefer asserting the generated diff behavior or using a much less environment-sensitive bound.
Useful? React with 👍 / 👎.
Summary
Turn diff updates repeatedly rendered and serialized the entire accumulated diff after every
apply_patch. The event path also rendered once before updating the tracker solely to test whether a diff existed. In production feedback CODEX-20PW, 2,589 patches across 72 paths produced 401 notifications totaling 441 MB, with the hottest paths patched 518 and 495 times.This change:
similartimeout; ordinary files complete far below this threshold, while pathological rewrites fall back to a coarse unified hunk that still represents the exact final contentsThe 100 ms deadline bounds synchronous tool-completion latency while leaving substantial headroom for normal diffs. The regression test applies the fallback diff through the repository's patch parser and verifies byte-for-byte final contents.
Validation
cargo test -p codex-core turn_diff_tracker::tests(14 passed)cargo test -p codex-core tools::events::tests(4 passed)just fix -p codex-corejust fmtFocused coverage verifies that 42 updates across two files perform 42 file renders rather than repeatedly rendering the accumulated set, unchanged paths are not re-diffed, clear events remain correct, and a 48,000-line near-total rewrite returns promptly and applies to the exact expected result. The full
codex-coresuite was not used as the final gate because an unrelated existing multi-agent test hit a stack overflow when run during investigation.Bug context
019eb2a9-13d2-74e0-b690-27ee224ffb6d,019e9ad7-09c3-7cb2-b728-ee3acba103ab