Repository navigation
perf: stop the flow dumper re-dissecting every frame, and options being parsed twice - #427
Merged
Merged
Conversation
…r packet `PCAPIO._append_value` handed the record fields and the packet octets it already held to a `Frame` constructor just to get bytes back. That constructor packs the record and then dissects it again through the whole protocol stack, so every frame the flow tracer wrote was parsed twice. - Pack the 16-octet record header here, with a `struct.Struct` chosen from the byte order of the global header the dumper wrote, and write it followed by `value.packet` verbatim. - Mask each field to 32 bits, because `UInt32Field` truncated rather than rejecting an out-of-range value and `struct.pack` raises instead. - Drop the now-unused `pcapkit.protocols.misc.pcap.frame` import. - Pin the record layout for both byte orders, the truncation, and the fact that appending a frame no longer parses it. The re-dissection was also warning about payloads the dumper only had to copy: writing a 3-octet payload raised `SchemaWarning: packet length < 0: -3`. Measured on `http.pcap` (1117 frames, best of 7, one interpreter per measurement): flow-traced extraction 2319 ms -> 1263 ms against an unchanged 1030 ms untraced baseline, so 82% of the tracing overhead is gone. `test.pcap` 62.1 -> 33.5 ms, `http6.cap` 48.4 -> 25.8 ms. Output is byte-identical: all 30 `tree`/`json` dumps and all 710 flow-trace files over the 15 fixtures with reassembly and tracing enabled, plus 1420 more flow-trace files over both `trace_byteorder` and both `trace_nanosecond` values. Suite 833 passed, 17 skipped; mypy message multiset unchanged at 124 errors.
…to select an option schema `OptionField.unpack` unpacked the base schema in full to read one field from it, threw the result away, rewound, and unpacked the option again with the real schema -- so every option was parsed twice. Measured on `examples/captures/profile.pcapng`: 2274 schema unpacks for 1137 options. - Read the base schema's type field alone, exactly the way `Schema.unpack` reads one field, so `code` keeps the enumeration type that the `eool` comparison and the `OrderedMultiDict` key rely on. - Rewind by the octets actually read rather than by `len(meta)`, which a truncated stream could have over-rewound. - Pin that each option is unpacked exactly once, that options of differing widths decode in sequence, and that the area past the end-of-option-list marker still reaches the following padding field. Safe because the type field is field 0 of every base schema in the package, always a fixed 1- or 2-octet `EnumField`/`OptionEnumField` with no length callback -- checked over all 34 `OptionField` declarations and all 18 distinct base schemas. None of those base schemas overrides `pre_unpack`, and the five that override `post_process` touch only `len`/`length`, never the type field. The discarded pre-parse's writes into `packet` were not load-bearing either: the three option schemas that do not re-declare the base schema's fields (HOPOPT `_SMFDPDOption` and `_QuickStartOption`, IPv4 `_QSOption`) parse identically with and without them, both in isolation and driven through `OptionField`. Measured best of 15 (best of 7 for `http.pcap`), one interpreter per measurement: `profile.pcapng` 43.3 -> 37.6 ms (-13.3%), `test.pcapng` 7.87 -> 7.04 ms (-10.5%), `test.pcap` 28.9 -> 26.3 ms (-8.9%), `many_interfaces.pcapng` 48.1 -> 44.8 ms (-7.0%), `http.pcap` 1038 -> 980 ms (-5.6%). Output is byte-identical: all 771 files of the equivalence run -- 30 `tree`/`json` dumps over the 15 fixtures with reassembly and tracing enabled, 710 flow-trace files, and the recorded warning multisets -- match exactly, with no change even to the warnings. Suite 836 passed, 17 skipped; mypy message multiset unchanged at 124 errors. `len(data)` is deliberately left alone: returning the consumed count from `Schema.unpack` is the clean fix and that file belongs to #420, a `file.tell()` delta is not equivalent for the three wrapper schemas whose `post_process` returns a nested schema, and the provably-equivalent `__buffer__` sum is worth only ~0.5%.
There was a problem hiding this comment.
🟡 Changes recommended
OptionField.unpack now implicitly assumes the type field is the first base-schema field (an undocumented constraint that can break external schemas), so it needs a compatibility fallback or explicit validation/error.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR targets two measured hot spots in PyPCAPKit’s output/tracing and option parsing paths while preserving byte-identical output: (1) PCAP flow dumping no longer rebuilds and re-dissects a Frame just to re-emit bytes already available, and (2) OptionField no longer unpacks the full base schema twice per option just to discover the option type.
Changes:
- Write PCAP per-record headers directly (in the global header’s byte order) and append
value.packetverbatim, avoiding costly re-dissection. - Optimize
OptionField.unpackto read only the option type field for dispatch, eliminating redundant base-schema unpacks. - Add focused unit tests to pin byte order, uint32 truncation behavior, no-reparse dumping, and “unpack exactly once” option parsing behavior.
File summaries
| File | Description |
|---|---|
| tests/dumpkit/test_common_unit.py | Adds regression tests for PCAP record header endianness, uint32 truncation semantics, and ensuring dumping doesn’t reparse/warn on payload bytes. |
| tests/corekit/test_fields_collections.py | Introduces unit tests ensuring options are unpacked once, parsed sequentially, and leftover bytes after EOOL are reported as padding. |
| pcapkit/dumpkit/pcap.py | Replaces Frame reconstruction with direct struct.Struct packing of the 16-byte record header + raw payload write. |
| pcapkit/corekit/fields/collections.py | Changes OptionField.unpack to read only the base schema’s type field before dispatch, avoiding a full base-schema unpack. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 17, 2026
… is not first Reading only the base schema's type field to select an option schema assumes that field sits at the front of the option. Every `OptionField` in this package satisfies that -- 34 declarations over 18 base schemas, all with a fixed 1- or 2-octet enum first -- but `OptionField` is public and `pcapkit.foundation.registry` invites third-party protocols to register their own base schemas, which need not. Measured on a base schema with the type field second: the shortcut read `size` as the type code, hit the end-of-option-list value, and returned `['End']` where the full unpack returns `['Body', 'End']`, reclassifying four octets as padding. No exception -- a silently misparsed packet. - Decide once, in `__init__`, whether the shortcut fits: the type field must be the first field, a `NumberField` (so `Schema.unpack` reads it through its ordinary per-field branch), and of fixed rather than callable width. - Where it does not fit, unpack the base schema in full, exactly as before the shortcut existed. A foreign base schema is accommodated rather than rejected, so a registration that works today cannot become an import-time failure; each test is written to select the slow path rather than raise. - State the constraint in the class docstring, where someone registering a protocol will read it. - Drop a now-redundant `cast`: the `isinstance` check narrows the type field to `NumberField`, whose `unpack` is already typed `int`. Costs nothing measurable: `profile.pcapng` 30.4/30.8 ms before against 30.7/30.9 ms after over two alternating rounds of best-of-15, the direction flipping between them, against a 2.4-2.8 ms spread. All 34 declarations in the package still take the shortcut, asserted by a new test so that a future declaration cannot quietly lose it. Output unchanged: all 771 files of the equivalence run are byte-identical to the branch without this commit, and still differ from `main` only in the duplicate warnings the dumper commit removed. Suite 867 passed, 17 skipped; mypy message multiset unchanged at 128 errors; pylint unchanged.
This was referenced Sep 17, 2026
There was a problem hiding this comment.
🟢 Approval recommended
The changes are correctness-pinned by focused tests (including fallback safety) and the modified code paths preserve documented/previous behavior while removing redundant parsing work.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
JarryShaw
added a commit
that referenced
this pull request
Sep 17, 2026
Brings in #427 (the option-parse shortcut), #428 (dispatch no longer writing to shared registries) and #430 (the generated typed `__init__`). One conflict, in `tests/protocols/transport/test_tcp_udp_unit.py`, where both sides appended test methods to the end of `TCPUDPUnitTests`. Both kept. `pcapkit/corekit/fields/collections.py` auto-merged, which is the file worth checking by hand rather than trusting: #427 rewrote the head of the `OptionField.unpack` loop and this branch rewrote its tail, so the two touch the same function without overlapping. The merged loop reads the type field through #427's shortcut, rewinds by its `consumed`, then subtracts `len(data)`, breaks on the end-of-option-list code, and only then checks that the option moved the stream. `git diff origin/main -- pcapkit/corekit/fields/collections.py` is additions only -- none of #427's work is lost, and the guard is still after the `eool` break, which is where #431's last commit had to move it.
JarryShaw
added
the
perf
2 of 12 tasks
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.
The last two of the three performance findings from #420, both measured and both output-preserving.
The flow dumper was rebuilding a
Frameper packet_append_valuehanded four fields and the packet's own octets to aFrameconstructor, which packed the record and then re-dissected it through the whole protocol stack to hand back bytes it had just been given. It now packs the 16-octet record header itself, with astruct.Structchosen from the byte order of the global header the dumper wrote, and writesvalue.packetverbatim.Independently re-measured against
origin/mainon the traced shape:http.pcap,tcp=True, trace=TrueAll 331 flow files byte-identical,
diff -rqclean. Full suite 863 passed, 17 skipped.One subtlety it had to preserve:
UInt32Fieldtruncated an out-of-range value inpre_process(ats_secof2**32 + 5was written as5) wherestruct.packraises. Without a mask the change would have turned a previously-written record into astruct.error, so the mask is there with a test pinning it — and that test passes on both trees.The only behaviour difference anywhere is fewer duplicate warnings: 1450
DeprecationWarning: 'maxsplit' is passed as positional argumentonhttp.pcapare gone, because they came from the re-dissection. It also used to warn about payloads it merely had to copy — writing a 3-octet payload raisedSchemaWarning: packet length < 0: -3from a dumper. The new test asserts that is gone, and fails onorigin/main.Half of this win was declined, on evidence
The profiling notes suggested also caching the output file handle rather than reopening per frame, citing "330 of 331 output files byte-identical". That figure was misleading, and the counterfactual is not safe: every flow still open when the last reference to the
Extractoris dropped loses all of its records, leaving the file as a bare 24-byte global header.Measured in isolation on
tcp.pcap:pcapkit.extract(...)as a statement) → 4 of 4 flow files truncated to 24 bytesgc.collect()→ still truncatedNo diagnostic is printed, under
-X deveither. So it is silent data loss whose occurrence depends on interpreter teardown order. The "330 of 331" count came fromhttp.pcap's directory alone, in a long-lived process where GC happened to flush the handles — andhttp.pcapis the one fixture where the loss does not show in isolation, despite 109 of its 331 flows never seeing a FIN.It was worth a further −2.1%. Landing it needs a flush/close protocol
TraceFlowdoes not have (submit()never touches the dumpers, and callers are handedIndex.fpoutpaths to read) plus bounded fd use, one per live flow, unbounded today against a macOS default of 256.Every option was parsed twice
collections.pyunpacked the whole base schema to read one field, discarded it, then unpacked again for real — 2274 schema unpacks for 1137 options. It now reads only the type field, the waySchema.unpackreads any field, socodekeeps its enumeration type (load-bearing for theeoolcomparison and theOrderedMultiDictkey).The pre-parse was verified not to be load-bearing rather than assumed: all 34
OptionFielddeclarations over 18 base schemas have the type field at index 0 as a fixed 1- or 2-octet enum field with no length callback; no base schema overridespre_unpack; and all fivepost_processoverrides assign onlylen/length, never the type field.Isolated, best of 15:
profile.pcapng43.3 → 37.6 ms (−13.3%),test.pcapng7.87 → 7.04 (−10.5%),test.pcap28.9 → 26.3 (−8.9%),many_interfaces.pcapng48.1 → 44.8 (−7.0%),http.pcap1038 → 980 (−5.6%). Byte comparison across all 771 files: zero differences, warning multisets included. Its test fails onorigin/mainwith['Base', 'Body', 'Base', 'End'] != ['Body', 'End']— the double parse itself.The three option schemas no fixture reaches — HOPOPT
_SMFDPDOption/_QuickStartOption, IPv4_QSOption— were covered by hand: valid octets packed from the schemas, each wrapper unpacked twice, with and without the keys the discarded pre-parse used to leave inpacket. Identical on all four paths.Found and not fixed
Extractor(format='pcap'|'cap')is dead:PCAPIO.__init__requiresprotocol, which the extractor never supplies. Pre-existing, already pinned attests/integration/test_files_output_naming.py:36. The flow tracer is PCAPIO's only live caller.trace_nanosecond=Trueonly flips the magic number —ts_usecis copied verbatim, so a microsecond capture is written under a nanosecond magic. Measured:ts_usecstays 774 either way. Pre-existing and byte-identical before/after.OptionField.unpackcan spin forever: the loop decrements bylen(data)and a parsed option can measure 0 octets. Reported by the agent on both trees; I have not reproduced it — a trailing HOPOPTPad1parsed fine for me in 20 s — so it is recorded here rather than filed until there is a repro.new_packetinOptionField.unpackis dead code, filled and discarded. Removal is provably neutral, kept out so this commit's measurement stays attributable.