Reuse one Utf8TextWriter for custom formatter bodies - #351
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
Benchmark comparisonBaseline:
|
Serialization allocated a Utf8TextWriter per event inside the batch loop on the custom-ITextFormatter path -- 48 bytes each, ~47 KB of garbage per 1000-event batch. SerializationBuffers already caches the equivalent writer for the message buffer; this adds the symmetric one over the body buffer. Safe because Utf8TextWriter holds no state beyond its backing buffer (every Write goes straight through, there is no internal buffering), that buffer is already cleared per event, and SerializationBuffers is sink-owned and used serially -- the same guarantee MessageWriter already relies on. Dropping the `use` costs nothing: Utf8TextWriter does not override Dispose, so a formatter retaining the reference is exactly as protected as before. CustomFormatterSinkBenchmarks, Push(EventCount: 1000): Payload=Simple 116.03 KB -> 69.37 KB (-40.2%) Payload=Exception 7573.80 KB -> 7519.11 KB (-0.7%, exception rendering dominates) Covered by the existing batch test, which drives a custom formatter across four events and would fail if the reused writer carried state between them. Refs #349 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8695645 to
01b132d
Compare
Second half of #349, now that #350 has landed. Diff is a single file.
Change
Serialization.serializeallocated aUtf8TextWriterper event, inside the batch loop, on the custom-ITextFormatterpath:SerializationBuffersalready caches the equivalent writer for the message buffer (MessageWriter); this adds the symmetricBodyTextWriterover the body buffer, and the call site becomestextFormatter.Format(event, buffers.BodyTextWriter).Why it is safe
Utf8TextWriterholds no state beyond its backing buffer — everyWriteencodes straight into the pooled buffer, there is no internal buffering (its own doc comment says so, which is whyWrittenSpanis valid without aFlush).buffers.Body.Clear(), which is the whole reset.SerializationBuffersis sink-owned and used serially — the same guaranteeMessageWriterhas relied on since the V9 rewrite.usecosts nothing:Utf8TextWriterdoes not overrideDispose, so a formatter that squirrelled away the reference is exactly as (un)protected as it is today.Measured
Isolated effect of this change, measured locally against
masterusingCustomFormatterSinkBenchmarks.Push(EventCount: 1000)(added in #350):The Simple saving is 46.66 KB against 46.9 KB predicted (48 bytes × 1000 events). Exception is dominated by exception rendering, so the same absolute saving barely registers.
The CI comparison table reports −54.0% on the Simple row. That is against published 9.0.1, which lacks both this change and the newline trim from #348, so it bundles the two — the extra ~16 KB is #348 shortening each body. −40% is this PR's own contribution.
The control worth checking:
SinkBenchmarksSimple rows are+0.0%at both 1000 and 10000 events, confirming the built-in formatter path is untouched. The+0.8%on the Exception rows is pre-existing master-vs-9.0.1 drift from #340, identical on both row sizes and unrelated to this change.Meanmoved too, but on shared runners it is noise —Allocatedis the signal.Coverage
No new test. The batch test added in #348 (
body: trimming is per entry across a batch and leaves no stale bytes) already drives a custom formatter across four events with differing bodies, long → short, and would fail if the reused writer carried state between them. 136/136 pass on net8.0/net9.0/net10.0.🤖 Generated with Claude Code