Optimize VCS diff loading to be up to 98% faster#2586
Optimize VCS diff loading to be up to 98% faster#2586justsomelegs wants to merge 21 commits intopingdotgg:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
ApprovabilityVerdict: Needs human review This PR restructures core VCS and checkpoint infrastructure, moving checkpoint operations into the Git driver and converting process spawning to Effect-native patterns. The architectural scope and changes to critical diff/checkpoint paths warrant human review despite being framed as a performance optimization. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
justsomelegs
force-pushed
the
t3code/9f93372b
branch
from
2fb898b to
d73ba82
Compare
This reverts commit 7c97ca3.
justsomelegs
force-pushed
the
t3code/9f93372b
branch
from
ae358ea to
01be4db
Compare
| const getFullThreadDiff: CheckpointDiffQueryShape["getFullThreadDiff"] = ( | ||
| input: OrchestrationGetFullThreadDiffInput, | ||
| ) => | ||
| Effect.gen(function* () { |
| * Read only the narrow context needed to compute a full-thread diff from | ||
| * checkpoint 0 to a specific turn count. | ||
| */ | ||
| readonly getFullThreadDiffContext?: ( |
| } | ||
| const resolveRepositoryIdentityCacheKey = Effect.fn("resolveRepositoryIdentityCacheKey")(function* ( | ||
| cwd: string, | ||
| runManagedProcess: ManagedProcessRunner, |
There was a problem hiding this comment.
nit: DI the ChildProcessSpawner (Effect.provideService) and runProcess like normal
| function joinPath(...parts: ReadonlyArray<string>): string { | ||
| return parts | ||
| .map((part, index) => { | ||
| if (index === 0) return part.replace(/[\\/]+$/g, ""); | ||
| return part.replace(/^[\\/]+|[\\/]+$/g, ""); | ||
| }) | ||
| .filter((part) => part.length > 0) | ||
| .join("/"); | ||
| } |
There was a problem hiding this comment.
what happened to path.join. ?
| truncatedMarker: input.appendTruncationMarker ? OUTPUT_TRUNCATED_MARKER : "", | ||
| timeoutBehavior: "error", | ||
| }).pipe( | ||
| Effect.mapError((cause) => { |
There was a problem hiding this comment.
effect pattern i sometimes use is adding static methods like
class VcsProcessSpawnError extends ... {
static fromProcessSpawnError = () => ...
}
sometimes ends up with cleaner business logic. Ref: https://github.com/pingdotgg/t3code/blob/main/packages/effect-acp/src/errors.ts#L50
There was a problem hiding this comment.
@effect/vitest and provide a test implemntation of the ChildProcessSpawner instead of writing scripts to FS?
3 participants
What Changed
CheckpointStore.VcsProcess.runandprocessRunneronto a sharedcapturedProcessprimitive for one-shot collected process execution.^{commit}) for checkpoint diff refsWhy
Opening diffs and switching turns was spending too much time in process execution overhead and checkpoint diff orchestration.
This branch improves that by:
Benchmark Summary
Synthetic workload used for all numbers below:
24changed files690,422patch bytes26,137patch linesBenchmarks were run sequentially to avoid contention between benchmark loops.
Mean Latency
upstream/maincheckpointStore.diffCheckpoints1179.81ms72.84mscheckpointDiffQuery.getTurnDiff1649.32ms52.05msparseTurnDiffFilesFromUnifiedDiff12.42ms11.97msTail Latency (
p99)upstream/maincheckpointStore.diffCheckpoints2262.90ms355.04mscheckpointDiffQuery.getTurnDiff2937.21ms191.04msparseTurnDiffFilesFromUnifiedDiff16.43ms19.63msKey Measurements
Compared directly with
upstream/main:checkpointStore.diffCheckpoints:1179.81ms->72.84msmean (16.20xfaster,93.8%lower)checkpointDiffQuery.getTurnDiff:1649.32ms->52.05msmean (31.69xfaster,96.8%lower)checkpointStore.diffCheckpoints:2262.90ms->355.04msp99checkpointDiffQuery.getTurnDiff:2937.21ms->191.04msp99Before
before.vcs.optimisation.mp4
AFTER
vsc.after.optimisations.mp4
Checklist
Note
Optimize VCS diff loading by delegating checkpoint operations to the Git driver
CheckpointStoreinto a newcheckpointsAPI onGitVcsDriver, eliminating redundant git subprocess calls and ref-existence preflights before diffing.processRunner.tswith an Effect-based API using aChildProcessSpawnerservice, typed errors (ProcessSpawnError,ProcessTimeoutError,ProcessOutputLimitError), structuredProcessRunInput/ProcessRunOutput, andoutputMode: 'truncate'support instead of rejecting on limit.CheckpointDiffQuery.getFullThreadDiffnow uses a dedicatedProjectionSnapshotQuery.getFullThreadDiffContextto look up the canonical turn-0 checkpoint ref directly, skipping per-ref existence checks.truncateOutputAtMaxBytestoappendTruncationMarkeracrossVcsProcess,GitVcsDriverCore, and all call sites to clarify the option's role.runProcessAPI is now Effect-based and requires aChildProcessSpawnerservice; all callers (terminal manager, repository identity resolver, environment label, source control discovery) have been updated but the signature change is breaking for any external consumers.Macroscope summarized 01be4db.