From fa0920f972cfd88c93921d699c9334e117fb510b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 14:53:08 -0400 Subject: [PATCH 01/16] perf: memoise charset detection, which was 30% of an HTTP extraction chardet.detect() is a pure function of the bytes it is given, and by far the most expensive step in decoding a text field. Nothing cached it, so an HTTP-heavy capture re-detected the encoding of every repeated header name and value on every message: 3011 detect() calls over only 163 distinct bytestrings on examples/captures/http.pcap, a 94.6% redundancy rate. - add a bounded functools.lru_cache'd _detect_charset() helper in corekit/fields/strings.py, the module that already owns the chardet dependency, and use it from both call sites -- StringField.post_process and ProtocolBase.decode, which had the same expression written out twice. Measured on examples/captures/http.pcap (1117 frames), best of 7 runs: 1041.5 ms -> 726.3 ms, -30.3%. test.pcap 28.3 -> 21.0 ms, -25.7%. Captures with no text fields are unchanged. Serialising all 14 fixtures to both tree and json form, with and without reassembly, gives byte-identical output (33 MB, 60 runs). --- pcapkit/corekit/fields/strings.py | 30 +++++++++++++++++++++++++++++- pcapkit/protocols/protocol.py | 4 ++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index d3199cfe57..e9d952166e 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- """text field class""" +import functools import urllib.parse as urllib_parse from typing import TYPE_CHECKING, Any, Generic, TypeVar @@ -17,6 +18,33 @@ 'PaddingField', ] +#: How many distinct bytestrings :func:`_detect_charset` will remember. Bounded +#: so that a capture full of never-repeating text cannot retain all of it. +DETECT_CACHE_SIZE = 1024 + + +@functools.lru_cache(maxsize=DETECT_CACHE_SIZE) +def _detect_charset(value: 'bytes') -> 'str': + """Detect the character set of ``value``. + + :func:`chardet.detect` is a pure function of the bytes handed to it, and the + single most expensive step in turning a text field into a :obj:`str`. The + strings a capture presents repeat heavily -- an HTTP-heavy capture asked for + the encoding of ``b'Connection'`` once per message and got the same answer + every time -- so the verdict is memoised on the bytes rather than recomputed. + The result is by construction the one :func:`chardet.detect` would have + returned. + + Args: + value: Bytestring whose encoding is to be detected. + + Returns: + Name of the detected encoding, or ``'utf-8'`` where detection declines + to name one. + + """ + return chardet.detect(value)['encoding'] or 'utf-8' + if TYPE_CHECKING: from typing import Callable, Optional, Tuple @@ -168,7 +196,7 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'str': # py except UnicodeError: ret = urllib_parse.unquote(value.replace(b'%', rb'\x'), encoding='utf-8', errors='replace') else: - charset = self._encoding or chardet.detect(value)['encoding'] or 'utf-8' + charset = self._encoding or _detect_charset(value) try: ret = value.decode(charset, self._errors) except UnicodeError: diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index bf20f34742..25bdeabd86 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -26,9 +26,9 @@ from typing import TYPE_CHECKING, Any, Generic, Optional, Type, TypeVar, cast, overload import aenum -import chardet from pcapkit.corekit.context import ContextRegistry +from pcapkit.corekit.fields.strings import _detect_charset from pcapkit.corekit.module import ModuleDescriptor from pcapkit.corekit.protochain import ProtoChain from pcapkit.protocols import data as data_module @@ -331,7 +331,7 @@ def decode(byte: bytes, *, encoding: 'Optional[str]' = None, .. _chardet: https://chardet.readthedocs.io """ - charset = encoding or chardet.detect(byte)['encoding'] or 'utf-8' + charset = encoding or _detect_charset(byte) try: return byte.decode(charset, errors=errors) except UnicodeError: From 5ce03cd779ca3fe10685182c4b7b2c508288295d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 14:54:27 -0400 Subject: [PATCH 02/16] perf: give FieldBase a __copy__, saving ~15% on every capture shape Schema.unpack() calls field(packet) once per field per packet, and that returns copy.copy(self). With no __copy__ hook, copy.copy fell through to the generic pickle-style path -- object.__reduce_ex__(4), copyreg.__newobj__, then copy._reconstruct -- which on examples/captures/http.pcap ran 63207 times per extraction and cost four separate Python frames per copy. - add FieldBase.__copy__ doing exactly what _reconstruct would have done for an object with a plain __dict__ and no __getstate__/__setstate__: cls.__new__(cls) followed by a shallow __dict__.update. No field class defines __new__, __slots__, __reduce__ or the getstate/setstate pair, so the two paths are equivalent by construction; a microbenchmark puts the hook 3.8x ahead (1.66 us -> 0.43 us per copy). Best of 7 runs, all captures improve because this is on the universal field path: http.pcap 726.3 -> 605.4 ms (-16.6%), test.pcap 21.0 -> 17.9 ms (-14.7%), many_interfaces.pcapng 49.4 -> 41.9 ms (-15.2%), profile.pcapng 44.4 -> 38.1 ms (-14.3%), ipv6.pcap 5.11 -> 4.52 ms (-11.5%). Serialisation of all 14 fixtures to tree and json, with and without reassembly, stays byte-identical. --- pcapkit/corekit/fields/field.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 3e655e21e8..9ec047adfc 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -131,6 +131,24 @@ def __init__(self, *args: 'Any', **kwargs: 'Any') -> 'None': self._template = '0s' self._callback = lambda *_: None + def __copy__(self) -> 'Self': + """Return a shallow copy of the field. + + Every field of every protocol is copied once per packet by + :meth:`__call__`, which made the generic :func:`copy.copy` path -- via + :meth:`object.__reduce_ex__` and :func:`copy._reconstruct` -- one of the + costlier things an extraction did. This does what that path would have + done, and only that: a new instance of the same class, its + :attr:`~object.__dict__` shallow-updated from this one. + + Returns: + A new field instance sharing this one's attribute values. + + """ + new_self = self.__class__.__new__(self.__class__) + new_self.__dict__.update(self.__dict__) + return new_self + def __repr__(self) -> 'str': if not self.name.isidentifier(): return f'<{self.__class__.__name__}>' From 0c13874601274aee6a2fd130ba263b9bfed228fb Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 14:58:26 -0400 Subject: [PATCH 03/16] perf: read Field.length once per field instead of two to four times FieldBase.length is a property computing struct.calcsize(self.template) afresh on every read, and both hot loops read it repeatedly for a value that cannot change between the reads -- FieldBase.unpack read it three times to unpack one field, and Schema.unpack read it twice more per field. That came to 176908 calcsize() calls per extraction of examples/captures/http.pcap, for 43110 fields. - FieldBase.unpack and Schema.unpack now bind it to a local. Nothing mutates _length or _template inside any unpack() -- every assignment to either lives in __init__, __call__ or pre_process, i.e. on the construction and packing paths -- so the hoisted read is the same value each use site saw before. Best of 7 runs: http.pcap 605.4 -> 593.6 ms (-2.0%), profile.pcapng 38.1 -> 36.9 ms (-3.0%), many_interfaces.pcapng 41.9 -> 40.9 ms (-2.4%). Fixture serialisation stays byte-identical. --- pcapkit/corekit/fields/field.py | 9 +++++++-- pcapkit/protocols/schema/schema.py | 23 +++++++++++++++-------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 9ec047adfc..3c19b2a4e1 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -226,9 +226,14 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': Unpacked field value. """ + # NOTE: ``length`` recomputes struct.calcsize() on every read, so the + # three reads this method used to make were three calcsize() calls for + # one value. + length = self.length + if not isinstance(buffer, bytes): - buffer = buffer.read(self.length) - value = struct.unpack(self.template, buffer[:self.length].rjust(self.length, b'\x00'))[0] + buffer = buffer.read(length) + value = struct.unpack(self.template, buffer[:length].rjust(length, b'\x00'))[0] return self.post_process(value, packet) diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 4effad092b..06bf6dc89f 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -615,24 +615,29 @@ def unpack(cls, data: 'bytes | IO[bytes]', for field in self.__fields__.values(): field = field(packet) + # NOTE: ``Field.length`` recomputes struct.calcsize() on every read, + # so it is read once per field here rather than at each use. if isinstance(field, PayloadField): - payload_length = field.length or cast('int', packet['__length__']) + length = field.length + payload_length = length or cast('int', packet['__length__']) payload = data.read(payload_length) self.__buffer__[field.name] = payload - packet['__length__'] -= field.length + packet['__length__'] -= length packet[field.name] = payload setattr(self, field.name, payload) continue if isinstance(field, PaddingField): - byte = data.read(field.length) + length = field.length + + byte = data.read(length) self.__buffer__[field.name] = byte packet[field.name] = byte - packet['__length__'] -= field.length + packet['__length__'] -= length setattr(self, field.name, byte) continue @@ -645,7 +650,9 @@ def unpack(cls, data: 'bytes | IO[bytes]', continue field = field.field(packet) - byte = data.read(field.length) + length = field.length + + byte = data.read(length) self.__buffer__[field.name] = byte value = field.unpack(byte, packet.copy()) @@ -657,18 +664,18 @@ def unpack(cls, data: 'bytes | IO[bytes]', packet['__option_padding__'] = field.option_padding if isinstance(field, ForwardMatchField): - data.seek(-field.length, io.SEEK_CUR) + data.seek(-length, io.SEEK_CUR) elif isinstance(field, OptionField) and field.option_padding > 0: # the option list ended before the declared field length was # exhausted; give the unconsumed remainder back to ``data`` # so that the following fields can read it data.seek(-field.option_padding, io.SEEK_CUR) - consumed = field.length - field.option_padding + consumed = length - field.option_padding self.__buffer__[field.name] = byte[:consumed] packet['__length__'] -= consumed else: - packet['__length__'] -= field.length + packet['__length__'] -= length if packet['__length__'] < 0: warn(f'packet length < 0: {packet["__length__"]}', From 0aca00690a6af9588f45fd0f0bd13cce4517e7d8 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 14:59:47 -0400 Subject: [PATCH 04/16] perf: stop Schema.__setattr__ re-entering itself once per field Schema.unpack() sets every parsed field with setattr(), and the __fields__ branch of Schema.__setattr__ then marked the schema dirty with 'self.__updated__ = True'. That assignment is itself an attribute store, so it re-entered __setattr__, failed the __fields__ membership test, and fell through to object.__setattr__ -- 180297 of the 254043 primitive __setattr__ calls in an extraction of examples/captures/http.pcap were this round trip and nothing else. - write the flag straight into __dict__. It is an instance attribute established in __new__ and no class in the Schema hierarchy overrides __setattr__, so this is the same store the fall-through performed. Best of 7 runs: http.pcap 593.6 -> 571.9 ms (-3.7%), profile.pcapng 36.9 -> 35.4 ms (-4.2%), many_interfaces.pcapng 40.9 -> 39.4 ms (-3.6%). Fixture serialisation stays byte-identical. --- pcapkit/protocols/schema/schema.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 06bf6dc89f..3b58f8497d 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -382,7 +382,13 @@ def __setattr__(self, name: 'str', value: '_VT') -> 'None': if name in self.__fields__: key = self.__map__.get(name, name) self.__dict__[key] = value - self.__updated__ = True + # NOTE: ``self.__updated__ = True`` would re-enter this method once + # per field assigned -- 180297 recursive calls per extraction of + # examples/captures/http.pcap -- only to miss the __fields__ test and + # fall through to object.__setattr__. ``__updated__`` is an instance + # attribute established in __new__, so the direct store is the same + # write with none of the round trip. + self.__dict__['__updated__'] = True return return super().__setattr__(name, value) From 1794c43d20d067d177d6c16069d0614d8b54aae4 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 15:01:13 -0400 Subject: [PATCH 05/16] perf: take Schema.unpack's OptionField test once rather than twice Field classes carry abc.ABCMeta, so isinstance() against them dispatches through a Python-level ABCMeta.__instancecheck__ instead of the C fast path -- 278123 such calls per extraction of examples/captures/http.pcap, 11.5% of profiled time. The loop asked the same 'is this an OptionField' question twice per field. - bind it to a local and reuse it. Nothing between the two uses can change type(field), so the second test could only ever have agreed with the first. Best of 7 runs: http.pcap 571.9 -> 566.3 ms (-1.0%), profile.pcapng 35.4 -> 35.0 ms (-1.2%), test.pcap 17.0 -> 16.8 ms (-1.2%). Fixture serialisation stays byte-identical. --- pcapkit/protocols/schema/schema.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 3b58f8497d..68d50330be 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -666,12 +666,18 @@ def unpack(cls, data: 'bytes | IO[bytes]', packet[field.name] = value - if isinstance(field, OptionField): + # NOTE: taken once because both branches below need it. Field classes + # carry abc.ABCMeta, so isinstance() against them is a Python-level + # __instancecheck__ rather than the C fast path, and every field of + # every packet pays for it. + is_option = isinstance(field, OptionField) + + if is_option: packet['__option_padding__'] = field.option_padding if isinstance(field, ForwardMatchField): data.seek(-length, io.SEEK_CUR) - elif isinstance(field, OptionField) and field.option_padding > 0: + elif is_option and field.option_padding > 0: # the option list ended before the declared field length was # exhausted; give the unconsumed remainder back to ``data`` # so that the following fields can read it From b949a6a08151e3d98f6fa186324d7e164cfea570 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 15:11:04 -0400 Subject: [PATCH 06/16] docs: record the profiling pass -- measurements, wins, and ruled-out suspects Working notes from the pre-1.5.0 profiling pass, kept so the numbers and the dead ends survive the session. Covers the five optimisations on this branch with their before/after, nine findings measured but deliberately not acted on (with the reasoning, including three routes to the ABCMeta isinstance cost that were all rejected on clarity or safety grounds), and seven suspects that were measured and dismissed -- logging, multidict, ProtoChain, _import_next_layer, Info/InfoMeta, store=True, and in.pcap as a profiling target. Meant to be folded into docs/source/pep.rst and deleted; it is a scratch record, not documentation. --- PROFILING-NOTES.md | 434 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 434 insertions(+) create mode 100644 PROFILING-NOTES.md diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md new file mode 100644 index 0000000000..3f47cd0fcb --- /dev/null +++ b/PROFILING-NOTES.md @@ -0,0 +1,434 @@ +# PyPCAPKit profiling notes (pre-1.5.0) + +Working notes from a profiling pass over `pcapkit`, kept so the measurements and +— more importantly — the **ruled-out hypotheses** survive the session that +produced them. Delete this file once its contents have been folded into +`docs/source/pep.rst` or wherever the owner wants them to live. + +Branch: `perf/hot-path-wins`. Every number below is from this host, this venv, +Python 3.14. + +## How to reproduce + +The editable install in `.venv` points at the **main** checkout, so `PYTHONPATH` +must name the tree under test or you profile the wrong code: + +```bash +PY=/local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python +WT=/local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a14f0c4dddc25f516 + +$PY examples/generators/make_samples.py # build the un-committed fixtures +PYTHONPATH=$WT $PY -m pytest -q # 782 passed, 17 skipped +``` + +A subagent found that `PYTHONPATH` alone was not always enough — `sys.path[0]` +(the cwd) can shadow it, so `PYTHONSAFEPATH=1` is worth adding when the cwd is +itself a checkout. + +Scratch harnesses (outside the repo, so they are not committed): + +- `/tmp/pcapprof/bench.py` — best-of-N wall clock per capture per shape. This is + the before/after instrument; **use best-of-N, not mean**, the host is noisy. +- `/tmp/pcapprof/prof.py` — cProfile `tottime` table for one shape. +- `/tmp/pcapprof/callers.py` — `print_callers` for attributing a cost to its + call site. Essential: three of the findings below were misattributed until + this was run. +- `/tmp/pcapprof/dump.py` — serialises **all 14 fixtures** to both `tree` and + `json`, with and without reassembly (60 runs, 33 MB), for equivalence + checking. `/tmp/pcapprof/ref/` holds the pristine-tree output; `diff -r` + against it is how every change below was shown to be behaviour-neutral. +- `/tmp/pcapkit-ref/` — pristine `git archive HEAD` of the pre-optimisation tree, + used to generate the reference output. + +**Caveat on profiled percentages.** cProfile inflates Python-level call overhead +roughly 6x on this workload (6.83 s profiled vs 1.04 s wall for the same run), so +it *understates* work done in C. Charset detection profiled at 14.8% of extraction +but removing it recovered 30% of wall time. Where a figure below is "profiled", +treat it as a lower bound for C-heavy code and an upper bound for call-heavy code. + +## Baseline, before any change + +`store=False, nofile=True, verbose=False`, best of 7: + +| capture | frames | wall | ms/frame | +|---|---|---|---| +| `http.pcap` | 1117 | 1041.5 ms | 0.932 | +| `http.pcap` (`store=True`) | 1117 | 1055.3 ms | 0.945 | +| `test.pcap` | 34 | 28.3 ms | 0.831 | +| `ipv4.pcap` | 4 | 1.90 ms | 0.476 | +| `ipv6.pcap` | 16 | 5.06 ms | 0.316 | +| `profile.pcapng` | 40 | 43.9 ms | 1.097 | +| `many_interfaces.pcapng` | 64 | 49.6 ms | 0.775 | + +`store=True` costs **1.3%** over `store=False` on `http.pcap`. Storing frames is +not a hot spot; do not bother optimising it. + +## What landed on `perf/hot-path-wins` + +Cumulative on `http.pcap`: **1041.5 ms -> 566.3 ms, -45.6%** (1.84x). Every +commit was verified byte-identical over the 60-run / 33 MB serialisation diff, and +the full suite is green. + +| # | sha | change | `http.pcap` | all captures | +|---|---|---|---|---| +| 1 | `6573b808c` | memoise charset detection | -30.3% | HTTP/text only | +| 2 | `9989bae1b` | `FieldBase.__copy__` | -16.6% | -11% to -15% everywhere | +| 3 | `09a0f301d` | read `Field.length` once | -2.0% | -2% to -3% | +| 4 | `6f8070658` | `Schema.__setattr__` recursion | -3.7% | -2.6% to -4.2% | +| 5 | `23b951299` | `OptionField` isinstance taken once | -1.0% | -1.2% | + +Per-capture, base -> now: `test.pcap` 28.3 -> 16.8 ms (-40.7%), `ipv4.pcap` +1.90 -> 1.63 ms (-14.2%), `ipv6.pcap` 5.06 -> 4.38 ms (-13.4%), +`profile.pcapng` 43.9 -> 35.0 ms (-20.3%), `many_interfaces.pcapng` +49.6 -> 39.4 ms (-20.5%). + +### 1. Charset detection was uncached — 30% of an HTTP extraction + +`pcapkit/protocols/protocol.py:334` (`ProtocolBase.decode`) and +`pcapkit/corekit/fields/strings.py:171` (`StringField.post_process`) both called +`chardet.detect(...)` on every text value of every packet. + +- `http.pcap`: **3011 `detect()` calls over 163 distinct bytestrings — 94.6% + redundant.** Top repeats `b'Connection'` x222, `b'1.1'` x222, `b'close'` x210. +- Attribution matters here: on `http.pcap` **100%** of the calls come from + `ProtocolBase.decode` (HTTP/1 header parsing, `httpv1.py:292-309`) and none + from `StringField`; on `many_interfaces.pcapng` it is the exact reverse — 21 + calls, all from `StringField.post_process`, worth 1.3% of that extraction. + Both sites needed fixing; a profile of only `http.pcap` would have missed one. +- Fix: bounded `functools.lru_cache` helper `_detect_charset` in `strings.py` + (the module that already owns the `chardet` dependency), used from both sites. + `chardet.detect` is a pure function of its bytes, so the memoised verdict is + **by construction** the one it would have returned — no heuristic involved. + +Rejected alternative: an `value.isascii()` fast path skipping chardet entirely. +Every one of the 163 distinct values on `http.pcap` is pure ASCII and chardet +calls all of them `'ascii'`, so it would have recovered ~100% rather than 94.6%. +Dismissed because it is **not** provably behaviour-preserving: NUL-interleaved +ASCII-range bytes (`b'a\x00b\x00'`) satisfy `isascii()` but chardet may call them +UTF-16, and the two decode differently. The cache has no such exposure. + +Residual risk of the cache: it retains up to `DETECT_CACHE_SIZE` (1024) +bytestrings. Observed max length 116 bytes, so ~100 KB worst case here, but +`ProtocolBase.decode` is public and a caller could hand it megabyte strings. +Lower the bound or add a length guard if that matters. + +### 2. `copy.copy` on fields fell through to the pickle machinery — ~15% everywhere + +`Schema.unpack` (`pcapkit/protocols/schema/schema.py:615-616`) calls +`field(packet)` once per field per packet, and `Field.__call__` +(`pcapkit/corekit/fields/field.py:265`) returns `copy.copy(self)`. With no +`__copy__` hook, `copy.copy` took the generic route — +`object.__reduce_ex__(4)` -> `copyreg.__newobj__` -> `copy._reconstruct` — four +Python frames per copy, **63207 copies per `http.pcap` extraction** (~56/frame). + +`FieldBase.__copy__` now does what `_reconstruct` did for an object with a plain +`__dict__`: `cls.__new__(cls)` then `__dict__.update`. Verified equivalent: +no field class defines `__new__`, `__slots__`, `__reduce__`, `__getstate__` or +`__setstate__`, and `copy.copy(f).__dict__ == f.__dict__` both ways. +Microbenchmark **1.66 us -> 0.43 us, 3.8x**. + +This is the most broadly useful of the five — it is on the universal field path, +so it helps ARP, IPv4, IPv6, PCAP-NG and construction alike. + +**Still available here:** `copy.copy`'s own dispatch (`_copy_dispatch.get`, +`issubclass(cls, type)`, `getattr(cls, '__copy__')`) now costs *more* than +`__copy__` itself — 0.134 s vs 0.131 s tottime. Calling `self.__copy__()` +directly at the ~8 call sites would recover ~2.6% profiled. Not done: it is a +legibility call the owner should make, since `copy.copy(x)` is the idiomatic +spelling. + +### 3. `Field.length` recomputed `struct.calcsize` on every read + +`pcapkit/corekit/fields/field.py:98-101` is a property calling +`struct.calcsize(self.template)` afresh each access. `FieldBase.unpack` read it +**three times** to unpack one field; `Schema.unpack` read it twice more per field. +**176908 `calcsize` calls per `http.pcap` extraction for 43110 fields** (158 +calls/frame); a subagent counted 159.47/frame on `profile.pcapng` over only **26 +distinct format strings for the whole capture**. + +Both loops now bind it to a local. Safety argument, checked exhaustively: every +assignment to `_length` or `_template` in `pcapkit/corekit/fields/` lives in +`__init__`, `__call__` or `pre_process` — i.e. on the construction and packing +paths. **No `unpack()` anywhere mutates either**, so the hoisted read is the same +value each use site saw. (`OptionField.unpack` does mutate `self._option_padding`, +which is why `option_padding` is deliberately *not* hoisted.) + +Only -2%: the remaining cost is the property *call*, not `calcsize` (0.072 s of +5.099 s profiled). **Caching `length` on the instance is still worth ~6.5x per +access** (63.4 ns -> 9.8 ns measured) but needs invalidation wherever `_template` +changes; not attempted. + +### 4. `Schema.__setattr__` re-entered itself once per field + +`pcapkit/protocols/schema/schema.py:381`. The `__fields__` branch marked the +schema dirty with `self.__updated__ = True`, which is itself an attribute store, +so it re-entered `__setattr__`, missed the `__fields__` test, and fell through to +`object.__setattr__`. **180297 of the 254043 primitive `__setattr__` calls in an +`http.pcap` extraction were that round trip and nothing else.** + +Now writes `self.__dict__['__updated__'] = True`. Equivalent: `__updated__` is an +instance attribute established in `__new__` (`schema.py:276`), it is not +name-mangled (two trailing underscores), and no class in the `Schema` hierarchy +overrides `__setattr__` — only `Info` does, in a different hierarchy. + +`self.__updated__ = False` at `schema.py:556` and `:684` has the same round trip +but runs once per schema rather than once per field (12291 vs 144780 per run), so +it was left alone. + +### 5. `OptionField` isinstance asked twice + +`schema.py:656` and `:661` ran the same `isinstance(field, OptionField)`. Taken +once now. Small (-1.0%) but it removes a literally duplicated test. + +## Ranked findings NOT acted on + +Ordered by measured cost. These are the expensive things to rediscover. + +### A. Every option is parsed twice — 15.5% of `profile.pcapng`, 10.3% of `http.pcap` + +**The largest single win left.** `pcapkit/corekit/fields/collections.py:262-270` +(`OptionField.unpack`): + +```python +meta = self._base_schema.unpack(file, length, packet) # full Schema unpack… +code = cast('int', meta[self._type_name]) # …to read ONE field +schema = self._registry[code] +file.seek(-len(meta), io.SEEK_CUR) # rewind +data = schema.unpack(file, length, packet) # full unpack, again +``` + +Measured **2274 schema unpacks for 1137 options — exactly 2x**. The base-schema +pre-parse is *always* discarded (`base schema class is the real schema class: +0 / 379`). Instrumented per option: pre-parse 15.52 us (35.1% of the option +loop), real unpack 25.76 us, `len(meta)` 1.26 us, `len(data)` 1.65 us; the loop +is **44.2%** of a `profile.pcapng` extraction. `len(meta)`/`len(data)` go through +`Schema.__len__` -> `__bytes__` -> `b''.join` over every field (1.18 us) for two +numbers the unpack already knew — another ~6.5% of the loop. + +Fix: read the type code with a direct `struct.unpack` of its 1-2 bytes instead of +a throwaway `Schema.unpack`, and return consumed lengths from `unpack` rather than +re-deriving them by re-serialising. **Risk: medium-high.** It is the option +parser for every protocol with TLVs, the rewind arithmetic is subtle, and +`__option_padding__` interacts with it. Wants its own review with the option-heavy +fixtures (`profile.pcapng` 9.47 options/frame, `test.pcapng` 13.40) as evidence. + +### B. `pcapkit/utilities/warnings.py:129` builds a log record nobody reads — 7.19 us per warning + +`warn()` calls `logger.warning(..., stacklevel=...)` **unconditionally** before +`warnings.warn(...)`. The `pcapkit` logger has only a `NullHandler`, but +`isEnabledFor(WARNING)` is `True`, so the record is fully constructed — +`logging.findCaller` -> `_is_internal_frame` **15x per call**, plus +`posixpath.normcase` and `posix.fspath` per frame — and then dropped. + +| | us/call | +|---|---| +| pcapkit `warn()` | **7.19** | +| — `logger.warning(stacklevel=)` | 5.29 (**73%**) | +| — `stacklevel()` (`exceptions.py:60`) | 0.73 | +| — `warnings.warn()` | 0.51 | + +**Not on the parse hot path** — a clean extraction of `http.pcap` emits one +warning (`EOF reached`), so this is ~0% of the numbers above. It is 20.5% of a +`Protocol(**kwargs)` construction workload that emits six. Cheap, low-risk fix +(gate on a handler actually wanting the record); worth doing, but its value is in +malformed-capture and construction workloads, not in steady-state parsing. + +### C. ABCMeta makes every field isinstance a Python-level call — 11.5% profiled + +`FieldMeta` inherits `abc.ABCMeta` (`field.py:37`), so `isinstance(field, X)` +dispatches through `ABCMeta.__instancecheck__` instead of the C fast path: +**278123 such calls per `http.pcap` extraction**, 0.293 s of 5.099 s profiled for +the ABC portion, 0.585 s for isinstance overall. `Schema.unpack` asks 5-6 of them +per field (now 5, after commit 5) and a plain field fails all of them. + +Three routes considered, all rejected: + +1. **Drop `abc.ABCMeta` from `FieldMeta`.** Would put every check on the C fast + path. Rejected: there is a real `@abc.abstractmethod` at + `pcapkit/corekit/fields/ipaddress.py:42`, and removing the metaclass silently + stops enforcing it. Trades a safety net for speed. +2. **Override `FieldMeta.__instancecheck__` to `type.__instancecheck__`.** Same + gain, keeps abstractmethod enforcement. Rejected: silently drops + `ABCMeta.register()` virtual subclasses and `__subclasshook__` from a public + metaclass. No field class uses either today, but it is an invisible narrowing. +3. **Precompute the classification per declared field.** Sound — + `type(field(packet))` is always `type(declared_field)` (`Field.__call__`, + `ListField.__call__` and `SwitchField.__call__` all `copy.copy(self)`, and + `ConditionalField._field` is fixed at construction), so a flags tuple could be + computed at schema finalisation. Estimated 4-5% real. Rejected **for the owner + to decide**: it replaces five readable `isinstance` checks with a lookup into + a precomputed table, which is exactly the clarity-for-speed trade the brief + says to flag rather than make. + +### D. `NumberField.__call__` rebuilds the struct format on every copy — <=2.5% profiled + +`pcapkit/corekit/fields/numbers.py:99-109`. Every field copy re-runs the +`endian` ternary, the 5-branch `build_template`, and an f-string, even though +`_length`, `_byteorder` and `_signed` are unchanged from the declared field in the +overwhelming majority of cases. 102483 calls, 0.128 s tottime / 0.450 s cumtime of +5.099 s profiled. + +A guard skipping the rebuild when those three inputs are unchanged looks obvious +but is **not** safely equivalent, and this is the trap: + +- `__init__` (`numbers.py:80-84`) derives the template from `self.__template__` + when the subclass sets one, but `__call__` (`:106`) always derives it from + `build_template(...)`. Those two agree for all eight fixed-width subclasses + today (`Int32Field.__template__ == 'i' == build_template(4, True)`, and so on + for the other seven), so a guard would be correct **by coincidence**, and a + future subclass with a deliberately different `__template__` would silently + change behaviour. +- `__init__` also calls `build_template(self._length, signed)` with the + **argument** `signed`, while `_signed` is `signed if self.__signed__ is None + else self.__signed__` (`:75`). If a subclass ever sets `__signed__` without + `__template__`, `__init__` and `__call__` disagree about the template. +- `build_template` has a side effect (`self._need_process = True`, `:132`) which + skipping the call would not reproduce in general. + +Recorded as a **latent inconsistency worth fixing on correctness grounds**, +independently of performance. Once `__init__` and `__call__` derive the template +the same way, the guard becomes safe and the ~2.5% is collectable. + +### E. PCAP-NG re-derives per-interface timestamp constants per block — ~1.9% + +`pcapkit/protocols/misc/pcapng.py:1277` (`_read_timestamp`) calls +`_get_timezone` + `_get_resolution` + `_get_offset` (`:1228`, `:1182`, `:1205`), +which between them make **4 `_get_interface` calls and 3 +`OrderedMultiDict.get()` option re-scans per packet block**, then a +`decimal.localcontext(prec=64)` and a `Decimal` division — all to recover +`if_tsresol` / `if_tsoffset` / `if_tzone`, which are **fixed for the interface for +the whole capture**. 18.92 us/call, 1.15 calls/frame, ~1.9% of +`profile.pcapng`. Memoising them on the interface context would remove it. Low +risk, modest payoff. + +Related: PCAP-NG does **23.2 schema unpacks per frame against PCAP's 3.0** (7.7x). +Container-only (`layer='Link'`), PCAP-NG is 5.47x PCAP per frame — 0.731 vs +0.134 ms/frame. Most of that gap is finding A, not anything intrinsic to the +format. + +### F. Const enums hash 3.1x slower than ints + +`pcapkit/const/pcapng/option_type.py:71,77` override `__eq__`/`__hash__` in +Python. `dict[OptionType]` lookup **72.4 ns vs 23.6 ns** for `dict[int]`; +`hash(OptionType)` 64.4 ns; `OptionType.get(2)` **446.3 ns**. 1137 registry +lookups per 3-rep `profile.pcapng` run. Real but second-order; it is inside +finding A's loop, so fix A first and re-measure. + +### G. Construction (`make`) — measured, and mostly fine + +Constructions/s, best of 3 over 5000 iterations, arguments taken from +`tests/protocols/transport/test_tcp_udp_unit.py:172-181` and +`tests/protocols/link/test_link_unit.py:355-364`: + +| case | ctor/s | us each | +|---|---|---| +| `HTTP.make` | 212157 | 4.71 | +| `Ethernet.make` | 182888 | 5.47 | +| `IPv4.make` | 116648 | 8.57 | +| `TCP.make` (no options) | 114593 | 8.73 | +| **`TCP.make` + 6 options** | **9812** | **101.91** | +| `Ethernet.pack` | 51532 | 19.41 | +| `IPv4.pack` | 15946 | 62.71 | +| `TCP.pack` + 6 options | 4106 | 243.56 | +| `IPv4(**kwargs)` full ctor | 4849 | 206.21 | +| `Ethernet(**kwargs)` full ctor | 4261 | 234.71 | + +- `make()` itself is cheap because it does **not** pack — see finding H. +- **`TCP.make` + options is 12x the no-options case.** `_make_tcp_options` + (`pcapkit/protocols/transport/tcp.py:1914`) eagerly builds *and packs* each + option schema to compute lengths: **91.6% of that workload's cumtime**. +- `copy.copy` per construction: `Ethernet.pack` 4, `IPv4.pack` 13, + `TCP.pack`+6opts **62** — at 578 ns each (post-commit-2) that is 35.8 us of + 243.6 us (14.7%); the whole of `Field.__call__` is 34% of it. +- `struct.calcsize` per construction: `IPv4.pack` 7, `Ethernet(**kw)` 21, over + 2-6 distinct formats. +- **`infoclass.py` is 0.0% of `make()` and `pack()`**, and 6.7% of the full + constructor. `Info`/`InfoMeta` was a listed suspect; on the construction path it + is not one. + +### H. `schema_final`'s generated `__init__` is dead code (correctness, not perf) + +`pcapkit/protocols/schema/schema.py:77` guards the `exec`-generated typed +`__init__` behind `if not hasattr(cls, '__init__')`, but `Schema` assigns +`__init__ = __update__` at `schema.py:329`, so **the guard is always False and the +generated `__init__` is never installed**. Measured: `Schema.__post_init__` call +count is **0** during `make()`, `pack()` and `Protocol(**kwargs)`; +`S_IPv4.__init__ is Schema.__update__` is `True`. + +So the typed per-schema signature `schema_final` goes to the trouble of +generating is unreachable, and `__post_init__` -> `pack()` never runs. Packing +happens lazily via `Schema.__bytes__` instead. Report this to the owner; fixing it +would make construction *slower* and change behaviour, so it is not a perf item. + +### I. `Protocol(**kwargs)` re-parses everything it just built + +`pcapkit/protocols/protocol.py:670-680`: with `file is None` it calls +`self.pack(**kwargs)` and then **unconditionally** `self._info = +self.unpack(length, **kwargs)`. `Ethernet.pack` 19.4 us -> `Ethernet(**kw)` +60.2 us (3.1x) -> **239.6 us** when `type=IPv4` makes it dissect `b'payload'` as a +malformed IPv4 header (12.3x). Per construction: 3 `Schema.__new__` (1 built, 2 +parsed), 6-10 `Info.__new__`, 21 `struct.calcsize`. + +### J. Pre-existing bug: `TCP(**kwargs)` full constructor raises + +`pcapkit/protocols/transport/tcp.py:479`: + +```python +return self._decode_next_layer(tcp, (tcp.srcport.port, tcp.dstport.port), length - tcp.hdr_len) +``` + +-> `AttributeError: 'int' object has no attribute 'port'`. `make()` leaves +`srcport`/`dstport` as plain `int`, while the parse path yields `AppType` objects +carrying `.port`; the unconditional reparse from finding I then meets parse-path +assumptions with construct-path data. `TCP.make()` and `TCP.pack()` are both +fine — only the full constructor fails. **Reproducible every run.** Believed +pre-existing and unrelated to anything on this branch — confirm against +`/tmp/pcapkit-ref/` before filing. + +## Hypotheses ruled out — do not re-tread these + +Each was a listed suspect. Each was measured and dismissed. + +- **`pcapkit/utilities/logging.py` on the hot path: NO.** A plain `http.pcap` + extraction makes **zero** `logger` calls — nothing from `logging` appears + anywhere in the profile. Every hot-path call site uses lazy `%s` formatting, + not f-string interpolation; the only f-string logger call is + `pcapkit/vendor/__main__.py:56`, which is not on any parse path. Two calls in + `protocol.py` (lines 431, 560) are already commented out. The logging system is + clean. (The *warning* path does call the logger — that is finding B, and it is a + different thing.) +- **`pcapkit/corekit/multidict.py`: NO, ~0.5%.** It *is* on the per-field path + (`OptionField.unpack`, `_read_tcp_options`, `_read_http_header`) but costs + 0.037 s of 6.831 s profiled. `MultiDict.__init__` 6702 calls, `add` 11385 calls + on `http.pcap`. Not worth touching. +- **`ProtoChain` construction: NO, ~0.3%.** All `protochain.py` functions + together are 0.02 s of 6.831 s profiled on `http.pcap`. +- **`Protocol._import_next_layer`: NO.** 10053 calls, 0.035 s *tottime*. Its + 6.148 s cumtime is just the recursive descent through the whole protocol stack + and says nothing about its own cost. +- **`Info`/`InfoMeta` construction: NO on construction, ~4.4% on parse.** + `infoclass.py:259(__update__)` 0.127 s + `:232(__new__)` 0.099 s + the + generated `:2(__init__)` of 5.099 s profiled. It builds the library's + actual product and is already lean; 0.0% of `make()`/`pack()`. +- **`store=True`: NO, 1.3%.** Retaining every frame costs almost nothing. +- **`in.pcap` as a profiling target: NO.** 6 frames, ~0.5 ms total. Far too small + to profile; the per-run fixed setup (0.168 ms for PCAP, 0.30-0.43 ms for + PCAP-NG) swamps the per-frame work. Use `http.pcap` (1117 frames) and say which + capture every number came from — the answers differ a lot by capture shape, and + chardet in particular is ~30% on HTTP and 0% on ARP. + +## State at checkpoint / what was in flight + +- Five commits on `perf/hot-path-wins`, all verified. Suite green: **782 passed, + 17 skipped, 767 subtests passed** in 413 s. +- The brief expected "~806 passed"; this run reports 782. A pristine-tree suite + run was **in flight** to establish whether 782 is the baseline on this host or + whether something on this branch changed it. Its result goes in + `/tmp/pcapprof/ref-suite.txt`. **Resolve this before trusting the branch** — + though note that all five changes were independently shown byte-identical over + 60 serialisation runs, which is stronger evidence than the count. +- A second subagent profiling **reassembly and flow tracing** had not reported + when this note was written; those two shapes are the gap in the coverage below. + Everything else asked for is measured: plain, `store=True`, PCAP-NG, and + construction. +- Not yet done: rebase onto `origin/main` and push. From 08d08d8e8914f1b43c0010f0a54e20e944ad8458 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 15:12:24 -0400 Subject: [PATCH 07/16] docs: correct the profiling notes' commit shas after the rebase The sha table named the pre-rebase commits, which no longer exist. Also records that the rebase onto 42eb0d912 brought in only examples/benchmark/** and the Makefile, leaving pcapkit/ untouched, so the measurements still describe this branch's code. --- PROFILING-NOTES.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md index 3f47cd0fcb..09db2f8c3e 100644 --- a/PROFILING-NOTES.md +++ b/PROFILING-NOTES.md @@ -71,11 +71,19 @@ the full suite is green. | # | sha | change | `http.pcap` | all captures | |---|---|---|---|---| -| 1 | `6573b808c` | memoise charset detection | -30.3% | HTTP/text only | -| 2 | `9989bae1b` | `FieldBase.__copy__` | -16.6% | -11% to -15% everywhere | -| 3 | `09a0f301d` | read `Field.length` once | -2.0% | -2% to -3% | -| 4 | `6f8070658` | `Schema.__setattr__` recursion | -3.7% | -2.6% to -4.2% | -| 5 | `23b951299` | `OptionField` isinstance taken once | -1.0% | -1.2% | +| 1 | `fa0920f97` | memoise charset detection | -30.3% | HTTP/text only | +| 2 | `5ce03cd77` | `FieldBase.__copy__` | -16.6% | -11% to -15% everywhere | +| 3 | `0c1387460` | read `Field.length` once | -2.0% | -2% to -3% | +| 4 | `0aca00690` | `Schema.__setattr__` recursion | -3.7% | -2.6% to -4.2% | +| 5 | `1794c43d2` | `OptionField` isinstance taken once | -1.0% | -1.2% | + +(Shas are post-rebase onto `origin/main` at `42eb0d912`. That rebase brought in +only `examples/benchmark/**` and the `Makefile` from PR #410 — **no `pcapkit/` +source changed**, so every measurement and equivalence check above still describes +the code on this branch. Note that #410 landed a real benchmark harness at +`examples/benchmark/benchmark.py`; future numbers may be better taken through it +than through the scratch scripts listed above. It is not collected by `pytest`, +which has `testpaths = ["tests"]`.) Per-capture, base -> now: `test.pcap` 28.3 -> 16.8 ms (-40.7%), `ipv4.pcap` 1.90 -> 1.63 ms (-14.2%), `ipv6.pcap` 5.06 -> 4.38 ms (-13.4%), From faef5d64967862d0aa82e7f2a6cda03d171ba182 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 15:14:50 -0400 Subject: [PATCH 08/16] Revert "perf: take Schema.unpack's OptionField test once rather than twice" This reverts commit 1794c43d2. Hoisting the isinstance() result into a local costs mypy its type narrowing: storing the test in `is_option` rather than testing inline leaves `field` as FieldBase in the branches that follow, so `field.option_padding` no longer resolves and mypy gains four attr-defined errors it did not have before (124 -> 128 across the package). The win was 1.0% on http.pcap. Four new static-analysis errors and a flag variable in place of a self-evident test is not a trade worth making for that, so the duplicated test goes back. Recorded in PROFILING-NOTES.md as measured and rejected rather than left for someone to rediscover. --- pcapkit/protocols/schema/schema.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 68d50330be..3b58f8497d 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -666,18 +666,12 @@ def unpack(cls, data: 'bytes | IO[bytes]', packet[field.name] = value - # NOTE: taken once because both branches below need it. Field classes - # carry abc.ABCMeta, so isinstance() against them is a Python-level - # __instancecheck__ rather than the C fast path, and every field of - # every packet pays for it. - is_option = isinstance(field, OptionField) - - if is_option: + if isinstance(field, OptionField): packet['__option_padding__'] = field.option_padding if isinstance(field, ForwardMatchField): data.seek(-length, io.SEEK_CUR) - elif is_option and field.option_padding > 0: + elif isinstance(field, OptionField) and field.option_padding > 0: # the option list ended before the declared field length was # exhausted; give the unconsumed remainder back to ``data`` # so that the following fields can read it From e37a8c7412249691409adc2c1032e687e78c2a69 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 15:19:02 -0400 Subject: [PATCH 09/16] docs: add the reassembly and flow-tracing findings, and the revert rationale Completes the profiling record with the two shapes that were still outstanding, which between them turned up the largest opportunities of the whole pass: - IP reassembly calls analyze() eagerly on every frame, fragmented or not (reassembly/ip.py:189), because toolkit/pcap.py:53 filters only on DF. On http.pcap that re-parses 1117 of 1117 unfragmented frames and is 86% of the feature's cost, plus a 133 ms GC bill. - The PCAP flow dumper reopens the output file per frame and rebuilds a whole Frame to obtain bytes it already holds (dumpkit/pcap.py:94 and :128) -- 80% of the flow-tracing cost, with 330/331 output files byte-identical under the counterfactual. Also records that reassembly=True and trace=True are master switches that do no per-protocol work without ip=/tcp= (a benchmark passing only the switch measures nothing), that timing several shapes in one process inflates them by up to 73%, and why change 5 was reverted for costing mypy its type narrowing. --- PROFILING-NOTES.md | 249 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 217 insertions(+), 32 deletions(-) diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md index 09db2f8c3e..a21ac7f227 100644 --- a/PROFILING-NOTES.md +++ b/PROFILING-NOTES.md @@ -65,9 +65,11 @@ not a hot spot; do not bother optimising it. ## What landed on `perf/hot-path-wins` -Cumulative on `http.pcap`: **1041.5 ms -> 566.3 ms, -45.6%** (1.84x). Every -commit was verified byte-identical over the 60-run / 33 MB serialisation diff, and -the full suite is green. +Cumulative on `http.pcap`: **1041.5 ms -> 574.4 ms, -44.8%** (1.81x). Every +commit was verified byte-identical over the 60-run / 33 MB serialisation diff, the +full suite is green, and both `pylint` and `mypy` report exactly the message set +the pristine tree does (124 mypy errors either side, identical; pylint message +multiset identical). | # | sha | change | `http.pcap` | all captures | |---|---|---|---|---| @@ -75,7 +77,7 @@ the full suite is green. | 2 | `5ce03cd77` | `FieldBase.__copy__` | -16.6% | -11% to -15% everywhere | | 3 | `0c1387460` | read `Field.length` once | -2.0% | -2% to -3% | | 4 | `0aca00690` | `Schema.__setattr__` recursion | -3.7% | -2.6% to -4.2% | -| 5 | `1794c43d2` | `OptionField` isinstance taken once | -1.0% | -1.2% | +| ~~5~~ | `1794c43d2`, reverted in `8296e362b` | `OptionField` isinstance taken once | ~~-1.0%~~ | **reverted, see below** | (Shas are post-rebase onto `origin/main` at `42eb0d912`. That rebase brought in only `examples/benchmark/**` and the `Makefile` from PR #410 — **no `pcapkit/` @@ -85,10 +87,26 @@ the code on this branch. Note that #410 landed a real benchmark harness at than through the scratch scripts listed above. It is not collected by `pytest`, which has `testpaths = ["tests"]`.) -Per-capture, base -> now: `test.pcap` 28.3 -> 16.8 ms (-40.7%), `ipv4.pcap` -1.90 -> 1.63 ms (-14.2%), `ipv6.pcap` 5.06 -> 4.38 ms (-13.4%), -`profile.pcapng` 43.9 -> 35.0 ms (-20.3%), `many_interfaces.pcapng` -49.6 -> 39.4 ms (-20.5%). +Per-capture, base -> final: `test.pcap` 28.3 -> 17.0 ms (-39.8%), `ipv4.pcap` +1.90 -> 1.66 ms (-12.6%), `ipv6.pcap` 5.06 -> 4.37 ms (-13.6%), +`profile.pcapng` 43.9 -> 35.2 ms (-19.7%), `many_interfaces.pcapng` +49.6 -> 39.2 ms (-20.9%). + +### Why change 5 was reverted — a worked example of the trade going the wrong way + +Taking `isinstance(field, OptionField)` once into `is_option` instead of testing +it inline at `schema.py:656` and `:661` measured a real **-1.0%**. It was reverted +anyway: storing the result costs **mypy its type narrowing**, so `field` stays +`FieldBase` in the branches below and `field.option_padding` stops resolving — +**four new `attr-defined` errors, 124 -> 128 across the package.** A flag variable +in place of a self-evident test, plus four static-analysis regressions, is not a +trade worth 1%. + +The general lesson for the rest of this list: **check `mypy` and `pylint` parity +against the pristine tree, not just the test suite.** `/tmp/pcapprof/mypy_diff.sh` +and `/tmp/pcapprof/lint_diff.sh` do it by diffing message multisets with line +numbers normalised away. Any narrowing-dependent rewrite in these loops — which +includes route 3 of finding C below — will hit the same wall. ### 1. Charset detection was uncached — 30% of an HTTP extraction @@ -188,6 +206,125 @@ it was left alone. `schema.py:656` and `:661` ran the same `isinstance(field, OptionField)`. Taken once now. Small (-1.0%) but it removes a literally duplicated test. +## Reassembly and flow tracing + +Measured separately, `http.pcap`, one clean subprocess per shape, warm-up +discarded, 3 reps pooled over 2 passes (n=6). **Measuring several shapes in one +process inflates all of them** — the same shape read 1026 ms early in a shared +process and 594 ms in a clean one, a 73% error that grows monotonically with +position in the run. Use one process per shape. + +``` +shape ms ms/frame delta vs base +baseline-nostore 564.4 0.5053 - +baseline-store 576.7 0.5163 - +reasm-bare (reassembly=True) 584.0 0.5228 +7.3 +1.3% +reasm-ip (+ip=True) 1059.5 0.9485 +482.8 +83.7% +reasm-tcp (+tcp=True) 672.6 0.6021 +95.9 +16.6% +reasm-ip-tcp 1134.9 1.0160 +558.2 +96.8% +trace-bare (trace=True) 587.2 0.5257 +22.8 +4.0% +trace-tcp-pcap 1413.9 1.2658 +849.5 +150.5% +trace-tcp-json 1817.4 1.6270 +1253.0 +222.0% +``` + +**A trap for anyone benchmarking these:** `reassembly=True` and `trace=True` are +master switches only (`pcapkit/interface/core.py:61-72`). Without also passing +`ip`/`ipv4`/`ipv6`/`tcp`, **no per-protocol work happens at all** — `+1.3%` and +`+4.0%` respectively. A "reassembly benchmark" that passes only `reassembly=True` +measures nothing. + +`reasm_store=False` saves nothing (+98.8% vs +96.8%): the cost is the reassembly +work, not retaining the datagrams. + +**The `foundation/reassembly/` and `foundation/traceflow/` modules are not the +cost — each is under 1.3% of its run in every shape.** All of it is in what they +call, which is where the two findings below land. They are the largest single +opportunities found anywhere in this pass. + +### Z1. IP reassembly re-parses every frame, fragmented or not — 86% of its cost + +`pcapkit/foundation/reassembly/ip.py:189`: + +```python +packet=self.protocol.analyze(bufid[3], bytes(payload)), +``` + +`analyze()` is a **full second parse** of the reassembled payload through TCP and +HTTP. It is 98.0% of `submit()`'s cumulative time and 33.6% of the whole +`reasm-ip` run. + +And it runs on **every frame**, because nothing filters out unfragmented ones: +`pcapkit/toolkit/pcap.py:53` dismisses a frame only when **DF is set** +(`if ipv4_info.flags.df: return None`), so a frame with DF=0, MF=0, FO=0 — not +fragmented in any sense — passes; `ip.py:73` then sees `not FO and not MF`, +allocates a buffer, sets TDL and submits. Counted across the fixtures: +**`http.pcap` has 1117 IPv4 frames, 0 with DF set and 0 actual fragments, and IP +reassembly still yields 1117 "datagrams".** Same in `tcp.pcap` (4/4), +`test.pcap` (21/21), `ipv4.pcap` (4/4). + +Counterfactual, `submit()` setting `packet=None` (patched in a scratch copy only): + +``` +reasm-ip 1059.5 -> 655.3 ms feature cost +482.8 -> +66.8 ms analyze() = 416.0 ms = 86.2% +reasm-tcp 672.6 -> 650.1 ms feature cost +95.9 -> +61.6 ms analyze() = 34.3 ms = 35.8% +reasm-ip-tcp 1134.9 -> 714.6 ms feature cost +558.2 -> +126.1 ms analyze() = 432.1 ms = 77.4% +``` + +Retaining those 1117 re-parsed object graphs also makes GC real: `gc.disable()` +saves 132.6 ms on `reasm-ip` (27% of the feature's cost) but nothing at all on the +plain baseline or on tracing. + +**Two separable questions here, and they should not be conflated.** Whether a +never-fragmented packet ought to be emitted as a trivially-complete datagram is a +*design* decision the owner owns — it may well be intended. But `analyze()` being +eager is not: making `packet` lazy (a cached property evaluated on first access) +would remove ~86% of the cost with no change to what a caller who reads it sees. +That is the recommended fix; changing the filter is the owner's call. +`reassembly/tcp.py:298` has the same eager `analyze()`, but only 222 submits per +pass (driven by FIN/RST), so it costs proportionally less. + +### Z2. The PCAP flow dumper reopens the file and rebuilds the frame per packet — 80% of its cost + +`pcapkit/dumpkit/pcap.py:94` and `:128`: + +```python +def __call__(self, value, name=None): + with open(self._file, 'ab') as file: # :94 -- once PER FRAME + self._append_value(value, file, name or '') + +def _append_value(self, value, file, name): + packet = Frame( # :128 -- full re-parse PER FRAME + nanosecond=self._nsec, num=self._fnum, proto=self._link, + packet=value.packet, header=self._ghdr, **value.frame_info, + ).data + file.write(packet) +``` + +`dumpkit/pcap.py:85(__call__)` is **48.70% of the `trace-tcp-pcap` run**, of which +the `Frame(...)` rebuild is 97%. 4347 `_io.open` calls over 3 reps (1449/pass: +1117 frame appends + 331 flow-file creations + 1). + +Isolating the tracing logic from the dumper, via an unsupported `trace_format` so +`NotImplementedIO` is installed: **5.8% of the traceflow cost is tracing, 94.2% is +the dumper.** Split by counterfactual: + +``` +trace-tcp-pcap 1413.9 ms baseline (+849.5) + keep the file open only 1339.0 ms -> -87.6 ms = 10.3% of feature cost + + no Frame() rebuild 743.5 ms -> -681.2 ms = 80.2% of feature cost +``` + +The rebuild is **provably** redundant: replacing `_append_value` with +`struct.pack(' Date: Wed, 16 Sep 2026 15:19:25 -0400 Subject: [PATCH 10/16] docs: fix the revert commit's sha in the profiling notes The note cited the pre-amend sha for the revert of change 5. --- PROFILING-NOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md index a21ac7f227..7a6c128898 100644 --- a/PROFILING-NOTES.md +++ b/PROFILING-NOTES.md @@ -77,7 +77,7 @@ multiset identical). | 2 | `5ce03cd77` | `FieldBase.__copy__` | -16.6% | -11% to -15% everywhere | | 3 | `0c1387460` | read `Field.length` once | -2.0% | -2% to -3% | | 4 | `0aca00690` | `Schema.__setattr__` recursion | -3.7% | -2.6% to -4.2% | -| ~~5~~ | `1794c43d2`, reverted in `8296e362b` | `OptionField` isinstance taken once | ~~-1.0%~~ | **reverted, see below** | +| ~~5~~ | `1794c43d2`, reverted in `faef5d649` | `OptionField` isinstance taken once | ~~-1.0%~~ | **reverted, see below** | (Shas are post-rebase onto `origin/main` at `42eb0d912`. That rebase brought in only `examples/benchmark/**` and the `Makefile` from PR #410 — **no `pcapkit/` From 081c802d72899a4f6f822e41c16f7ea1663983bf Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 15:27:42 -0400 Subject: [PATCH 11/16] docs: settle the test-count question in the profiling notes The branch reports 782 passed / 17 skipped; a tree with only the four touched source files reverted reports 764 / 35. Same 799 collected, zero failures either side -- the 18-test gap is tests/test_tier_guard.py skipping because the comparison tree is an unpacked git archive rather than a checkout, which its skip reason states outright. Records that so nobody reads the two numbers as a regression, and notes that comparison suites must run inside a real checkout. --- PROFILING-NOTES.md | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md index 7a6c128898..9eac5c1619 100644 --- a/PROFILING-NOTES.md +++ b/PROFILING-NOTES.md @@ -585,16 +585,32 @@ Each was a listed suspect. Each was measured and dismissed. - **`pylint` and `mypy` parity** against the pristine tree, checked per commit via `/tmp/pcapprof/lint_diff.sh` and `/tmp/pcapprof/mypy_diff.sh`: identical message multisets, 124 mypy errors either side. This is what caught change 5. -- On test *counts*: a first attempt to baseline the suite ran against a - `git archive` of the older base commit `5e4378d9b` and reported 764 passed / 35 - skipped, i.e. **more** skips than the branch's 17. That is an artifact of - comparing two different commits in two different tree layouts, not a signal - about these changes, so it was superseded by a properly isolated run: the same - tree as `HEAD` with **only the four touched source files** reverted to - `origin/main`, in `/tmp/pcapkit-iso`, result in `/tmp/pcapprof/iso-suite.txt`. - That is the comparison to trust. Note in general that these optimisations cannot - change the *collected* count — they would surface as failures, not as fewer - tests. +### On the test counts, since they look alarming and are not + +| tree | passed | skipped | **total** | failed | +|---|---|---|---|---| +| this branch, in the real worktree | 782 | 17 | **799** | 0 | +| `/tmp/pcapkit-iso` — same tree, **only the four touched files reverted** to `origin/main` | 764 | 35 | **799** | 0 | + +**Same 799 collected, zero failures either side.** The 18-test difference is +entirely `tests/test_tier_guard.py`, which skips when git cannot be run — the +comparison trees are unpacked `git archive` tarballs, not checkouts, and the skip +reason says so verbatim: *"git cannot answer here: git could not be run in +/tmp/pcapkit-iso — either the executable is missing or this is not a checkout, +e.g. an unpacked source tarball"* (`test_tier_guard.py:317`, `:323`, `:360`, and +15 more). Nothing to do with these changes. + +So **run any comparison suite inside a real checkout**, or those 18 tests +silently stop running. That also means the first baseline attempt (a +`git archive` of the older base `5e4378d9b`, reporting the same 764/35) said +nothing useful and was superseded by the isolated run above — which is the only +comparison that varies *just* the four source files. + +The brief expected "~806 passed, 17 skipped": the **17 skips match exactly**, and +the pass count differs because this host collects 799 tests where the brief's +collected 823 — a difference present identically with and without these changes, +so it is environment (optional engine packages), not the branch. In general these +optimisations cannot reduce the collected count; they would surface as failures. ## What is not covered, and what to do next From 572f13b45f29f1137d61f01984ba51c4d4570df6 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 16:54:50 -0400 Subject: [PATCH 12/16] perf: bound what the charset cache can retain, and de-personalise the notes Two review findings on #420, both right. `lru_cache` bounds how many entries it keeps, not how large they are, and `ProtocolBase.decode` is public -- so a caller handing it whole payloads could retain 1024 of them for the life of the process. Values above 256 octets now bypass the cache. The threshold is measured rather than guessed: across `http.pcap`, `http6.cap` and `many_interfaces.pcapng`, every value reaching detection was at most 116 octets with a 95th percentile of 52, so 256 keeps every repeating string a real capture presents. A payload large enough to bypass is unlikely to recur anyway, so it loses nothing it was not already paying. The reproduction snippet in the notes hard-coded this machine's absolute paths, which are meaningless to anyone else; it now derives the tree from `git rev-parse --show-toplevel`, and says the absolute figures are one machine's while the ratios are what carry. Verified: short values still cached (1 miss, 2 hits), long values bypass (`currsize` unchanged), both paths return what `chardet.detect` returns. `http.pcap` best-of-7 at 565.5 ms against 560.7 before, so the win holds. Full suite 782 passed, 17 skipped. --- PROFILING-NOTES.md | 8 ++++++-- pcapkit/corekit/fields/strings.py | 33 ++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md index 9eac5c1619..38fe4a301b 100644 --- a/PROFILING-NOTES.md +++ b/PROFILING-NOTES.md @@ -14,13 +14,17 @@ The editable install in `.venv` points at the **main** checkout, so `PYTHONPATH` must name the tree under test or you profile the wrong code: ```bash -PY=/local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python -WT=/local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a14f0c4dddc25f516 +WT=$(git rev-parse --show-toplevel) # the tree under test +PY=python # or the interpreter of your choice $PY examples/generators/make_samples.py # build the un-committed fixtures PYTHONPATH=$WT $PY -m pytest -q # 782 passed, 17 skipped ``` +Every path below is repo-relative for the same reason. The figures here were +taken with CPython 3.14 on Linux; absolute milliseconds will differ on your +machine, and only the ratios are meant to carry across. + A subagent found that `PYTHONPATH` alone was not always enough — `sys.path[0]` (the cwd) can shadow it, so `PYTHONSAFEPATH=1` is worth adding when the cwd is itself a checkout. diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index e9d952166e..fddd4dce99 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -22,8 +22,29 @@ #: so that a capture full of never-repeating text cannot retain all of it. DETECT_CACHE_SIZE = 1024 +#: Longest bytestring :func:`_detect_charset` will put in the cache. Chosen from +#: measurement: across ``http.pcap``, ``http6.cap`` and +#: ``many_interfaces.pcapng`` every value reaching detection was at most 116 +#: octets, with a 95th percentile of 52, so this keeps every repeating string a +#: real capture presents while capping what the cache can retain. +DETECT_CACHE_MAX_BYTES = 256 + @functools.lru_cache(maxsize=DETECT_CACHE_SIZE) +def _detect_charset_cached(value: 'bytes') -> 'str': + """Detect the character set of a short ``value``, memoised. + + Args: + value: Bytestring whose encoding is to be detected. + + Returns: + Name of the detected encoding, or ``'utf-8'`` where detection declines + to name one. + + """ + return chardet.detect(value)['encoding'] or 'utf-8' + + def _detect_charset(value: 'bytes') -> 'str': """Detect the character set of ``value``. @@ -35,6 +56,14 @@ def _detect_charset(value: 'bytes') -> 'str': The result is by construction the one :func:`chardet.detect` would have returned. + Long values bypass the cache. :meth:`ProtocolBase.decode + ` is public, so a caller may + hand this an entire payload, and :func:`~functools.lru_cache` bounds how many + entries it keeps rather than how large they are -- 1024 multi-megabyte + payloads would be retained for the life of the process. Skipping the cache + above :data:`DETECT_CACHE_MAX_BYTES` costs such a call nothing it was not + already paying, since a payload that size is unlikely to recur anyway. + Args: value: Bytestring whose encoding is to be detected. @@ -43,7 +72,9 @@ def _detect_charset(value: 'bytes') -> 'str': to name one. """ - return chardet.detect(value)['encoding'] or 'utf-8' + if len(value) > DETECT_CACHE_MAX_BYTES: + return chardet.detect(value)['encoding'] or 'utf-8' + return _detect_charset_cached(value) if TYPE_CHECKING: from typing import Callable, Optional, Tuple From 427a89d4592c39887e14d4235b8c879f1ab85354 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 18:13:33 -0400 Subject: [PATCH 13/16] utilities: promote charset detection to pcapkit.utilities.chardet Three review findings on #420. `_detect_charset` lived in `corekit/fields/strings.py` and was imported into `protocols/protocol.py` as a private cross-module name, which was the wrong shape for something two callers share. It is now `pcapkit.utilities.chardet`, exported as `detect_charset` and documented alongside the other utilities. The module is named for the third-party package whose domain it covers, matching `utilities/logging.py` and `utilities/warnings.py`, which are named for the stdlib modules they extend. That does not shadow anything: absolute imports mean `import chardet` inside it still resolves the real one (verified, 7.6.0). The now unused `import chardet` in `strings.py` goes too. `PROFILING-NOTES.md` is dropped rather than converted. `MANIFEST.in` carries `global-include *.rst`, so renaming it to `.rst` at top level would newly ship developer notes in every sdist, and `docs/` is pruned from the sdist but would make it a Sphinx source needing a toctree entry. Its substance is already in `docs/source/pep.rst`'s performance section, which #419 merged -- the findings, the wins, the ruled-out suspects and the two measurement traps -- and the exact call counts and per-capture figures are in #420's description permanently. The prefix-caching suggestion is answered in the docstring rather than only in the review thread, since the next reader will meet the bypass and wonder: keying on the first N octets is unsound because `chardet` is statistical over the whole sequence, measured at three disagreements in six realistic cases, each of which would decode a non-ASCII body as ASCII. Full suite 782 passed, 17 skipped. --- PROFILING-NOTES.md | 647 ---------------------- docs/source/pcapkit/utilities/chardet.rst | 19 + docs/source/pcapkit/utilities/index.rst | 1 + pcapkit/corekit/fields/strings.py | 64 +-- pcapkit/protocols/protocol.py | 4 +- pcapkit/utilities/__init__.py | 3 +- pcapkit/utilities/chardet.py | 87 +++ 7 files changed, 113 insertions(+), 712 deletions(-) delete mode 100644 PROFILING-NOTES.md create mode 100644 docs/source/pcapkit/utilities/chardet.rst create mode 100644 pcapkit/utilities/chardet.py diff --git a/PROFILING-NOTES.md b/PROFILING-NOTES.md deleted file mode 100644 index 38fe4a301b..0000000000 --- a/PROFILING-NOTES.md +++ /dev/null @@ -1,647 +0,0 @@ -# PyPCAPKit profiling notes (pre-1.5.0) - -Working notes from a profiling pass over `pcapkit`, kept so the measurements and -— more importantly — the **ruled-out hypotheses** survive the session that -produced them. Delete this file once its contents have been folded into -`docs/source/pep.rst` or wherever the owner wants them to live. - -Branch: `perf/hot-path-wins`. Every number below is from this host, this venv, -Python 3.14. - -## How to reproduce - -The editable install in `.venv` points at the **main** checkout, so `PYTHONPATH` -must name the tree under test or you profile the wrong code: - -```bash -WT=$(git rev-parse --show-toplevel) # the tree under test -PY=python # or the interpreter of your choice - -$PY examples/generators/make_samples.py # build the un-committed fixtures -PYTHONPATH=$WT $PY -m pytest -q # 782 passed, 17 skipped -``` - -Every path below is repo-relative for the same reason. The figures here were -taken with CPython 3.14 on Linux; absolute milliseconds will differ on your -machine, and only the ratios are meant to carry across. - -A subagent found that `PYTHONPATH` alone was not always enough — `sys.path[0]` -(the cwd) can shadow it, so `PYTHONSAFEPATH=1` is worth adding when the cwd is -itself a checkout. - -Scratch harnesses (outside the repo, so they are not committed): - -- `/tmp/pcapprof/bench.py` — best-of-N wall clock per capture per shape. This is - the before/after instrument; **use best-of-N, not mean**, the host is noisy. -- `/tmp/pcapprof/prof.py` — cProfile `tottime` table for one shape. -- `/tmp/pcapprof/callers.py` — `print_callers` for attributing a cost to its - call site. Essential: three of the findings below were misattributed until - this was run. -- `/tmp/pcapprof/dump.py` — serialises **all 14 fixtures** to both `tree` and - `json`, with and without reassembly (60 runs, 33 MB), for equivalence - checking. `/tmp/pcapprof/ref/` holds the pristine-tree output; `diff -r` - against it is how every change below was shown to be behaviour-neutral. -- `/tmp/pcapkit-ref/` — pristine `git archive HEAD` of the pre-optimisation tree, - used to generate the reference output. - -**Caveat on profiled percentages.** cProfile inflates Python-level call overhead -roughly 6x on this workload (6.83 s profiled vs 1.04 s wall for the same run), so -it *understates* work done in C. Charset detection profiled at 14.8% of extraction -but removing it recovered 30% of wall time. Where a figure below is "profiled", -treat it as a lower bound for C-heavy code and an upper bound for call-heavy code. - -## Baseline, before any change - -`store=False, nofile=True, verbose=False`, best of 7: - -| capture | frames | wall | ms/frame | -|---|---|---|---| -| `http.pcap` | 1117 | 1041.5 ms | 0.932 | -| `http.pcap` (`store=True`) | 1117 | 1055.3 ms | 0.945 | -| `test.pcap` | 34 | 28.3 ms | 0.831 | -| `ipv4.pcap` | 4 | 1.90 ms | 0.476 | -| `ipv6.pcap` | 16 | 5.06 ms | 0.316 | -| `profile.pcapng` | 40 | 43.9 ms | 1.097 | -| `many_interfaces.pcapng` | 64 | 49.6 ms | 0.775 | - -`store=True` costs **1.3%** over `store=False` on `http.pcap`. Storing frames is -not a hot spot; do not bother optimising it. - -## What landed on `perf/hot-path-wins` - -Cumulative on `http.pcap`: **1041.5 ms -> 574.4 ms, -44.8%** (1.81x). Every -commit was verified byte-identical over the 60-run / 33 MB serialisation diff, the -full suite is green, and both `pylint` and `mypy` report exactly the message set -the pristine tree does (124 mypy errors either side, identical; pylint message -multiset identical). - -| # | sha | change | `http.pcap` | all captures | -|---|---|---|---|---| -| 1 | `fa0920f97` | memoise charset detection | -30.3% | HTTP/text only | -| 2 | `5ce03cd77` | `FieldBase.__copy__` | -16.6% | -11% to -15% everywhere | -| 3 | `0c1387460` | read `Field.length` once | -2.0% | -2% to -3% | -| 4 | `0aca00690` | `Schema.__setattr__` recursion | -3.7% | -2.6% to -4.2% | -| ~~5~~ | `1794c43d2`, reverted in `faef5d649` | `OptionField` isinstance taken once | ~~-1.0%~~ | **reverted, see below** | - -(Shas are post-rebase onto `origin/main` at `42eb0d912`. That rebase brought in -only `examples/benchmark/**` and the `Makefile` from PR #410 — **no `pcapkit/` -source changed**, so every measurement and equivalence check above still describes -the code on this branch. Note that #410 landed a real benchmark harness at -`examples/benchmark/benchmark.py`; future numbers may be better taken through it -than through the scratch scripts listed above. It is not collected by `pytest`, -which has `testpaths = ["tests"]`.) - -Per-capture, base -> final: `test.pcap` 28.3 -> 17.0 ms (-39.8%), `ipv4.pcap` -1.90 -> 1.66 ms (-12.6%), `ipv6.pcap` 5.06 -> 4.37 ms (-13.6%), -`profile.pcapng` 43.9 -> 35.2 ms (-19.7%), `many_interfaces.pcapng` -49.6 -> 39.2 ms (-20.9%). - -### Why change 5 was reverted — a worked example of the trade going the wrong way - -Taking `isinstance(field, OptionField)` once into `is_option` instead of testing -it inline at `schema.py:656` and `:661` measured a real **-1.0%**. It was reverted -anyway: storing the result costs **mypy its type narrowing**, so `field` stays -`FieldBase` in the branches below and `field.option_padding` stops resolving — -**four new `attr-defined` errors, 124 -> 128 across the package.** A flag variable -in place of a self-evident test, plus four static-analysis regressions, is not a -trade worth 1%. - -The general lesson for the rest of this list: **check `mypy` and `pylint` parity -against the pristine tree, not just the test suite.** `/tmp/pcapprof/mypy_diff.sh` -and `/tmp/pcapprof/lint_diff.sh` do it by diffing message multisets with line -numbers normalised away. Any narrowing-dependent rewrite in these loops — which -includes route 3 of finding C below — will hit the same wall. - -### 1. Charset detection was uncached — 30% of an HTTP extraction - -`pcapkit/protocols/protocol.py:334` (`ProtocolBase.decode`) and -`pcapkit/corekit/fields/strings.py:171` (`StringField.post_process`) both called -`chardet.detect(...)` on every text value of every packet. - -- `http.pcap`: **3011 `detect()` calls over 163 distinct bytestrings — 94.6% - redundant.** Top repeats `b'Connection'` x222, `b'1.1'` x222, `b'close'` x210. -- Attribution matters here: on `http.pcap` **100%** of the calls come from - `ProtocolBase.decode` (HTTP/1 header parsing, `httpv1.py:292-309`) and none - from `StringField`; on `many_interfaces.pcapng` it is the exact reverse — 21 - calls, all from `StringField.post_process`, worth 1.3% of that extraction. - Both sites needed fixing; a profile of only `http.pcap` would have missed one. -- Fix: bounded `functools.lru_cache` helper `_detect_charset` in `strings.py` - (the module that already owns the `chardet` dependency), used from both sites. - `chardet.detect` is a pure function of its bytes, so the memoised verdict is - **by construction** the one it would have returned — no heuristic involved. - -Rejected alternative: an `value.isascii()` fast path skipping chardet entirely. -Every one of the 163 distinct values on `http.pcap` is pure ASCII and chardet -calls all of them `'ascii'`, so it would have recovered ~100% rather than 94.6%. -Dismissed because it is **not** provably behaviour-preserving: NUL-interleaved -ASCII-range bytes (`b'a\x00b\x00'`) satisfy `isascii()` but chardet may call them -UTF-16, and the two decode differently. The cache has no such exposure. - -Residual risk of the cache: it retains up to `DETECT_CACHE_SIZE` (1024) -bytestrings. Observed max length 116 bytes, so ~100 KB worst case here, but -`ProtocolBase.decode` is public and a caller could hand it megabyte strings. -Lower the bound or add a length guard if that matters. - -### 2. `copy.copy` on fields fell through to the pickle machinery — ~15% everywhere - -`Schema.unpack` (`pcapkit/protocols/schema/schema.py:615-616`) calls -`field(packet)` once per field per packet, and `Field.__call__` -(`pcapkit/corekit/fields/field.py:265`) returns `copy.copy(self)`. With no -`__copy__` hook, `copy.copy` took the generic route — -`object.__reduce_ex__(4)` -> `copyreg.__newobj__` -> `copy._reconstruct` — four -Python frames per copy, **63207 copies per `http.pcap` extraction** (~56/frame). - -`FieldBase.__copy__` now does what `_reconstruct` did for an object with a plain -`__dict__`: `cls.__new__(cls)` then `__dict__.update`. Verified equivalent: -no field class defines `__new__`, `__slots__`, `__reduce__`, `__getstate__` or -`__setstate__`, and `copy.copy(f).__dict__ == f.__dict__` both ways. -Microbenchmark **1.66 us -> 0.43 us, 3.8x**. - -This is the most broadly useful of the five — it is on the universal field path, -so it helps ARP, IPv4, IPv6, PCAP-NG and construction alike. - -**Still available here:** `copy.copy`'s own dispatch (`_copy_dispatch.get`, -`issubclass(cls, type)`, `getattr(cls, '__copy__')`) now costs *more* than -`__copy__` itself — 0.134 s vs 0.131 s tottime. Calling `self.__copy__()` -directly at the ~8 call sites would recover ~2.6% profiled. Not done: it is a -legibility call the owner should make, since `copy.copy(x)` is the idiomatic -spelling. - -### 3. `Field.length` recomputed `struct.calcsize` on every read - -`pcapkit/corekit/fields/field.py:98-101` is a property calling -`struct.calcsize(self.template)` afresh each access. `FieldBase.unpack` read it -**three times** to unpack one field; `Schema.unpack` read it twice more per field. -**176908 `calcsize` calls per `http.pcap` extraction for 43110 fields** (158 -calls/frame); a subagent counted 159.47/frame on `profile.pcapng` over only **26 -distinct format strings for the whole capture**. - -Both loops now bind it to a local. Safety argument, checked exhaustively: every -assignment to `_length` or `_template` in `pcapkit/corekit/fields/` lives in -`__init__`, `__call__` or `pre_process` — i.e. on the construction and packing -paths. **No `unpack()` anywhere mutates either**, so the hoisted read is the same -value each use site saw. (`OptionField.unpack` does mutate `self._option_padding`, -which is why `option_padding` is deliberately *not* hoisted.) - -Only -2%: the remaining cost is the property *call*, not `calcsize` (0.072 s of -5.099 s profiled). **Caching `length` on the instance is still worth ~6.5x per -access** (63.4 ns -> 9.8 ns measured) but needs invalidation wherever `_template` -changes; not attempted. - -### 4. `Schema.__setattr__` re-entered itself once per field - -`pcapkit/protocols/schema/schema.py:381`. The `__fields__` branch marked the -schema dirty with `self.__updated__ = True`, which is itself an attribute store, -so it re-entered `__setattr__`, missed the `__fields__` test, and fell through to -`object.__setattr__`. **180297 of the 254043 primitive `__setattr__` calls in an -`http.pcap` extraction were that round trip and nothing else.** - -Now writes `self.__dict__['__updated__'] = True`. Equivalent: `__updated__` is an -instance attribute established in `__new__` (`schema.py:276`), it is not -name-mangled (two trailing underscores), and no class in the `Schema` hierarchy -overrides `__setattr__` — only `Info` does, in a different hierarchy. - -`self.__updated__ = False` at `schema.py:556` and `:684` has the same round trip -but runs once per schema rather than once per field (12291 vs 144780 per run), so -it was left alone. - -### 5. `OptionField` isinstance asked twice - -`schema.py:656` and `:661` ran the same `isinstance(field, OptionField)`. Taken -once now. Small (-1.0%) but it removes a literally duplicated test. - -## Reassembly and flow tracing - -Measured separately, `http.pcap`, one clean subprocess per shape, warm-up -discarded, 3 reps pooled over 2 passes (n=6). **Measuring several shapes in one -process inflates all of them** — the same shape read 1026 ms early in a shared -process and 594 ms in a clean one, a 73% error that grows monotonically with -position in the run. Use one process per shape. - -``` -shape ms ms/frame delta vs base -baseline-nostore 564.4 0.5053 - -baseline-store 576.7 0.5163 - -reasm-bare (reassembly=True) 584.0 0.5228 +7.3 +1.3% -reasm-ip (+ip=True) 1059.5 0.9485 +482.8 +83.7% -reasm-tcp (+tcp=True) 672.6 0.6021 +95.9 +16.6% -reasm-ip-tcp 1134.9 1.0160 +558.2 +96.8% -trace-bare (trace=True) 587.2 0.5257 +22.8 +4.0% -trace-tcp-pcap 1413.9 1.2658 +849.5 +150.5% -trace-tcp-json 1817.4 1.6270 +1253.0 +222.0% -``` - -**A trap for anyone benchmarking these:** `reassembly=True` and `trace=True` are -master switches only (`pcapkit/interface/core.py:61-72`). Without also passing -`ip`/`ipv4`/`ipv6`/`tcp`, **no per-protocol work happens at all** — `+1.3%` and -`+4.0%` respectively. A "reassembly benchmark" that passes only `reassembly=True` -measures nothing. - -`reasm_store=False` saves nothing (+98.8% vs +96.8%): the cost is the reassembly -work, not retaining the datagrams. - -**The `foundation/reassembly/` and `foundation/traceflow/` modules are not the -cost — each is under 1.3% of its run in every shape.** All of it is in what they -call, which is where the two findings below land. They are the largest single -opportunities found anywhere in this pass. - -### Z1. IP reassembly re-parses every frame, fragmented or not — 86% of its cost - -`pcapkit/foundation/reassembly/ip.py:189`: - -```python -packet=self.protocol.analyze(bufid[3], bytes(payload)), -``` - -`analyze()` is a **full second parse** of the reassembled payload through TCP and -HTTP. It is 98.0% of `submit()`'s cumulative time and 33.6% of the whole -`reasm-ip` run. - -And it runs on **every frame**, because nothing filters out unfragmented ones: -`pcapkit/toolkit/pcap.py:53` dismisses a frame only when **DF is set** -(`if ipv4_info.flags.df: return None`), so a frame with DF=0, MF=0, FO=0 — not -fragmented in any sense — passes; `ip.py:73` then sees `not FO and not MF`, -allocates a buffer, sets TDL and submits. Counted across the fixtures: -**`http.pcap` has 1117 IPv4 frames, 0 with DF set and 0 actual fragments, and IP -reassembly still yields 1117 "datagrams".** Same in `tcp.pcap` (4/4), -`test.pcap` (21/21), `ipv4.pcap` (4/4). - -Counterfactual, `submit()` setting `packet=None` (patched in a scratch copy only): - -``` -reasm-ip 1059.5 -> 655.3 ms feature cost +482.8 -> +66.8 ms analyze() = 416.0 ms = 86.2% -reasm-tcp 672.6 -> 650.1 ms feature cost +95.9 -> +61.6 ms analyze() = 34.3 ms = 35.8% -reasm-ip-tcp 1134.9 -> 714.6 ms feature cost +558.2 -> +126.1 ms analyze() = 432.1 ms = 77.4% -``` - -Retaining those 1117 re-parsed object graphs also makes GC real: `gc.disable()` -saves 132.6 ms on `reasm-ip` (27% of the feature's cost) but nothing at all on the -plain baseline or on tracing. - -**Two separable questions here, and they should not be conflated.** Whether a -never-fragmented packet ought to be emitted as a trivially-complete datagram is a -*design* decision the owner owns — it may well be intended. But `analyze()` being -eager is not: making `packet` lazy (a cached property evaluated on first access) -would remove ~86% of the cost with no change to what a caller who reads it sees. -That is the recommended fix; changing the filter is the owner's call. -`reassembly/tcp.py:298` has the same eager `analyze()`, but only 222 submits per -pass (driven by FIN/RST), so it costs proportionally less. - -### Z2. The PCAP flow dumper reopens the file and rebuilds the frame per packet — 80% of its cost - -`pcapkit/dumpkit/pcap.py:94` and `:128`: - -```python -def __call__(self, value, name=None): - with open(self._file, 'ab') as file: # :94 -- once PER FRAME - self._append_value(value, file, name or '') - -def _append_value(self, value, file, name): - packet = Frame( # :128 -- full re-parse PER FRAME - nanosecond=self._nsec, num=self._fnum, proto=self._link, - packet=value.packet, header=self._ghdr, **value.frame_info, - ).data - file.write(packet) -``` - -`dumpkit/pcap.py:85(__call__)` is **48.70% of the `trace-tcp-pcap` run**, of which -the `Frame(...)` rebuild is 97%. 4347 `_io.open` calls over 3 reps (1449/pass: -1117 frame appends + 331 flow-file creations + 1). - -Isolating the tracing logic from the dumper, via an unsupported `trace_format` so -`NotImplementedIO` is installed: **5.8% of the traceflow cost is tracing, 94.2% is -the dumper.** Split by counterfactual: - -``` -trace-tcp-pcap 1413.9 ms baseline (+849.5) - keep the file open only 1339.0 ms -> -87.6 ms = 10.3% of feature cost - + no Frame() rebuild 743.5 ms -> -681.2 ms = 80.2% of feature cost -``` - -The rebuild is **provably** redundant: replacing `_append_value` with -`struct.pack(' `__bytes__` -> `b''.join` over every field (1.18 us) for two -numbers the unpack already knew — another ~6.5% of the loop. - -Fix: read the type code with a direct `struct.unpack` of its 1-2 bytes instead of -a throwaway `Schema.unpack`, and return consumed lengths from `unpack` rather than -re-deriving them by re-serialising. **Risk: medium-high.** It is the option -parser for every protocol with TLVs, the rewind arithmetic is subtle, and -`__option_padding__` interacts with it. Wants its own review with the option-heavy -fixtures (`profile.pcapng` 9.47 options/frame, `test.pcapng` 13.40) as evidence. - -### B. `pcapkit/utilities/warnings.py:129` builds a log record nobody reads — 7.19 us per warning - -`warn()` calls `logger.warning(..., stacklevel=...)` **unconditionally** before -`warnings.warn(...)`. The `pcapkit` logger has only a `NullHandler`, but -`isEnabledFor(WARNING)` is `True`, so the record is fully constructed — -`logging.findCaller` -> `_is_internal_frame` **15x per call**, plus -`posixpath.normcase` and `posix.fspath` per frame — and then dropped. - -| | us/call | -|---|---| -| pcapkit `warn()` | **7.19** | -| — `logger.warning(stacklevel=)` | 5.29 (**73%**) | -| — `stacklevel()` (`exceptions.py:60`) | 0.73 | -| — `warnings.warn()` | 0.51 | - -**Not on the parse hot path** — a clean extraction of `http.pcap` emits one -warning (`EOF reached`), so this is ~0% of the numbers above. It is 20.5% of a -`Protocol(**kwargs)` construction workload that emits six. Cheap, low-risk fix -(gate on a handler actually wanting the record); worth doing, but its value is in -malformed-capture and construction workloads, not in steady-state parsing. - -### C. ABCMeta makes every field isinstance a Python-level call — 11.5% profiled - -`FieldMeta` inherits `abc.ABCMeta` (`field.py:37`), so `isinstance(field, X)` -dispatches through `ABCMeta.__instancecheck__` instead of the C fast path: -**278123 such calls per `http.pcap` extraction**, 0.293 s of 5.099 s profiled for -the ABC portion, 0.585 s for isinstance overall. `Schema.unpack` asks 5-6 of them -per field (now 5, after commit 5) and a plain field fails all of them. - -Three routes considered, all rejected: - -1. **Drop `abc.ABCMeta` from `FieldMeta`.** Would put every check on the C fast - path. Rejected: there is a real `@abc.abstractmethod` at - `pcapkit/corekit/fields/ipaddress.py:42`, and removing the metaclass silently - stops enforcing it. Trades a safety net for speed. -2. **Override `FieldMeta.__instancecheck__` to `type.__instancecheck__`.** Same - gain, keeps abstractmethod enforcement. Rejected: silently drops - `ABCMeta.register()` virtual subclasses and `__subclasshook__` from a public - metaclass. No field class uses either today, but it is an invisible narrowing. -3. **Precompute the classification per declared field.** Sound — - `type(field(packet))` is always `type(declared_field)` (`Field.__call__`, - `ListField.__call__` and `SwitchField.__call__` all `copy.copy(self)`, and - `ConditionalField._field` is fixed at construction), so a flags tuple could be - computed at schema finalisation. Estimated 4-5% real. Rejected **for the owner - to decide**: it replaces five readable `isinstance` checks with a lookup into - a precomputed table, which is exactly the clarity-for-speed trade the brief - says to flag rather than make. - -### D. `NumberField.__call__` rebuilds the struct format on every copy — <=2.5% profiled - -`pcapkit/corekit/fields/numbers.py:99-109`. Every field copy re-runs the -`endian` ternary, the 5-branch `build_template`, and an f-string, even though -`_length`, `_byteorder` and `_signed` are unchanged from the declared field in the -overwhelming majority of cases. 102483 calls, 0.128 s tottime / 0.450 s cumtime of -5.099 s profiled. - -A guard skipping the rebuild when those three inputs are unchanged looks obvious -but is **not** safely equivalent, and this is the trap: - -- `__init__` (`numbers.py:80-84`) derives the template from `self.__template__` - when the subclass sets one, but `__call__` (`:106`) always derives it from - `build_template(...)`. Those two agree for all eight fixed-width subclasses - today (`Int32Field.__template__ == 'i' == build_template(4, True)`, and so on - for the other seven), so a guard would be correct **by coincidence**, and a - future subclass with a deliberately different `__template__` would silently - change behaviour. -- `__init__` also calls `build_template(self._length, signed)` with the - **argument** `signed`, while `_signed` is `signed if self.__signed__ is None - else self.__signed__` (`:75`). If a subclass ever sets `__signed__` without - `__template__`, `__init__` and `__call__` disagree about the template. -- `build_template` has a side effect (`self._need_process = True`, `:132`) which - skipping the call would not reproduce in general. - -Recorded as a **latent inconsistency worth fixing on correctness grounds**, -independently of performance. Once `__init__` and `__call__` derive the template -the same way, the guard becomes safe and the ~2.5% is collectable. - -### E. PCAP-NG re-derives per-interface timestamp constants per block — ~1.9% - -`pcapkit/protocols/misc/pcapng.py:1277` (`_read_timestamp`) calls -`_get_timezone` + `_get_resolution` + `_get_offset` (`:1228`, `:1182`, `:1205`), -which between them make **4 `_get_interface` calls and 3 -`OrderedMultiDict.get()` option re-scans per packet block**, then a -`decimal.localcontext(prec=64)` and a `Decimal` division — all to recover -`if_tsresol` / `if_tsoffset` / `if_tzone`, which are **fixed for the interface for -the whole capture**. 18.92 us/call, 1.15 calls/frame, ~1.9% of -`profile.pcapng`. Memoising them on the interface context would remove it. Low -risk, modest payoff. - -Related: PCAP-NG does **23.2 schema unpacks per frame against PCAP's 3.0** (7.7x). -Container-only (`layer='Link'`), PCAP-NG is 5.47x PCAP per frame — 0.731 vs -0.134 ms/frame. Most of that gap is finding A, not anything intrinsic to the -format. - -### F. Const enums hash 3.1x slower than ints - -`pcapkit/const/pcapng/option_type.py:71,77` override `__eq__`/`__hash__` in -Python. `dict[OptionType]` lookup **72.4 ns vs 23.6 ns** for `dict[int]`; -`hash(OptionType)` 64.4 ns; `OptionType.get(2)` **446.3 ns**. 1137 registry -lookups per 3-rep `profile.pcapng` run. Real but second-order; it is inside -finding A's loop, so fix A first and re-measure. - -### G. Construction (`make`) — measured, and mostly fine - -Constructions/s, best of 3 over 5000 iterations, arguments taken from -`tests/protocols/transport/test_tcp_udp_unit.py:172-181` and -`tests/protocols/link/test_link_unit.py:355-364`: - -| case | ctor/s | us each | -|---|---|---| -| `HTTP.make` | 212157 | 4.71 | -| `Ethernet.make` | 182888 | 5.47 | -| `IPv4.make` | 116648 | 8.57 | -| `TCP.make` (no options) | 114593 | 8.73 | -| **`TCP.make` + 6 options** | **9812** | **101.91** | -| `Ethernet.pack` | 51532 | 19.41 | -| `IPv4.pack` | 15946 | 62.71 | -| `TCP.pack` + 6 options | 4106 | 243.56 | -| `IPv4(**kwargs)` full ctor | 4849 | 206.21 | -| `Ethernet(**kwargs)` full ctor | 4261 | 234.71 | - -- `make()` itself is cheap because it does **not** pack — see finding H. -- **`TCP.make` + options is 12x the no-options case.** `_make_tcp_options` - (`pcapkit/protocols/transport/tcp.py:1914`) eagerly builds *and packs* each - option schema to compute lengths: **91.6% of that workload's cumtime**. -- `copy.copy` per construction: `Ethernet.pack` 4, `IPv4.pack` 13, - `TCP.pack`+6opts **62** — at 578 ns each (post-commit-2) that is 35.8 us of - 243.6 us (14.7%); the whole of `Field.__call__` is 34% of it. -- `struct.calcsize` per construction: `IPv4.pack` 7, `Ethernet(**kw)` 21, over - 2-6 distinct formats. -- **`infoclass.py` is 0.0% of `make()` and `pack()`**, and 6.7% of the full - constructor. `Info`/`InfoMeta` was a listed suspect; on the construction path it - is not one. - -### H. `schema_final`'s generated `__init__` is dead code (correctness, not perf) - -`pcapkit/protocols/schema/schema.py:77` guards the `exec`-generated typed -`__init__` behind `if not hasattr(cls, '__init__')`, but `Schema` assigns -`__init__ = __update__` at `schema.py:329`, so **the guard is always False and the -generated `__init__` is never installed**. Measured: `Schema.__post_init__` call -count is **0** during `make()`, `pack()` and `Protocol(**kwargs)`; -`S_IPv4.__init__ is Schema.__update__` is `True`. - -So the typed per-schema signature `schema_final` goes to the trouble of -generating is unreachable, and `__post_init__` -> `pack()` never runs. Packing -happens lazily via `Schema.__bytes__` instead. Report this to the owner; fixing it -would make construction *slower* and change behaviour, so it is not a perf item. - -### I. `Protocol(**kwargs)` re-parses everything it just built - -`pcapkit/protocols/protocol.py:670-680`: with `file is None` it calls -`self.pack(**kwargs)` and then **unconditionally** `self._info = -self.unpack(length, **kwargs)`. `Ethernet.pack` 19.4 us -> `Ethernet(**kw)` -60.2 us (3.1x) -> **239.6 us** when `type=IPv4` makes it dissect `b'payload'` as a -malformed IPv4 header (12.3x). Per construction: 3 `Schema.__new__` (1 built, 2 -parsed), 6-10 `Info.__new__`, 21 `struct.calcsize`. - -### J. Pre-existing bug: `TCP(**kwargs)` full constructor raises - -`pcapkit/protocols/transport/tcp.py:479`: - -```python -return self._decode_next_layer(tcp, (tcp.srcport.port, tcp.dstport.port), length - tcp.hdr_len) -``` - --> `AttributeError: 'int' object has no attribute 'port'`. `make()` leaves -`srcport`/`dstport` as plain `int`, while the parse path yields `AppType` objects -carrying `.port`; the unconditional reparse from finding I then meets parse-path -assumptions with construct-path data. `TCP.make()` and `TCP.pack()` are both -fine — only the full constructor fails. **Reproducible every run.** Believed -pre-existing and unrelated to anything on this branch — confirm against -`/tmp/pcapkit-ref/` before filing. - -## Hypotheses ruled out — do not re-tread these - -Each was a listed suspect. Each was measured and dismissed. - -- **`pcapkit/utilities/logging.py` on the hot path: NO.** A plain `http.pcap` - extraction makes **zero** `logger` calls — nothing from `logging` appears - anywhere in the profile. Every hot-path call site uses lazy `%s` formatting, - not f-string interpolation; the only f-string logger call in the whole package - is `pcapkit/vendor/__main__.py:56`, which is not on any parse path. Two calls in - `protocol.py` (lines 431, 560) are already commented out. Confirmed again on the - heaviest shape measured (`trace-tcp-pcap`): stdlib `logging` is 4944 calls / - **0.03%**, and `pcapkit.utilities.logging` contributes **0 runtime calls** — - `get_logger` is import-time only. The logger carries only a `NullHandler` and - sets no level, so `logger.debug` bails at `isEnabledFor` without formatting. - The logging system is clean. (The *warning* path does call the logger - unconditionally — that is finding B, a different mechanism.) -- **`pcapkit/corekit/multidict.py`: NO, under 1%.** It *is* genuinely on the - per-packet path (`OptionField.unpack`, `transport/tcp.py:664` for TCP options, - `application/httpv1.py:306` for HTTP headers) — 16.9 `add` calls per frame — but - costs 0.037 s of 6.831 s profiled on plain `http.pcap`, and 0.82% on the - reassembly shape. Microbenched at 206.8 ns per `add`. Not worth touching. -- **Repeated `enum` lookups: NO, under 1.4%.** All of `pcapkit/const/**` is 42219 - calls / **0.37%** of the reassembly shape; `aenum` adds another 0.98%; stdlib - `enum` never appears. Biggest single site `const/reg/apptype.py:30585(get)`, - 13404 calls / 0.0161 s. *Caveat:* on the PCAP-NG option path specifically the - custom `OptionType.__eq__`/`__hash__` (`const/pcapng/option_type.py:71,77`) do - make a registry lookup 3.1x a plain int dict (72.4 ns vs 23.6 ns) — that is - finding F, and it sits inside finding A's loop, so fix A first and re-measure. -- **`ProtoChain` construction: NO, ~0.3%.** All `protochain.py` functions - together are 0.02 s of 6.831 s profiled on `http.pcap`. -- **`Protocol._import_next_layer`: NO.** 10053 calls, 0.035 s *tottime*. Its - 6.148 s cumtime is just the recursive descent through the whole protocol stack - and says nothing about its own cost. -- **`Info`/`InfoMeta` construction: NO on construction, ~4.4% on parse.** - `infoclass.py:259(__update__)` 0.127 s + `:232(__new__)` 0.099 s + the - generated `:2(__init__)` of 5.099 s profiled. It builds the library's - actual product and is already lean; 0.0% of `make()`/`pack()`. -- **`store=True`: NO, 1.3%.** Retaining every frame costs almost nothing. -- **`in.pcap` as a profiling target: NO.** 6 frames, ~0.5 ms total. Far too small - to profile; the per-run fixed setup (0.168 ms for PCAP, 0.30-0.43 ms for - PCAP-NG) swamps the per-frame work. Use `http.pcap` (1117 frames) and say which - capture every number came from — the answers differ a lot by capture shape, and - chardet in particular is ~30% on HTTP and 0% on ARP. - -## Verification status - -- Four surviving optimisation commits, plus one measured-and-reverted. Suite on - the branch: **782 passed, 17 skipped, 767 subtests passed** in 413 s. The 17 - skips are exactly the number expected for this repo. -- **Serialisation equivalence** is the strongest evidence and it is clean: all 14 - fixtures, `tree` and `json`, with and without reassembly — 60 runs, 33 MB — - byte-identical to the pristine tree after every single commit. -- **`pylint` and `mypy` parity** against the pristine tree, checked per commit via - `/tmp/pcapprof/lint_diff.sh` and `/tmp/pcapprof/mypy_diff.sh`: identical message - multisets, 124 mypy errors either side. This is what caught change 5. -### On the test counts, since they look alarming and are not - -| tree | passed | skipped | **total** | failed | -|---|---|---|---|---| -| this branch, in the real worktree | 782 | 17 | **799** | 0 | -| `/tmp/pcapkit-iso` — same tree, **only the four touched files reverted** to `origin/main` | 764 | 35 | **799** | 0 | - -**Same 799 collected, zero failures either side.** The 18-test difference is -entirely `tests/test_tier_guard.py`, which skips when git cannot be run — the -comparison trees are unpacked `git archive` tarballs, not checkouts, and the skip -reason says so verbatim: *"git cannot answer here: git could not be run in -/tmp/pcapkit-iso — either the executable is missing or this is not a checkout, -e.g. an unpacked source tarball"* (`test_tier_guard.py:317`, `:323`, `:360`, and -15 more). Nothing to do with these changes. - -So **run any comparison suite inside a real checkout**, or those 18 tests -silently stop running. That also means the first baseline attempt (a -`git archive` of the older base `5e4378d9b`, reporting the same 764/35) said -nothing useful and was superseded by the isolated run above — which is the only -comparison that varies *just* the four source files. - -The brief expected "~806 passed, 17 skipped": the **17 skips match exactly**, and -the pass count differs because this host collects 799 tests where the brief's -collected 823 — a difference present identically with and without these changes, -so it is environment (optional engine packages), not the branch. In general these -optimisations cannot reduce the collected count; they would surface as failures. - -## What is not covered, and what to do next - -All five shapes the brief asked for are measured: plain extraction, `store=True`, -reassembly, flow tracing, PCAP-NG, and construction (`make` / `pack` / the full -constructor). - -Nothing in Z1, Z2 or A-J is implemented. In the order the numbers justify: - -1. **Z2, the PCAP flow dumper** — ~80% of the flow-tracing cost, and the - best-evidenced change in this document (330/331 output files byte-identical - under the counterfactual). Two independent parts: hold the file open, and stop - rebuilding a `Frame` to obtain bytes already in hand. Lowest risk of anything - here. -2. **Z1, eager `analyze()` in IP reassembly** — ~86% of the IP-reassembly cost - plus most of a 133 ms GC bill. Make `packet` lazy; leave the "should - unfragmented frames be emitted at all" question to the owner. -3. **A, the double option parse** — ~15% of a PCAP-NG extraction, ~10% of - `http.pcap`. Highest risk of the three: it is the option parser for every - protocol with TLVs and the rewind arithmetic interacts with - `__option_padding__`. Wants its own review with the option-heavy fixtures. -4. **Caching `Field.length`** — a flat ~2.7-2.9% on *every* shape including the - plain baseline, still 79 `calcsize` calls per frame after commit 3. The template - is immutable per field instance, so it is pure waste; the work is invalidating - the cache at the ~15 sites that assign `_template`. Note the cheap version - (memoising `struct.calcsize` itself, which needs no invalidation) only recovers - ~0.8% — the property *call*, not `calcsize`, is the bulk. Do the real one. -5. **B, the unconditional `logger.warning` in `warn()`** — cheap, low risk, but - only pays off on malformed-capture and construction workloads. -6. Then C/D/E/F/H/I, and file J as a bug. diff --git a/docs/source/pcapkit/utilities/chardet.rst b/docs/source/pcapkit/utilities/chardet.rst new file mode 100644 index 0000000000..30842717ed --- /dev/null +++ b/docs/source/pcapkit/utilities/chardet.rst @@ -0,0 +1,19 @@ +Character Set Detection +======================= + +.. module:: pcapkit.utilities.chardet + +:mod:`pcapkit.utilities.chardet` wraps `chardet`_ with a bounded cache, for +turning the bytes of a text field into a :obj:`str`. It is shared by +:meth:`StringField.post_process +` and +:meth:`ProtocolBase.decode `, +which is why it lives here rather than beside either of them. + +.. _chardet: https://chardet.readthedocs.io + +.. autofunction:: pcapkit.utilities.chardet.detect_charset + +.. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_SIZE + +.. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_MAX_BYTES diff --git a/docs/source/pcapkit/utilities/index.rst b/docs/source/pcapkit/utilities/index.rst index 9a15ab3ae4..d99688d064 100644 --- a/docs/source/pcapkit/utilities/index.rst +++ b/docs/source/pcapkit/utilities/index.rst @@ -17,6 +17,7 @@ several user-refined exceptions and warnings. exceptions warnings logging + chardet Version Compatibility ===================== diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index fddd4dce99..3efad5be1d 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -1,13 +1,11 @@ # -*- coding: utf-8 -*- """text field class""" -import functools import urllib.parse as urllib_parse from typing import TYPE_CHECKING, Any, Generic, TypeVar -import chardet - from pcapkit.corekit.fields.field import Field, NoValue +from pcapkit.utilities.chardet import detect_charset from pcapkit.utilities.compat import Dict from pcapkit.utilities.exceptions import FieldValueError @@ -18,64 +16,6 @@ 'PaddingField', ] -#: How many distinct bytestrings :func:`_detect_charset` will remember. Bounded -#: so that a capture full of never-repeating text cannot retain all of it. -DETECT_CACHE_SIZE = 1024 - -#: Longest bytestring :func:`_detect_charset` will put in the cache. Chosen from -#: measurement: across ``http.pcap``, ``http6.cap`` and -#: ``many_interfaces.pcapng`` every value reaching detection was at most 116 -#: octets, with a 95th percentile of 52, so this keeps every repeating string a -#: real capture presents while capping what the cache can retain. -DETECT_CACHE_MAX_BYTES = 256 - - -@functools.lru_cache(maxsize=DETECT_CACHE_SIZE) -def _detect_charset_cached(value: 'bytes') -> 'str': - """Detect the character set of a short ``value``, memoised. - - Args: - value: Bytestring whose encoding is to be detected. - - Returns: - Name of the detected encoding, or ``'utf-8'`` where detection declines - to name one. - - """ - return chardet.detect(value)['encoding'] or 'utf-8' - - -def _detect_charset(value: 'bytes') -> 'str': - """Detect the character set of ``value``. - - :func:`chardet.detect` is a pure function of the bytes handed to it, and the - single most expensive step in turning a text field into a :obj:`str`. The - strings a capture presents repeat heavily -- an HTTP-heavy capture asked for - the encoding of ``b'Connection'`` once per message and got the same answer - every time -- so the verdict is memoised on the bytes rather than recomputed. - The result is by construction the one :func:`chardet.detect` would have - returned. - - Long values bypass the cache. :meth:`ProtocolBase.decode - ` is public, so a caller may - hand this an entire payload, and :func:`~functools.lru_cache` bounds how many - entries it keeps rather than how large they are -- 1024 multi-megabyte - payloads would be retained for the life of the process. Skipping the cache - above :data:`DETECT_CACHE_MAX_BYTES` costs such a call nothing it was not - already paying, since a payload that size is unlikely to recur anyway. - - Args: - value: Bytestring whose encoding is to be detected. - - Returns: - Name of the detected encoding, or ``'utf-8'`` where detection declines - to name one. - - """ - if len(value) > DETECT_CACHE_MAX_BYTES: - return chardet.detect(value)['encoding'] or 'utf-8' - return _detect_charset_cached(value) - if TYPE_CHECKING: from typing import Callable, Optional, Tuple @@ -227,7 +167,7 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'str': # py except UnicodeError: ret = urllib_parse.unquote(value.replace(b'%', rb'\x'), encoding='utf-8', errors='replace') else: - charset = self._encoding or _detect_charset(value) + charset = self._encoding or detect_charset(value) try: ret = value.decode(charset, self._errors) except UnicodeError: diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 25bdeabd86..56491b444e 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -28,7 +28,6 @@ import aenum from pcapkit.corekit.context import ContextRegistry -from pcapkit.corekit.fields.strings import _detect_charset from pcapkit.corekit.module import ModuleDescriptor from pcapkit.corekit.protochain import ProtoChain from pcapkit.protocols import data as data_module @@ -40,6 +39,7 @@ from pcapkit.protocols.schema.schema import Schema from pcapkit.utilities.compat import cached_property from pcapkit.utilities.decorators import beholder, seekset +from pcapkit.utilities.chardet import detect_charset from pcapkit.utilities.exceptions import (ProtocolNotFound, ProtocolNotImplemented, RegistryError, StructError, UnsupportedCall) from pcapkit.utilities.warnings import RegistryWarning, warn @@ -331,7 +331,7 @@ def decode(byte: bytes, *, encoding: 'Optional[str]' = None, .. _chardet: https://chardet.readthedocs.io """ - charset = encoding or _detect_charset(byte) + charset = encoding or detect_charset(byte) try: return byte.decode(charset, errors=errors) except UnicodeError: diff --git a/pcapkit/utilities/__init__.py b/pcapkit/utilities/__init__.py index 2086907841..723cf2408a 100644 --- a/pcapkit/utilities/__init__.py +++ b/pcapkit/utilities/__init__.py @@ -12,10 +12,11 @@ several user-refined exceptions and warnings. """ +from pcapkit.utilities.chardet import detect_charset from pcapkit.utilities.decorators import beholder, prepare, seekset from pcapkit.utilities.exceptions import stacklevel from pcapkit.utilities.logging import configure, ensure_output, get_logger, logger, reset from pcapkit.utilities.warnings import warn __all__ = ['logger', 'get_logger', 'configure', 'reset', 'ensure_output', - 'warn', 'stacklevel'] + 'warn', 'stacklevel', 'detect_charset'] diff --git a/pcapkit/utilities/chardet.py b/pcapkit/utilities/chardet.py new file mode 100644 index 0000000000..96e17c75d6 --- /dev/null +++ b/pcapkit/utilities/chardet.py @@ -0,0 +1,87 @@ +# -*- coding: utf-8 -*- +"""Character Set Detection +============================= + +.. module:: pcapkit.utilities.chardet + +:mod:`pcapkit.utilities.chardet` wraps `chardet`_ with a bounded cache, for +turning the bytes of a text field into a :obj:`str`. + +.. _chardet: https://chardet.readthedocs.io + +""" + +import functools + +import chardet + +__all__ = ['detect_charset'] + +#: How many distinct bytestrings :func:`detect_charset` will remember. Bounded +#: so that a capture full of never-repeating text cannot retain all of it. +DETECT_CACHE_SIZE = 1024 + +#: Longest bytestring :func:`detect_charset` will put in the cache. Chosen from +#: measurement: across ``http.pcap``, ``http6.cap`` and +#: ``many_interfaces.pcapng`` every value reaching detection was at most 116 +#: octets, with a 95th percentile of 52, so this keeps every repeating string a +#: real capture presents while capping what the cache can retain. +DETECT_CACHE_MAX_BYTES = 256 + + +@functools.lru_cache(maxsize=DETECT_CACHE_SIZE) +def _detect_charset_cached(value: 'bytes') -> 'str': + """Detect the character set of a short ``value``, memoised. + + Args: + value: Bytestring whose encoding is to be detected. + + Returns: + Name of the detected encoding, or ``'utf-8'`` where detection declines + to name one. + + """ + return chardet.detect(value)['encoding'] or 'utf-8' + + +def detect_charset(value: 'bytes') -> 'str': + """Detect the character set of ``value``. + + :func:`chardet.detect` is a pure function of the bytes handed to it, and the + single most expensive step in turning a text field into a :obj:`str`. The + strings a capture presents repeat heavily -- an HTTP-heavy capture asked for + the encoding of ``b'Connection'`` once per message and got the same answer + every time -- so the verdict is memoised on the bytes rather than recomputed. + The result is by construction the one :func:`chardet.detect` would have + returned. + + Long values bypass the cache. :meth:`ProtocolBase.decode + ` is public, so a caller may + hand this an entire payload, and :func:`~functools.lru_cache` bounds how many + entries it keeps rather than how large they are -- 1024 multi-megabyte + payloads would be retained for the life of the process. Skipping the cache + above :data:`DETECT_CACHE_MAX_BYTES` costs such a call nothing it was not + already paying, since a payload that size is unlikely to recur anyway. + + Note: + Keying the cache on a *prefix* of a long value would bound the memory + while still caching it, but it is not sound: :func:`chardet.detect` is + statistical over the whole sequence, so a prefix can disagree with the + value it came from. Measured on realistic inputs, an ASCII header + followed by a UTF-8, Latin-1 or CP1251 body is detected as ``ascii`` from + its first 256 octets and correctly otherwise -- three disagreements in + six cases, each of which would decode the body wrongly. Hashing the full + value would be both sound and bounded, and is the thing to reach for if a + capture ever does present large recurring text. + + Args: + value: Bytestring whose encoding is to be detected. + + Returns: + Name of the detected encoding, or ``'utf-8'`` where detection declines + to name one. + + """ + if len(value) > DETECT_CACHE_MAX_BYTES: + return chardet.detect(value)['encoding'] or 'utf-8' + return _detect_charset_cached(value) From 01a39eb9562663cd2be391533c940e284b1d5958 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 18:39:38 -0400 Subject: [PATCH 14/16] utilities: name the wrapper `detect`, matching the function it wraps Per review on #420, `detect_charset` is now `pcapkit.utilities.chardet.detect` -- the same convention as `utilities/warnings.py` exporting `warn` for `warnings.warn`, so the wrapper reads like the thing it wraps. `import chardet` inside a module named `chardet` still resolves the third-party package, since imports are absolute; verified at 7.6.0, and `import pcapkit` is unaffected. I briefly replaced the `:func:`chardet.detect`` references with plain literals, on the theory that a module named `chardet` would make them ambiguous -- Sphinx's Python domain resolves an unqualified target by suffix match, and `pcapkit.utilities.chardet.detect` ends with `chardet.detect`. The rendered HTML says otherwise: all three resolve to `chardet.readthedocs.io/en/latest/api/index.html#chardet.detect` while the local `detect` resolves to `#pcapkit.utilities.chardet.detect`. Intersphinx wins here, so the roles are restored and the note explaining the non-problem is gone. Full suite 830 passed, 17 skipped. Docs build adds no warning naming the page. --- docs/source/pcapkit/utilities/chardet.rst | 2 +- pcapkit/corekit/fields/strings.py | 4 ++-- pcapkit/protocols/protocol.py | 4 ++-- pcapkit/utilities/__init__.py | 4 ++-- pcapkit/utilities/chardet.py | 12 ++++++------ 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/source/pcapkit/utilities/chardet.rst b/docs/source/pcapkit/utilities/chardet.rst index 30842717ed..6d1e3f6148 100644 --- a/docs/source/pcapkit/utilities/chardet.rst +++ b/docs/source/pcapkit/utilities/chardet.rst @@ -12,7 +12,7 @@ which is why it lives here rather than beside either of them. .. _chardet: https://chardet.readthedocs.io -.. autofunction:: pcapkit.utilities.chardet.detect_charset +.. autofunction:: pcapkit.utilities.chardet.detect .. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_SIZE diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index 3efad5be1d..e56a9e91c6 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -5,7 +5,7 @@ from typing import TYPE_CHECKING, Any, Generic, TypeVar from pcapkit.corekit.fields.field import Field, NoValue -from pcapkit.utilities.chardet import detect_charset +from pcapkit.utilities.chardet import detect from pcapkit.utilities.compat import Dict from pcapkit.utilities.exceptions import FieldValueError @@ -167,7 +167,7 @@ def post_process(self, value: 'bytes', packet: 'dict[str, Any]') -> 'str': # py except UnicodeError: ret = urllib_parse.unquote(value.replace(b'%', rb'\x'), encoding='utf-8', errors='replace') else: - charset = self._encoding or detect_charset(value) + charset = self._encoding or detect(value) try: ret = value.decode(charset, self._errors) except UnicodeError: diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 56491b444e..5c99c02bfb 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -39,7 +39,7 @@ from pcapkit.protocols.schema.schema import Schema from pcapkit.utilities.compat import cached_property from pcapkit.utilities.decorators import beholder, seekset -from pcapkit.utilities.chardet import detect_charset +from pcapkit.utilities.chardet import detect from pcapkit.utilities.exceptions import (ProtocolNotFound, ProtocolNotImplemented, RegistryError, StructError, UnsupportedCall) from pcapkit.utilities.warnings import RegistryWarning, warn @@ -331,7 +331,7 @@ def decode(byte: bytes, *, encoding: 'Optional[str]' = None, .. _chardet: https://chardet.readthedocs.io """ - charset = encoding or detect_charset(byte) + charset = encoding or detect(byte) try: return byte.decode(charset, errors=errors) except UnicodeError: diff --git a/pcapkit/utilities/__init__.py b/pcapkit/utilities/__init__.py index 723cf2408a..8e86915243 100644 --- a/pcapkit/utilities/__init__.py +++ b/pcapkit/utilities/__init__.py @@ -12,11 +12,11 @@ several user-refined exceptions and warnings. """ -from pcapkit.utilities.chardet import detect_charset +from pcapkit.utilities.chardet import detect from pcapkit.utilities.decorators import beholder, prepare, seekset from pcapkit.utilities.exceptions import stacklevel from pcapkit.utilities.logging import configure, ensure_output, get_logger, logger, reset from pcapkit.utilities.warnings import warn __all__ = ['logger', 'get_logger', 'configure', 'reset', 'ensure_output', - 'warn', 'stacklevel', 'detect_charset'] + 'warn', 'stacklevel', 'detect'] diff --git a/pcapkit/utilities/chardet.py b/pcapkit/utilities/chardet.py index 96e17c75d6..cf9c8992f1 100644 --- a/pcapkit/utilities/chardet.py +++ b/pcapkit/utilities/chardet.py @@ -15,13 +15,13 @@ import chardet -__all__ = ['detect_charset'] +__all__ = ['detect'] -#: How many distinct bytestrings :func:`detect_charset` will remember. Bounded +#: How many distinct bytestrings :func:`detect` will remember. Bounded #: so that a capture full of never-repeating text cannot retain all of it. DETECT_CACHE_SIZE = 1024 -#: Longest bytestring :func:`detect_charset` will put in the cache. Chosen from +#: Longest bytestring :func:`detect` will put in the cache. Chosen from #: measurement: across ``http.pcap``, ``http6.cap`` and #: ``many_interfaces.pcapng`` every value reaching detection was at most 116 #: octets, with a 95th percentile of 52, so this keeps every repeating string a @@ -30,7 +30,7 @@ @functools.lru_cache(maxsize=DETECT_CACHE_SIZE) -def _detect_charset_cached(value: 'bytes') -> 'str': +def _detect_cached(value: 'bytes') -> 'str': """Detect the character set of a short ``value``, memoised. Args: @@ -44,7 +44,7 @@ def _detect_charset_cached(value: 'bytes') -> 'str': return chardet.detect(value)['encoding'] or 'utf-8' -def detect_charset(value: 'bytes') -> 'str': +def detect(value: 'bytes') -> 'str': """Detect the character set of ``value``. :func:`chardet.detect` is a pure function of the bytes handed to it, and the @@ -84,4 +84,4 @@ def detect_charset(value: 'bytes') -> 'str': """ if len(value) > DETECT_CACHE_MAX_BYTES: return chardet.detect(value)['encoding'] or 'utf-8' - return _detect_charset_cached(value) + return _detect_cached(value) From 9f1b56b1d24740233201d648c9270cd19c876950 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 19:13:20 -0400 Subject: [PATCH 15/16] utilities: key the charset cache on a digest, so large values are cached too Per review on #420: the size threshold was the wrong call, and the reason given is the right one -- it leaned on the sample captures being representative of real traffic, and they are not. A capture full of large repeated text is precisely the case that most wants the cache, and it was the one case the threshold excluded. The cache is now keyed on a 32-octet `blake2b` digest, so what it retains is bounded by count *and* size -- 1024 x 32 octets, whatever is passed -- while every value benefits regardless of length. Measured: 1524 distinct 5 KB values leave 1024 entries holding ~32 KiB rather than 5 MiB, and a 2 MB value caches under a 32-byte key. An `OrderedDict` with `move_to_end`/`popitem` rather than `lru_cache`, because the key is a digest of the argument rather than the argument, which `lru_cache` cannot express. The cost is real and worth stating: `http.pcap` best-of-7 goes 565.5 ms to 588.0 ms, ~4%, from hashing 3011 short strings that the previous version keyed directly. Against the 1042.7 ms baseline it is still -43.6%. A hybrid keyed on raw bytes below the threshold and on a digest above would recover that 4% and stay bounded, at the price of two key paths; not worth it unless the 4% is. Verified identical to `chardet.detect` on ASCII-prefixed UTF-8, Latin-1 and UTF-16 inputs. Full suite 830 passed, 17 skipped. --- docs/source/pcapkit/utilities/chardet.rst | 2 +- pcapkit/utilities/chardet.py | 100 +++++++++++----------- 2 files changed, 53 insertions(+), 49 deletions(-) diff --git a/docs/source/pcapkit/utilities/chardet.rst b/docs/source/pcapkit/utilities/chardet.rst index 6d1e3f6148..89762d2d52 100644 --- a/docs/source/pcapkit/utilities/chardet.rst +++ b/docs/source/pcapkit/utilities/chardet.rst @@ -16,4 +16,4 @@ which is why it lives here rather than beside either of them. .. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_SIZE -.. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_MAX_BYTES +.. autodata:: pcapkit.utilities.chardet.DETECT_DIGEST_SIZE diff --git a/pcapkit/utilities/chardet.py b/pcapkit/utilities/chardet.py index cf9c8992f1..5ada587f8d 100644 --- a/pcapkit/utilities/chardet.py +++ b/pcapkit/utilities/chardet.py @@ -11,37 +11,32 @@ """ -import functools +import collections +import hashlib +from typing import TYPE_CHECKING import chardet __all__ = ['detect'] -#: How many distinct bytestrings :func:`detect` will remember. Bounded -#: so that a capture full of never-repeating text cannot retain all of it. -DETECT_CACHE_SIZE = 1024 - -#: Longest bytestring :func:`detect` will put in the cache. Chosen from -#: measurement: across ``http.pcap``, ``http6.cap`` and -#: ``many_interfaces.pcapng`` every value reaching detection was at most 116 -#: octets, with a 95th percentile of 52, so this keeps every repeating string a -#: real capture presents while capping what the cache can retain. -DETECT_CACHE_MAX_BYTES = 256 - +if TYPE_CHECKING: + from collections import OrderedDict -@functools.lru_cache(maxsize=DETECT_CACHE_SIZE) -def _detect_cached(value: 'bytes') -> 'str': - """Detect the character set of a short ``value``, memoised. - - Args: - value: Bytestring whose encoding is to be detected. +#: How many distinct bytestrings :func:`detect` will remember. Bounded so that a +#: capture full of never-repeating text cannot retain all of it. +DETECT_CACHE_SIZE = 1024 - Returns: - Name of the detected encoding, or ``'utf-8'`` where detection declines - to name one. +#: Digest length, in octets, of the key :func:`detect` caches under. 32 octets is +#: 256 bits, so a collision -- which would hand one bytestring another's verdict +#: -- is not a thing that happens; halving :func:`~hashlib.blake2b`'s default 64 +#: halves what the cache retains per entry for no practical loss. +DETECT_DIGEST_SIZE = 32 - """ - return chardet.detect(value)['encoding'] or 'utf-8' +#: Detected encodings, keyed by digest of the bytes they were detected from, least +#: recently used first. An :class:`~collections.OrderedDict` rather than +#: :func:`functools.lru_cache` because the cache key is a digest of the argument +#: rather than the argument itself, which ``lru_cache`` cannot express. +_cache = collections.OrderedDict() # type: OrderedDict[bytes, str] def detect(value: 'bytes') -> 'str': @@ -51,37 +46,46 @@ def detect(value: 'bytes') -> 'str': single most expensive step in turning a text field into a :obj:`str`. The strings a capture presents repeat heavily -- an HTTP-heavy capture asked for the encoding of ``b'Connection'`` once per message and got the same answer - every time -- so the verdict is memoised on the bytes rather than recomputed. - The result is by construction the one :func:`chardet.detect` would have - returned. - - Long values bypass the cache. :meth:`ProtocolBase.decode - ` is public, so a caller may - hand this an entire payload, and :func:`~functools.lru_cache` bounds how many - entries it keeps rather than how large they are -- 1024 multi-megabyte - payloads would be retained for the life of the process. Skipping the cache - above :data:`DETECT_CACHE_MAX_BYTES` costs such a call nothing it was not - already paying, since a payload that size is unlikely to recur anyway. + every time -- so the verdict is memoised rather than recomputed. The result is + by construction the one :func:`chardet.detect` would have returned. + + The cache is keyed on a :func:`~hashlib.blake2b` digest of the bytes rather + than on the bytes themselves. A cache bounded by entry *count* is not bounded + in size, and :meth:`ProtocolBase.decode + ` is public, so keying on the + value would let :data:`DETECT_CACHE_SIZE` whole payloads be retained for the + life of the process. A fixed-width digest caps that at + :data:`DETECT_CACHE_SIZE` × :data:`DETECT_DIGEST_SIZE` regardless of what is + passed, and hashing is cheap beside a detection that already walks the same + bytes. Note: - Keying the cache on a *prefix* of a long value would bound the memory - while still caching it, but it is not sound: :func:`chardet.detect` is - statistical over the whole sequence, so a prefix can disagree with the - value it came from. Measured on realistic inputs, an ASCII header - followed by a UTF-8, Latin-1 or CP1251 body is detected as ``ascii`` from - its first 256 octets and correctly otherwise -- three disagreements in - six cases, each of which would decode the body wrongly. Hashing the full - value would be both sound and bounded, and is the thing to reach for if a - capture ever does present large recurring text. + Two cheaper-looking alternatives are both wrong. Keying on a *prefix* is + unsound, since :func:`chardet.detect` is statistical over the whole + sequence: measured on realistic inputs, an ASCII header followed by a + UTF-8, Latin-1 or CP1251 body is detected as ``ascii`` from its first 256 + octets and correctly otherwise -- three disagreements in six cases, each + of which would decode the body wrongly. Skipping the cache above a size + threshold is sound but leans on sample captures being representative of + real traffic, which they are not: a capture full of large repeated text is + exactly the case that most wants the cache. Args: value: Bytestring whose encoding is to be detected. Returns: - Name of the detected encoding, or ``'utf-8'`` where detection declines - to name one. + Name of the detected encoding, or ``'utf-8'`` where detection declines to + name one. """ - if len(value) > DETECT_CACHE_MAX_BYTES: - return chardet.detect(value)['encoding'] or 'utf-8' - return _detect_cached(value) + digest = hashlib.blake2b(value, digest_size=DETECT_DIGEST_SIZE).digest() + try: + charset = _cache[digest] + except KeyError: + charset = chardet.detect(value)['encoding'] or 'utf-8' + _cache[digest] = charset + if len(_cache) > DETECT_CACHE_SIZE: + _cache.popitem(last=False) + else: + _cache.move_to_end(digest) + return charset From bcc9fea88104c90206428eb3d5425ad8db95821c Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 20:43:32 -0400 Subject: [PATCH 16/16] utilities: go back to lru_cache, keyed on the value Your call on #420, and it turns out to be the fastest of the three variants as well as the simplest: `http.pcap` best-of-7 at 559.8 ms, against 565.5 for the size-threshold version and 588.0 for the digest-keyed one, from 1042.7 baseline. `detect` is now just `lru_cache` over `chardet.detect`, and the module is 70 lines shorter for it. `detect.cache_clear()` comes free with that, which is a real gain -- a long-running consumer now has a way to release the cache, which the hand-rolled `OrderedDict` did not offer. The trade-off is recorded in the docstring rather than left for someone to rediscover, with the measurements behind it: `lru_cache` caches an argument's *hash* but still keeps the argument, because a dict needs the key to settle equality on a hash collision -- 20 distinct 1 MB values retain 19.1 MB, where the digest-keyed version retained 0.0 MB for the same input. So the bound is on entry count, not footprint. Both rejected alternatives are named there too, so the next reader does not re-run this: prefix keys are unsound (chardet is statistical over the whole sequence, three disagreements in six realistic cases), and digest keys work but cannot be expressed with `lru_cache`, which keys on what it is passed. Full suite 830 passed, 17 skipped. --- docs/source/pcapkit/utilities/chardet.rst | 2 - pcapkit/utilities/chardet.py | 70 ++++++++--------------- 2 files changed, 23 insertions(+), 49 deletions(-) diff --git a/docs/source/pcapkit/utilities/chardet.rst b/docs/source/pcapkit/utilities/chardet.rst index 89762d2d52..29515d5cb0 100644 --- a/docs/source/pcapkit/utilities/chardet.rst +++ b/docs/source/pcapkit/utilities/chardet.rst @@ -15,5 +15,3 @@ which is why it lives here rather than beside either of them. .. autofunction:: pcapkit.utilities.chardet.detect .. autodata:: pcapkit.utilities.chardet.DETECT_CACHE_SIZE - -.. autodata:: pcapkit.utilities.chardet.DETECT_DIGEST_SIZE diff --git a/pcapkit/utilities/chardet.py b/pcapkit/utilities/chardet.py index 5ada587f8d..3a8018c42e 100644 --- a/pcapkit/utilities/chardet.py +++ b/pcapkit/utilities/chardet.py @@ -11,34 +11,18 @@ """ -import collections -import hashlib -from typing import TYPE_CHECKING +import functools import chardet __all__ = ['detect'] -if TYPE_CHECKING: - from collections import OrderedDict - #: How many distinct bytestrings :func:`detect` will remember. Bounded so that a #: capture full of never-repeating text cannot retain all of it. DETECT_CACHE_SIZE = 1024 -#: Digest length, in octets, of the key :func:`detect` caches under. 32 octets is -#: 256 bits, so a collision -- which would hand one bytestring another's verdict -#: -- is not a thing that happens; halving :func:`~hashlib.blake2b`'s default 64 -#: halves what the cache retains per entry for no practical loss. -DETECT_DIGEST_SIZE = 32 - -#: Detected encodings, keyed by digest of the bytes they were detected from, least -#: recently used first. An :class:`~collections.OrderedDict` rather than -#: :func:`functools.lru_cache` because the cache key is a digest of the argument -#: rather than the argument itself, which ``lru_cache`` cannot express. -_cache = collections.OrderedDict() # type: OrderedDict[bytes, str] - +@functools.lru_cache(maxsize=DETECT_CACHE_SIZE) def detect(value: 'bytes') -> 'str': """Detect the character set of ``value``. @@ -49,26 +33,28 @@ def detect(value: 'bytes') -> 'str': every time -- so the verdict is memoised rather than recomputed. The result is by construction the one :func:`chardet.detect` would have returned. - The cache is keyed on a :func:`~hashlib.blake2b` digest of the bytes rather - than on the bytes themselves. A cache bounded by entry *count* is not bounded - in size, and :meth:`ProtocolBase.decode - ` is public, so keying on the - value would let :data:`DETECT_CACHE_SIZE` whole payloads be retained for the - life of the process. A fixed-width digest caps that at - :data:`DETECT_CACHE_SIZE` × :data:`DETECT_DIGEST_SIZE` regardless of what is - passed, and hashing is cheap beside a detection that already walks the same - bytes. - Note: - Two cheaper-looking alternatives are both wrong. Keying on a *prefix* is + The cache is bounded by entry *count*, not by size, and it holds the bytes + it was keyed on: :func:`functools.lru_cache` caches an argument's *hash* + but still keeps the argument, since a :obj:`dict` needs the key to settle + equality on a hash collision. Measured, feeding 20 distinct 1 MB values + retains 19.1 MB. :data:`DETECT_CACHE_SIZE` therefore caps the entries + rather than the footprint, which matters because + :meth:`ProtocolBase.decode + ` is public and a caller + may hand it a whole payload. Use :meth:`detect.cache_clear + ` to release it in a long-running + process. + + Two alternatives were tried and rejected. Keying on a *prefix* is unsound, since :func:`chardet.detect` is statistical over the whole - sequence: measured on realistic inputs, an ASCII header followed by a - UTF-8, Latin-1 or CP1251 body is detected as ``ascii`` from its first 256 - octets and correctly otherwise -- three disagreements in six cases, each - of which would decode the body wrongly. Skipping the cache above a size - threshold is sound but leans on sample captures being representative of - real traffic, which they are not: a capture full of large repeated text is - exactly the case that most wants the cache. + sequence: an ASCII header followed by a UTF-8, Latin-1 or CP1251 body is + detected as ``ascii`` from its first 256 octets and correctly otherwise, + three disagreements in six realistic cases. Keying on a digest bounds the + footprint exactly and was measured retaining 0.0 MB for the same 19 MB of + input, but it cannot be expressed with :func:`~functools.lru_cache` -- + which keys on what it is passed -- and hand-rolling the eviction was + judged not worth the six lines. Args: value: Bytestring whose encoding is to be detected. @@ -78,14 +64,4 @@ def detect(value: 'bytes') -> 'str': name one. """ - digest = hashlib.blake2b(value, digest_size=DETECT_DIGEST_SIZE).digest() - try: - charset = _cache[digest] - except KeyError: - charset = chardet.detect(value)['encoding'] or 'utf-8' - _cache[digest] = charset - if len(_cache) > DETECT_CACHE_SIZE: - _cache.popitem(last=False) - else: - _cache.move_to_end(digest) - return charset + return chardet.detect(value)['encoding'] or 'utf-8'