Repository navigation
Conversation
901c95a to
70db5f4
Compare
|
Analysis of PR #22766 at 70db5f49: Test 712 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 4 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (7, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
ron-kuper
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds block-based formatted output to reduce per-byte dynbuf overhead.
Changes:
- Introduces optional block-output callbacks.
- Batches strings, numbers, padding, and dynbuf writes.
- Adds block writers for buffers and
FILEstreams.
File summaries
| File | Description |
|---|---|
lib/mprintf.c |
Implements block output and 512-byte dynbuf staging. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e182739 to
faff82f
Compare
alloc_addbyter() appended one byte at a time through curlx_dyn_addn(), paying its bounds checks, growth logic, memcpy and NUL-termination once per output byte, and formatf() had no way to hand an output function more than one byte at a time. Stage dynbuf output in a 512-byte buffer and append a chunk at a time, and add an optional block-output callback alongside the per-byte stream callback, so literal segments, %s bodies, digit runs and padding are emitted as runs. Each output function gets a memcpy-based variant: addrun(), alloc_addrun(), storerun() and fwrite_wrapper(). The dynbuf size limit is still reported at exactly the byte that exceeds it: alloc_addbyter() and alloc_addrun() check whether what they are about to stage can still fit and, when it cannot, flush and append that byte alone. A staged byte is not committed when it is accepted, so formatf() commits pending output before %n and performs it only if that succeeds. Without that, an allocation failure could leave %n executed where the byte-at-a-time path would have stopped short of it. stream_zrun(), which stops at a NUL, scans with a byte loop rather than memchr(), because the length it is given is the requested precision and is not clamped to the string length. curl_maprintf() of a request line: -73% on x86-64, -67% on a Cortex-A55, -68% on a 250MHz e300c3. Of one header line: -81%, -77%, -71%. Of a 300-byte all-%s body: -83%, -78%, -77%.
7202c6a to
69786e8
Compare
|
On my x86-64 machine the snprintf test runs about 5-6% faster with this PR. |
We were running different benchmarks. The harness that I've been using mostly exercises |
|
yes sorry, I wasn't suggesting you did anything wrong. Since I just added this performance test to https://curl.se/perf/ it was easy to do a test run against this PR. |
Would it be useful to pull in some of the tests from my own private benchmarking tool into |
|
To close the loop on the benchmarking numbers, I added test cases for
Your tests show the least improvement because they exercise an already relatively cheaper code path (no per-byte callback, and most of the format strings don't have long runs of text). The regression on the older e300c3 seems to be because on that platform/toolchain, with very small transfer sizes, the overhead of memcpy is greater than that of a small for loop. IMO it's acceptable. |
I am all for extending the performance tests. I've tried to design them as smaller stand-alone basic components that each test "one thing" so that I can integrate them easily into the https://curl.se/perf performance tests. This setup is still young so I'm sure there are lots of things to improve and polish. The snprintf() test I added might be a little too simple for example. It seems suitable we do such perf test extension work in a separate PR and we leave this to be this specific mprintf improvement only. I intend to merge this, I just figured I wait until after the release tomorrow just to not rock the boat too much this close to shipping. |
|
Thanks! |
Code in
mprintf.cemits formatted output one byte at a time, viaalloc_addbyter()which callscurlx_dyn_addn(). This means each byte written pays the cost for bounds checks, growth logic,memcpyand NUL termination.formatf()has no way to hand an output function more than one byte.This PR adds an optional block-output callback next to the per-byte one. We now stage dynbuf output in a 512-byte buffer and emit literal format segments,
%sbodies, digit runs and padding as runs. Sinks without a block variant keep working. If the callback is NULL the output falls back to the byte-by-byte path.Benchmarking
Harness: https://github.com/ron-kuper/curlbench
curl_maprintf()of a request linecurl_maprintf()of one header linecurl_maprintf()of a 300-byte all-%sbodyFYI an x86-64 desktop shows −73%, −81% and −83% on the same cases, but I've omitted them from the table because on that platform the results differ at times by tens of percent.
Note that
alloc_addbyter()andalloc_addrun()check whether what they are about to stage still fits. When it does not they flush and append that byte alone, so the limit is reported at exactly the byte that exceeds it, as it was before. That matters for%n, whose side effect would otherwise land past the overflow point.Checked against an unpatched build: with a dynbuf limited to 100 bytes and a format emitting 200 bytes before a
%n, both leave the%ntarget untouched.Testing
make -C tests nonflaky-test, built--enable-debug --enable-warnings --disable-unity --with-openssl --with-nghttp2 --with-zlib: 1739 of 1739 OK, including 1398 (curl_msnprintf).scripts/checksrc.plclean, no new compiler warnings, and a second build under clang with-Wunused-macrosand--disable-threaded-resolveris clean too.AI disclosure
I used an AI assistant on this work: for the benchmark harness, and for checking the generated code and the
%nbehaviour described above. The change, the reasoning and the measurements are mine, and I have verified every claim here myself.