Reuse a scratch buffer in light_write_packet to avoid per-packet allocation#2178
Open
MerinoSheep wants to merge 7 commits into
Open
Reuse a scratch buffer in light_write_packet to avoid per-packet allocation#2178MerinoSheep wants to merge 7 commits into
MerinoSheep wants to merge 7 commits into
Conversation
…cation. Keep a reusable buffer on the light_pcapng_t handle for building enhanced packet blocks instead of calloc/free-ing one for every written packet. The buffer grows on demand and is released in light_pcapng_close. Only the PADD32 padding tail is zeroed since the fixed EPB fields and packet data are fully overwritten on each write. Co-authored-by: Cursor <cursoragent@cursor.com>
Write packets of varying sizes through a single writer, mixing small-then-large and large-then-small transitions, then read them back and verify data and comments round-trip exactly. Covers the reusable write buffer added to light_write_packet. Co-authored-by: Cursor <cursoragent@cursor.com>
Move each PCPP patch marker onto its own line, separate from the explanatory text, and rewrite the comments to focus on why the scratch buffer exists and why only its padding tail needs zeroing, rather than restating what the code does. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
hmmm, let me fix tests failures |
The payload layer was held in a unique_ptr declared after (and thus destroyed before) the owning packet, so the packet's destructor later walked its layer chain into freed memory. This only crashed reliably under stricter allocators (MSVC debug heap on Windows CI, ASan), which is why it slipped through locally. Let the packet own the layer instead via addLayer's ownInPacket flag so their lifetimes are tied together. Found by building with -fsanitize=address,undefined, which reproduced the same use-after-free that the Windows CI jobs were hitting. Co-authored-by: Cursor <cursoragent@cursor.com>
"No longer freed here" only makes sense to someone comparing against the pre-patch code. State the ownership invariant on its own terms instead, since that's what a reader without that history needs at this call site. Co-authored-by: Cursor <cursoragent@cursor.com>
Use a PcapFileNamesDef.h path macro like every other read/write test in this file instead of a locally-scoped literal, and drop the std::remove cleanup, which no other test here does (output files are gitignored, and the cleanup was skipped anyway on any assertion failure since PTF_ASSERT_* returns early). Also drops <cstdio> and <memory>, which were only needed by code that no longer exists in this test. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2178 +/- ##
==========================================
+ Coverage 82.78% 82.80% +0.01%
==========================================
Files 331 331
Lines 60050 60097 +47
Branches 12427 12416 -11
==========================================
+ Hits 49713 49762 +49
Misses 9454 9454
+ Partials 883 881 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
light_pcapng_thandle when building enhancedpacket blocks in
light_write_packet, instead ofcalloc/free-ing one for everywritten packet. The buffer grows on demand and is released in
light_pcapng_close.PADD32padding tail is zeroed on reuse, since the fixed EPB header fieldsand the packet payload are fully overwritten on each write.
TestPcapNgFileWriteScratchReuse, which writes packets of varying sizes (mixingbuffer-grow and buffer-reuse transitions), then reads them back and verifies data and
per-packet comments round-trip exactly.
Performance
Measured writing 2,000,000 packets to a tmpfs file, 5 runs each, comparing this branch
against its base with only the patched file reverted:
Gains are largest for small-to-medium packets, where the per-packet allocation
overhead removed by this change is a larger fraction of total write cost.
Test plan
Bin/Pcap++Test -n— all pcapng tests pass, including the new oneper-packet comments
devclang-formatandcppcheckchecked manually against the pre-commit hookinvocations (no diffs / no warnings)
Made with Cursor