From 920dc5c72f0728574aef7be7f4f08821338cb3eb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 21:20:52 +0000 Subject: [PATCH] Close the decode-throughput regression against dpkt (#147) Re-measuring every held claim after #107/#124 closed surfaced a regression none of the five prior tiers had been watching for: decode throughput against dpkt had reversed from 1.16x faster (right after Tier 1) to ~11% slower, without anyone profiling why. cProfile against the corpus decode loop found two causes -- neither one of the three tiers originally suspected: - bytes_to_ipv6 (added by #99 for Pyodide portability, landing after all three originally-suspected tiers) formatted its eight address words one f-string at a time through a generator, ~18% of total corpus decode time. Reformatted to one %-format call plus a split, byte-identical output (verified against glibc's inet_ntop via the existing hypothesis test), 2.1x faster in isolation. - decode_frame()'s Packet(*layers, ...) construction paid a redundant isinstance(layer, Protocol) check per layer through Protocol's ABC __instancecheck__ machinery, ~7-8% of decode_frame()'s own time -- every element in that list is already guaranteed to be a Protocol instance by decode_frame()'s own loop. Packet gained a private _from_decoded() fast-construction path (object.__new__ plus direct attribute assignment, same shortcut _base.py already uses for Ethernet/ARP/IPv4), used only by decode_frame(). The public Packet(...) constructor is untouched. #87's registry dispatch and #91's structured diagnostics were both ruled out directly rather than re-guessed: every next_protocol() override still hits its documented single-dict.get fast path, and every raise site #91 touched only populates diagnostic fields inside a raise, never on the happy path. Also fixed a benchmark methodology bug this investigation surfaced: scripts/benchmark.py never called decode_frame(), the documented public chain-walking API #88 shipped -- it kept a hand-rolled copy of the pre-#88 loop, so every decode-throughput figure this project has published described code real callers never ran. decode_netprotocols() and _netprotocols_chain() now call decode_frame() directly; the now-dead walk() is deleted. benchmarks/baseline.json, five tiers stale at a v1.3.0-era figure, is refreshed to match. docs/CLAIMS.md 1.1/1.2/1.6 re-measured under the new methodology; README's headline numbers updated to match. Full writeup, including the old-vs-new-methodology breakdown, in CLAIMS.md's new "Re-measured after closing the dpkt-throughput regression" section. Closes #147. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01RzqEV89zgdGLb3QgnZy3vp --- CHANGELOG.md | 39 +++++++++ README.md | 2 +- benchmarks/baseline.json | 8 +- docs/CLAIMS.md | 159 +++++++++++++++++++++++++++++++++---- scripts/benchmark.py | 25 +----- src/netprotocols/_base.py | 26 +++++- src/netprotocols/packet.py | 24 ++++++ src/netprotocols/walk.py | 2 +- 8 files changed, 236 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80fa7fd..4e2194d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,45 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **Closed a decode-throughput regression against dpkt that nobody had + been watching tier over tier (#147).** Re-measuring every held claim + after #107/#124 closed showed netprotocols had gone from 1.16× faster + than dpkt right after Tier 1 to ~11% slower after Tiers 2-4 — a + reversal none of the five tiers' own re-measurements had caught, + because none of them profiled, only re-measured before/after. + `cProfile` against the corpus decode loop found two causes, neither + of them the tiers first suspected: `bytes_to_ipv6` (added by #99 for + Pyodide portability, not one of the three tiers originally named) + formatted its eight address words one f-string at a time through a + generator, ~18% of total corpus decode time; `decode_frame()`'s + `Packet` construction paid a redundant `isinstance` check, through + `Protocol`'s ABC machinery, on a list already guaranteed to hold only + `Protocol` instances, ~7-8% of `decode_frame()`'s own time. Both + fixed without changing observable behavior: `bytes_to_ipv6` now + formats all eight words in one `%`-format call (verified + byte-identical against glibc's `inet_ntop` via the existing + hypothesis test, 2.1× faster in isolation); `Packet` gained an + internal `_from_decoded()` fast-construction path, used only by + `decode_frame()` — the public `Packet(...)` constructor still + validates arbitrary arguments exactly as before. +- **`scripts/benchmark.py` now measures `decode_frame()`, the + documented public chain-walking API, instead of a hand-rolled loop + that predated it (#147).** The benchmark's `decode_netprotocols()` + kept its own copy of the pre-#88 walk loop, never updated when + `decode_frame()` shipped, so every decode-throughput figure this + project has published — including every number in `docs/CLAIMS.md` + section 1 before this release — described code the documented API's + callers never actually ran. `benchmarks/baseline.json`, five tiers + and this fix stale at a v1.3.0-era figure, was refreshed to match. + +See `docs/CLAIMS.md`'s "Re-measured after closing the +dpkt-throughput regression" section for the full profiling writeup, +before/after numbers under both the old and new benchmark +methodology, and updated 1.1/1.2/1.6 figures. + ## [2.2.0] - 2026-09-04 ### Added diff --git a/README.md b/README.md index 3b6b4d9..06d750f 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ real captured traffic (CPython 3.12.3, x86-64 Linux; re-measured 2026-09-04 — see [docs/CLAIMS.md](docs/CLAIMS.md) for every number below, its reproduction command, and its caveats): -- **Within 15% of dpkt on decode, 5.6× faster than scapy** — +- **Within 15% of dpkt on decode, 5.4× faster than scapy** — `uv run --group bench python scripts/benchmark.py --compare`. dpkt is the faster of the two comparators on this corpus; the gap moves both ways as this library and dpkt each change, and this file's job is to diff --git a/benchmarks/baseline.json b/benchmarks/baseline.json index a1d74d0..e4d6448 100644 --- a/benchmarks/baseline.json +++ b/benchmarks/baseline.json @@ -1,13 +1,13 @@ { - "frames_per_sec": 114388.4, - "calibration_per_sec": 17159.4, - "normalized": 6.6662, + "frames_per_sec": 98007.5, + "calibration_per_sec": 14922.6, + "normalized": 6.5677, "frames": 97, "repetitions": 30, "trials": 9, "recorded": { "on": "CPython 3.12.3 / x86_64 / Linux", "python": "3.12.3", - "netprotocols": "1.3.0" + "netprotocols": "2.2.0" } } diff --git a/docs/CLAIMS.md b/docs/CLAIMS.md index acc0b7c..121f49e 100644 --- a/docs/CLAIMS.md +++ b/docs/CLAIMS.md @@ -148,6 +148,121 @@ still true — more true than it was, since the gap narrowed from the 2.9× that motivated Tier 1 in the first place — it just now describes the other direction. +### Re-measured after closing the dpkt-throughput regression (2026-09-04, #147) + +Re-measuring every held claim once #107/#124 closed surfaced a +regression none of the five tiers had been watching for: decode +throughput against dpkt had reversed from the Tier 1 finding (1.16× +faster) to ~11% slower, without anyone profiling why. #147 root-caused +it with `cProfile`/`pstats` against the corpus decode loop rather than +guessing from the tier history — two of the three tiers first +suspected turned out on inspection not to touch the decode happy path +at all. + +**What the profile actually showed**, against the loop +`scripts/benchmark.py` measured at the time (a hand-rolled walk loop, +not `decode_frame()` — see the methodology fix below): + +1. **`bytes_to_ipv6` (added by #99, not one of the three originally + suspected tiers) accounted for ~18% of total corpus decode time.** + #99 replaced `socket.inet_ntop` with a hand-rolled RFC 5952 + canonicalizer for a real, non-negotiable reason — Pyodide's CPython + build has `AF_INET6` disabled — but formatted each of the 8 address + words with a separate f-string inside a generator, markedly slower + than the C-level socket call it replaced. #99 landed chronologically + after #91 (the last of the three suspects the roadmap named), so it + was invisible to that suspect list even though it lands squarely in + the Tier-1-to-now window that produced the regression. +2. **`decode_frame()`'s `Packet(*layers, ...)` construction paid an + `isinstance(layer, Protocol)` check per layer that goes through + `Protocol`'s ABC `__instancecheck__` machinery** — ~7-8% of + `decode_frame()`'s own time. Every element in that list came from + this module's own `protocol.decode()` calls immediately before, so + the invariant was already guaranteed; the check was pure redundant + validation on an internal path, not a relaxation of anything the + *public* `Packet(...)` constructor still validates for arbitrary + caller-supplied arguments. +3. **#87's registry dispatch and #91's structured diagnostics were + both ruled out directly, not just re-confirmed.** Reading every + `next_protocol()` override confirmed each keeps the documented + single-`dict.get` fast path when no registry override is passed; + reading every raise site #91 touched confirmed diagnostic fields + are populated only inside a raise, never on the happy path. Neither + shows up in the profile in any meaningful way (~2-3% combined, + matching #87's own prior 5.6 measurement of a *win*). + +**Fixes**, both behavior-preserving and verified as such before being +measured for speed: + +- `bytes_to_ipv6` rewritten to format all eight words with one + `%`-formatting call instead of eight separate f-string calls through + a generator, then slice the pre-split result for the compressed + forms. Byte-identical to the previous implementation and to glibc's + `inet_ntop` — verified by the existing + `test_bytes_to_ipv6_matches_glibc` hypothesis test (`tests/test_ip.py`) + plus a 200,000-address differential run against the prior + implementation during development, zero mismatches. 2.1× faster in + isolation. +- `Packet` gained a private `_from_decoded()` fast-construction path + (`object.__new__` plus direct attribute assignment, the same + shortcut `_base.py` already documents for Ethernet/ARP/IPv4 field + construction), used only by `decode_frame()`. The public + `Packet(...)` constructor is untouched and still validates arbitrary + arguments. + +**A methodology fix, not just a code fix:** `scripts/benchmark.py`'s +measured/gated function had never called `decode_frame()` — #88 +shipped it as the documented public chain-walking API, but the +benchmark kept its own hand-rolled copy of the pre-#88 loop, unchanged +since before Tier 1. Every decode-throughput figure published in this +file to date, including the "94,453 f/s" one this section replaces, +described code real callers of the documented API never actually ran. +`decode_netprotocols()` and `_netprotocols_chain()` now call +`decode_frame()` directly, and the now-dead hand-rolled `walk()` was +deleted rather than kept side by side. This is why the number below is +not a clean apples-to-apples successor to the figure it replaces: + +- Under the **old** methodology (hand-rolled loop), the two fixes + above closed the dpkt gap from 1.12× back to near parity (~1.02-1.03× + across repeated runs) — the two accidental-overhead fixes alone did + almost all of that work, confirming the profile. +- Under the **new** methodology (`decode_frame()`, what ships), + `decode_frame()` carries real, deliberate overhead beyond that + hand-rolled loop — the bounded-depth check (section 5) and + materializing every layer into a returned `Packet` — measured at + +5.9% over the hand-rolled loop after the `Packet` fast path (down + from +16.5% before it). That is capability, not accident, and this + register does not trade it away to chase a bigger number. + +Re-measured with the harness both fixes and the methodology fix apply +to: + +``` +uv run --group bench python scripts/benchmark.py --compare --repetitions 30 --trials 9 --depth +``` + +- CPython 3.12.3, x86-64 Linux, 97 corpus frames, same machine class as + every prior measurement in this file, run-to-run variance ≈ ±2%. + +| | frames/sec | vs. netprotocols | +|---|---|---| +| **netprotocols (post-#147, 2.2.0, via `decode_frame()`)** | **97,739** | — | +| dpkt 1.9.8 | 106,619 | 1.09× (dpkt is faster) | +| scapy 2.7.0 | 18,144 | 0.19× | + +netprotocols now decodes the corpus at 97,739 f/s against dpkt's +106,619 — dpkt is ~9% faster, an improvement on the ~11% gap this +section replaces even after accounting for the harder, more honest +workload now being measured. The vs.-scapy gap is essentially +unchanged, 5.4× against the prior 5.6× (noise-level). Decode depth is +unchanged: netprotocols still reaches further than dpkt on 27 of 97 +frames and matches it on the other 70 (see 1.6). + +`benchmarks/baseline.json` — five tiers and this fix stale at a +v1.3.0-era figure (114,388 f/s) that predated everything above — was +refreshed to this tree's number (98,007.5 f/s / 6.5677 normalized, +matching methodology). + --- ## 1. Performance @@ -155,35 +270,42 @@ the other direction. ### 1.1 "2.1× faster than scapy at decoding" **Status: VERIFIED — and still understated** -*Published 2026-09-04 (embargo lifted, #107).* +*Published 2026-09-04 (embargo lifted, #107). Re-measured 2026-09-04 +after #147 closed the dpkt-throughput regression below — see that +section for the methodology change (the benchmark now measures +`decode_frame()`, not a hand-rolled loop that predated it).* Was 36,500 frames/sec against scapy 2.7.0's 17,100 at v1.3.0. Tier 1 -brought it to 6.2×; re-measured after Tiers 2-4 (see the embargo-lift -re-measurement above): **94,453 against 16,826, a 5.6× gap.** The ratio -narrowed as netprotocols took on more work per frame (registry -dispatch, structured errors, the chain walker), but the headline "2.1×" -claim was always a conservative floor and stays true by a wide margin. +brought it to 6.2×; post-Tiers-2-4 it read 5.6×; re-measured via +`decode_frame()` after #147 (see the regression-close re-measurement +above): **97,739 against 18,144, a 5.4× gap** — flat against the prior +figure within this file's own noise band. The headline "2.1×" claim +was always a conservative floor and stays true by a wide margin. Reproduce: `uv run --group bench python scripts/benchmark.py --compare`. -The wording is a positioning decision, not a measurement one: 5.6× is +The wording is a positioning decision, not a measurement one: 5.4× is what the corpus shows on one machine today, and whoever writes the README should pick the number they are willing to defend on someone else's. ### 1.2 "Within 15% of dpkt on decode" -**Status: VERIFIED — true again, from the other side** +**Status: VERIFIED — the gap narrowed back, on a harder workload** -*Published 2026-09-04 (embargo lifted, #107).* +*Published 2026-09-04 (embargo lifted, #107). Re-measured 2026-09-04 +after #147 closed the dpkt-throughput regression below.* v1.3.0 was 2.9× slower than dpkt. Tier 1 (#82-#86) briefly put -netprotocols 1.16× *faster*. Re-measured after Tiers 2-4 (see the -embargo-lift re-measurement above): **94,453 f/s against dpkt 1.9.8's -105,525 — netprotocols is now ~11% slower**, comfortably still inside -the 15% band the claim names, just no longer ahead. The added surface -across Tiers 2-4 — a registry-backed dispatch table (#87), a shipped -chain walker (#88), and structured per-raise-site error context (#91) -chief among them — cost more than #82-#85's dispatch win banked. +netprotocols 1.16× *faster*. Post-Tiers-2-4 it read ~11% slower — a +regression nobody had profiled, closed by #147 (see the re-measurement +above for the full root-cause writeup and the methodology fix that +switched the benchmark to `decode_frame()`, the documented public API, +in the same pass). Re-measured via `decode_frame()`: **97,739 f/s +against dpkt 1.9.8's 106,619 — netprotocols is now ~9% slower**, +comfortably still inside the 15% band the claim names, and a smaller +gap than the figure it replaces despite now measuring more work per +call (the bounded-depth check and full `Packet` construction that the +old hand-rolled benchmark loop never exercised). Reproduce: `uv run --group bench python scripts/benchmark.py --compare --repetitions 30 --trials 9`. @@ -263,7 +385,10 @@ file. Both move with releases — re-check before quoting. **Status: VERIFIED** *Published 2026-09-04 (embargo lifted, #107). Re-confirmed unchanged -against the Tiers-2-4 tree — same 27/70 split as originally measured.* +against the Tiers-2-4 tree, and again after #147's `decode_frame()` +methodology fix — same 27/70 split as originally measured; depth +comes from the decoders' own strictness, untouched by anything the +throughput fix or the benchmark-methodology change did.* On the 97-frame corpus, netprotocols reaches a deeper layer than dpkt on **27 frames** and stops at the same layer on the other 70. dpkt diff --git a/scripts/benchmark.py b/scripts/benchmark.py index 2972487..b424404 100755 --- a/scripts/benchmark.py +++ b/scripts/benchmark.py @@ -62,7 +62,7 @@ sys.path.insert(0, str(REPOSITORY / "src")) -from netprotocols import Ethernet, Protocol, ProtocolError # noqa: E402 +from netprotocols import ProtocolError, decode_frame # noqa: E402 def read_pcap(path: Path) -> list[bytes]: @@ -92,23 +92,11 @@ def corpus_frames() -> list[bytes]: ] -def walk(frame: bytes) -> int: - """The documented chain walk; returns the number of layers.""" - cursor, layers = 0, 0 - protocol: type[Protocol] | None = Ethernet - while protocol is not None: - header = protocol.decode(frame[cursor:]) - cursor += header.header_len - protocol = header.next_protocol() - layers += 1 - return layers - - def decode_netprotocols(frames: list[bytes]) -> int: decoded = 0 for frame in frames: try: - walk(frame) + decode_frame(frame) except ProtocolError: continue decoded += 1 @@ -264,14 +252,7 @@ def _comparison_caveats() -> list[str]: def _netprotocols_chain(frame: bytes) -> list[str]: - cursor, names = 0, [] - protocol: type[Protocol] | None = Ethernet - while protocol is not None: - header = protocol.decode(frame[cursor:]) - cursor += header.header_len - names.append(type(header).__name__) - protocol = header.next_protocol() - return names + return [type(layer).__name__ for layer in decode_frame(frame)] def _dpkt_chain(frame: bytes) -> list[str]: diff --git a/src/netprotocols/_base.py b/src/netprotocols/_base.py index ef6b8f0..da4ad02 100644 --- a/src/netprotocols/_base.py +++ b/src/netprotocols/_base.py @@ -126,8 +126,17 @@ def bytes_to_ipv6(data: bytes) -> str: whatever :mod:`ipaddress` decides this release. See :func:`ipv6_to_bytes` for why sidestepping ``socket`` entirely also matters — Pyodide's CPython build has ``AF_INET6`` disabled. + + The eight words are rendered with one ``%``-format call rather + than a per-word f-string through a generator: profiling the + decode corpus (see the PR that made this change) showed the + generator/join form costing ~18% of total decode time, almost + entirely interpreter overhead of formatting one word at a time + rather than the algorithm itself. ``%`` formats all eight in a + single C call; the compressed forms then slice the pre-split + result instead of re-formatting a subset of the words. """ - words = _IPV6_WORDS.unpack(bytes(data)) + words = _IPV6_WORDS.unpack(data) # Longest run of consecutive zero words, leftmost on a tie (RFC # 5952 4.2.3); single zero words are not worth compressing (4.2.2). @@ -162,11 +171,20 @@ def bytes_to_ipv6(data: bytes) -> str: ) return prefix + ".".join(str(octet) for octet in octets) + # %-formatting the whole tuple in one call, not f-string + # specifiers: measured faster here specifically (a single + # all-in-one f-string interpolating all eight words ran ~2.6x + # slower, str.format() ~1.7x slower — both still pay per-value + # interpreter overhead that % 's one-shot tuple formatting does + # not), so this one line keeps %, everywhere else in the codebase + # still prefers f-strings. + hexed = "%x:%x:%x:%x:%x:%x:%x:%x" % words # noqa: UP031 if best_start == -1: - return ":".join(f"{word:x}" for word in words) + return hexed - head = ":".join(f"{word:x}" for word in words[:best_start]) - tail = ":".join(f"{word:x}" for word in words[best_start + best_len :]) + parts = hexed.split(":") + head = ":".join(parts[:best_start]) + tail = ":".join(parts[best_start + best_len :]) return f"{head}::{tail}" diff --git a/src/netprotocols/packet.py b/src/netprotocols/packet.py index c3a13f9..b9ba8c0 100644 --- a/src/netprotocols/packet.py +++ b/src/netprotocols/packet.py @@ -51,6 +51,30 @@ def __init__( #: to report. self.stopped_by: ProtocolError | None = stopped_by + @classmethod + def _from_decoded( + cls, layers: list[Protocol], stopped_by: ProtocolError | None + ) -> Self: + """Build a packet from headers :func:`~netprotocols.decode_frame` + just decoded, skipping :meth:`__init__`'s per-layer + ``isinstance`` check. + + Internal only — the public constructor takes arbitrary + caller-supplied arguments and has to validate them, but + ``decode_frame`` calls this with a list built exclusively from + this module's own ``protocol.decode()`` calls, so the + invariant ``__init__`` checks already holds. Skipping it + matters: profiling showed that check alone, run through + :class:`~netprotocols._base.Protocol`'s ABC machinery, as + ~7-8% of ``decode_frame``'s total time. Same + ``object.__new__`` shortcut :mod:`netprotocols._base` uses for + Ethernet/ARP/IPv4 field construction, documented there. + """ + packet = object.__new__(cls) + packet.layers = tuple(layers) + packet.stopped_by = stopped_by + return packet + def __bytes__(self) -> bytes: return b"".join(bytes(layer) for layer in self.layers) diff --git a/src/netprotocols/walk.py b/src/netprotocols/walk.py index 4d1d03c..7376370 100644 --- a/src/netprotocols/walk.py +++ b/src/netprotocols/walk.py @@ -170,4 +170,4 @@ def decode_frame( raise stopped_by = error - return Packet(*layers, stopped_by=stopped_by) + return Packet._from_decoded(layers, stopped_by)