agentHost: batch AHP log writes and skip URI deep clone#318864
Merged
roblourens merged 2 commits intoMay 29, 2026
Conversation
Two perf fixes for AhpJsonlLogger, the per-message JSONL transport logger. CPU traces of the agents window under heavy AHP traffic showed ~23% of wall time in MajorGC, dominated by VSBuffer allocations from renderer-side writeFile IPCs and the recursive _replaceUris deep clone applied to every message before JSON.stringify. - log() now appends to a pending buffer list and schedules a single drain on the write queue. While a writeFile is in flight, all subsequent log() calls accumulate and land in the next drain via VSBuffer.concat, capped at 1 MiB per write. Rotation is still checked per entry, and flush()/ordering semantics are preserved via a _drainScheduled invariant. - stringifyAhpLogEntry() now uses a JSON.stringify replacer instead of a tree-walking deep clone. URI.toJSON() stamps the output with $mid: MarshalledId.Uri, which the replacer detects (guarded by isUriComponents) and rewrites to the canonical URI string. This removes per-message allocation of a clone of the entire message payload. Adds tests for batching, flush ordering across drains, and URI replacer edge cases (nested URIs, raw UriComponents from prior toJSON, URI-shaped objects without $mid, and non-URI objects that happen to carry $mid: 1). (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves AhpJsonlLogger performance for high-volume Agent Host Protocol traffic by reducing per-message write overhead and avoiding a full deep clone for URI serialization.
Changes:
- Batches pending JSONL log buffers into capped writes while preserving ordering and rotation behavior.
- Replaces URI deep-cloning with a
JSON.stringifyreplacer that detects marshalled URI components. - Adds tests for batching, flush ordering, and URI serialization behavior.
Show a summary per file
| File | Description |
|---|---|
src/vs/platform/agentHost/common/ahpJsonlLogger.ts |
Implements batched writes and URI-aware JSON serialization. |
src/vs/platform/agentHost/test/common/ahpJsonlLogger.test.ts |
Adds coverage for batching, flush ordering, and URI stringification cases. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Addresses review feedback: the previous `writeCount < messageCount` assertion would pass even with a near-pathological 49-writes-for-50-logs regression. All 50 log() calls are queued synchronously and must land in the very first drain, so assert writeCount === 1. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
connor4312
approved these changes
May 29, 2026
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
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.
Two perf fixes for
AhpJsonlLogger, the per-message JSONL transport logger used by AHP-based agent host connections.Why
CPU traces of the agents window under heavy AHP traffic showed ~23% of wall time in MajorGC (3.3 s of a 14.16 s trace) on the sessions/agents renderer. The hot stack was:
main
writeFileIPC. Each call allocated a freshVSBuffer, marshalled it through the IPC channel proxy, and got a reply buffer back. With sustained rates of 1300 messages/s the GC simply couldn't keep up.500A second cost was
_replaceUris, which deep-cloned every JSON-RPC message beforeJSON.stringifyso it could rewrite URI instances to once per log line.stringsWhat changed
1. Batched writes via natural backpressure.
log()now appends to a_pending: VSBuffer[]and schedules a single drain on the write queue. While awriteFileis in flight, all subsequent log calls accumulate and land in the next drain viaVSBuffer. capped at 1 MiB per write so the concat itself doesn't create new GC pressure. Rotation is still checked per-entry inside the batch.flush()and ordering semantics are preserved via a_drainScheduledinvariant: pending entries always have a drain on the queue, soawait this._queueis sufficient. **Zero added batching only happens when writes are already backed up.latency** concat2. Replaced
_replaceUrisdeep clone with aJSON.stringifyreplacer.URI.prototype.toJSON()already stamps its output with$mid: MarshalledId.Uri. The new replacer detects that marker (guarded byisUriComponentsto avoid false positives on user payloads that happen to carry$mid: 1) and rewrites to the canonical URI string viaURI.revive(...).toString(). The previous comment in the code warned against using a replacer becausetoJSONruns but that's actually what makes this approach work: we use the post-toJSONmarker as our signal.firstTests
Added 7 tests on top of the existing 2:
log()calls produce all 50 lines in order but far fewer writeslog/flush/logcallsUriComponentsfrom a priortoJSON(), URI-shaped objects without$mid(left alone), and$mid: 1on a non-UriComponentspayload (left alone)All 10
AhpJsonlLoggertests pass and the existinglocalAhpJsonlLoggingtests are unaffected.Notes for reviewers
emit
) with no application-level handlers above, which suggests the inbound load is mostly *replies* to outbound requests rather than a stream of notifications. Identifying the producer would benefit from per-method counters inAhpJsonlLogger.log`.(Written by Copilot)