perf(corekit): resolve a ModuleDescriptor through sys.modules, not import_module (#574) - #586
Conversation
…port_module (#574) (#586) Proposed by @Ts-Boom in #563. A next layer code nobody registered resolves to the fallback ModuleDescriptor the registry's default factory produces, and _lookup_next_layer deliberately does not write that back -- recording a miss in a class-level defaultdict is the defect #425/#428 fixed at this layer and #560 fixed at the schema layer. So every unrecognised frame resolved the same descriptor again: 48 of the 52 ModuleDescriptor.klass resolutions an extraction of many_interfaces.pcapng performs, each re-entering importlib.import_module for a module sys.modules already held. - ModuleDescriptor.klass now reads sys.modules first, and enters import_module only when the module is not loaded yet -- or when the loaded module does not have the attribute, which is a body still executing (a circular import, or another thread part way through importing it) and is what import_module's per-module lock exists to wait for. Measured on CPython 3.14.7: klass 436 -> 117 ns, the whole miss path 883 -> 526 ns, and import_module calls during extract() 48 -> 0 on many_interfaces.pcapng and 4 -> 0 on ipv4.pcap. The hit path is untouched (124 -> 127 ns, inside noise), since it is already memoised by the registry write-back. - Nothing memoises the resolved class, which is the deliberate part. The class is re-read with getattr on every access, so sys.modules stays the only module cache in play and its invalidation is the interpreter's: importlib.reload rebinds the class inside the same module object, and sys.modules.pop() replaces the object outright. #563's class-level _MODULE_CACHE followed neither, and an instance built from the class it kept fails isinstance against the live one. - Records in _lookup_next_layer's docstring why no memo lives there, so the next reader does not add one. Honest about the scale: this is not measurable in extract() wall clock. 48 avoided calls is ~17 us against a ~37 ms extraction, two orders of magnitude inside this host's single-digit-millisecond run-to-run variance, and repeated A/B pairs flipped sign. #563's "~40% of cumulative time" does not reproduce: _import_next_layer's self time is 0.58% against its 90.2% cumulative, because it is a recursive-descent dispatcher that has the whole nested parse beneath it. aenum.extend_enum at 16.7% self time is where the real time is (#575). Also found, not fixed here: the function-level `from ... import NoPayload` statements on the protocol layer, one of which is _import_next_layer's length == 0 fast path, run 890 times on http.pcap at ~162 ns against ~58 ns for the sys.modules equivalent -- ~92 us on a ~540 ms extraction, so left alone rather than paid for with a second resolution path. Adds tests/protocols/test_dispatch_default_resolution_unit.py, and four cases to tests/corekit/test_module.py. The two that assert the saving fail on the pre-fix code (5 import_module calls for 5 lookups); the three guards fail against the designs this one rejects -- the reload guard against #563's cache, and all three against writing the resolved fallback back under the missed code. Closes #574.
5c8bc21 to
524d246
Compare
…port_module (#574) (#586) Proposed by @Ts-Boom in #563. A next layer code nobody registered resolves to the fallback ModuleDescriptor the registry's default factory produces, and _lookup_next_layer deliberately does not write that back -- recording a miss in a class-level defaultdict is the defect #425/#428 fixed at this layer and #560 fixed at the schema layer. So every unrecognised frame resolved the same descriptor again: 48 of the 52 ModuleDescriptor.klass resolutions an extraction of many_interfaces.pcapng performs, each re-entering importlib.import_module for a module sys.modules already held. - ModuleDescriptor.klass now reads sys.modules first, and enters import_module only when the module is not loaded yet -- or when the loaded module does not have the attribute, which is a body still executing (a circular import, or another thread part way through importing it) and is what import_module's per-module lock exists to wait for. Measured on CPython 3.14.7: klass 436 -> 117 ns, the whole miss path 883 -> 526 ns, and import_module calls during extract() 48 -> 0 on many_interfaces.pcapng and 4 -> 0 on ipv4.pcap. The hit path is untouched (124 -> 127 ns, inside noise), since it is already memoised by the registry write-back. - Nothing memoises the resolved class, which is the deliberate part. The class is re-read with getattr on every access, so sys.modules stays the only module cache in play and its invalidation is the interpreter's: importlib.reload rebinds the class inside the same module object, and sys.modules.pop() replaces the object outright. #563's class-level _MODULE_CACHE followed neither, and an instance built from the class it kept fails isinstance against the live one. - Records in _lookup_next_layer's docstring why no memo lives there, so the next reader does not add one. Honest about the scale: this is not measurable in extract() wall clock. 48 avoided calls is ~17 us against a ~37 ms extraction, two orders of magnitude inside this host's single-digit-millisecond run-to-run variance, and repeated A/B pairs flipped sign. #563's "~40% of cumulative time" does not reproduce: _import_next_layer's self time is 0.58% against its 90.2% cumulative, because it is a recursive-descent dispatcher that has the whole nested parse beneath it. aenum.extend_enum at 16.7% self time is where the real time is (#575). Also found, not fixed here: the function-level `from ... import NoPayload` statements on the protocol layer, one of which is _import_next_layer's length == 0 fast path, run 890 times on http.pcap at ~162 ns against ~58 ns for the sys.modules equivalent -- ~92 us on a ~540 ms extraction, so left alone rather than paid for with a second resolution path. Adds tests/protocols/test_dispatch_default_resolution_unit.py, and four cases to tests/corekit/test_module.py. The two that assert the saving fail on the pre-fix code (5 import_module calls for 5 lookups); the three guards fail against the designs this one rejects -- the reload guard against #563's cache, and all three against writing the resolved fallback back under the missed code. Closes #574.
524d246 to
b6c314f
Compare
|
❌ NEEDS CHANGES at head sha |
|
Reviewer: Sonnet; PR authored on Opus 5. Falsify-not-bless pass on PR #586 ( 1. The behavioral claim, re-derived from the code itself (no RFC applies here)Read
2. Tests: proven to fail without the fix, independently, not taken on the PR's wordChecked out Then This matches the PR body's own reported "2 failed, 6 passed" exactly. Verdict: the two new regression tests genuinely pin the fix; they do not pass either way. I did not separately re-verify the two reload-invalidation tests and the write-back guard test against a re-applied #563-style cache (the PR's own claim for that specific reproduction) — noting that as unverified by me, not confirmed. What I did verify directly: 3.
|
|
✅ GOOD TO GO at head sha |
|
Reviewer: Sonnet; PR authored on Opus 5. Re-point of my review from head What carries over unchanged, and what I actually re-checkedThe source (
Verdict basisThe blocking issue from my first pass is resolved and independently confirmed at the source (grep + |
JarryShaw
added
the
perf
1 participant
Proposed by @Ts-Boom in #563. The profiling that found this, and the observation that a registry miss re-resolves its module on every frame, are theirs. #563 was declined for the shape of its cache rather than for the finding; this is the version #574 asked for, and the miss-path measurement in it reproduces exactly as they described.
Fixes #574.
The defect
A next layer code nobody registered resolves to the fallback
ModuleDescriptorthe registry's default factory produces — normallyRaw— andProtocolBase._lookup_next_layerdeliberately does not write that back. Recording a miss in a class-leveldefaultdictis the defect #425/#428 fixed at this layer and #560 fixed at the schema layer, so not writing it back is correct. The cost is that every unrecognised frame resolved the same descriptor again, each time re-enteringimportlib.import_modulefor a modulesys.modulesalready held.Measured
ModuleDescriptor.klassresolutions per extraction, in a warmed process:klassreadsimport_modulecalls, beforehttp.pcapipv4.pcapmany_interfaces.pcapngThe change
pcapkit/corekit/module.py:68-80—ModuleDescriptor.klassreadssys.modulesfirst and entersimportlib.import_moduleonly when the module is not loaded yet, or when the loaded module does not have the attribute. That second case is a module whose body is still executing — a circular import, or another thread part way through importing it — which is exactly whatimport_module's per-module lock exists to wait for, so behaviour is preserved rather than traded away. A name that genuinely does not exist still raisesAttributeErrorwith the same message.pcapkit/protocols/protocol.py:1373-1381— a docstring paragraph on_lookup_next_layerrecording why no memo lives there, so the next reader does not add one.Invalidation: there is no memo, deliberately
The class is re-read with
getattron every access. That is the whole design:sys.modulesis the module cache, and it is the only one whose invalidation the interpreter maintains. Every alternative caches a class object, and every cached class object goes stale:importlib.reloadre-executes the module body into the same module object, so the class it defines is a new object whilesys.modules[name]is unchanged. A memo validated against the module object's identity would not even notice.sys.modules.pop()followed by a re-import replaces the module object outright.Reproduced on this branch with #563's
_MODULE_CACHEreapplied —test_no_stale_class_survives_a_module_reloadfails with<class 'pcapkit.protocols.misc.raw.Raw'> is not <class 'pcapkit.protocols.misc.raw.Raw'>, which is the owner's reproduction on #563 turned into a test.Two shapes #574 raised as candidates were tried and rejected, both proven to break the guards:
test_a_missed_code_can_still_be_registered_without_a_warning(99 unexpectedly found in defaultdict(...)), and fails the pre-existingtest_lookup_next_layer_reads_the_fallback_without_recording_ittoo. It is the protocols: stop next-layer dispatch writing to shared registries, and fix two transport defects #426/protocols: stop option, chunk and block dispatch writing to shared registries #428 symptom: a later genuineregister(99, ...)warns about an entry no caller asked for.default_factory. The descriptor itself is two strings and cannot go stale, so this is safe — but it saves only the 250 ns factory call (~12 µs per capture), mutates a shared registry attribute, and puts dispatch state in a second place. Measured and left out; the numbers are below.Measurement, and what it does not show
Method:
PYTHONSAFEPATH=1, worktree root prepended tosys.path,pcapkit.__file__asserted to be inside the worktree before anything else is imported;.venv/bin/python3.14.7. Microbenchmarks aretimeit, best of 5 × 200k. Call counts are exact, from a counting wrapper aroundimportlib.import_module.ModuleDescriptor.klass_lookup_next_layer, miss_lookup_next_layer, hitregistry.default_factory()aloneWhole-
extract()wall clock shows nothing, and I am not going to claim otherwise. 15 runs each:http.pcapipv4.pcapmany_interfaces.pcapngThe mechanism predicts ~17 µs of saving on
many_interfaces.pcapng(48 × 357 ns) — 25× smaller than that capture's run-to-run stdev, and the measured median moved the wrong way by less than one stdev. So the change is taken for its shape, not its speed.#563's "~40% of cumulative time" does not reproduce, and #575's warning holds:
_import_next_layeris a recursive-descent dispatcher, so its 90.2% cumulative time onhttp.pcapis the whole nested parse beneath it, while its self time is 0.58%.aenum.extend_enumat 16.7% self time is where the real time is.Tests
coverage run -m pytest(neverpytest-cov), onpcapkit/corekit/module.py:Six more statements and two new branch arcs, all exercised; 1 test case became 8 across the two files.
Proven to fail on the unfixed tree (exit code read from a file, not from the printed summary):
— five
import_modulecalls for five lookups of an already-imported module. With the fix:8 passed.The three guards pass on
mainas well, becausemainis not stale-prone; what gives them teeth is that each fails against the design it exists to rule out, shown above.Found, not fixed
The function-level
from ... import NoPayloadstatements on the protocol layer — one of which is_import_next_layer'slength == 0fast path, and which #563 also cached — execute 890 times onhttp.pcapat ~162 ns each, against ~58 ns for thesys.modulesequivalent. That is ~92 µs on a ~540 ms extraction, so it is left alone rather than paid for with a second resolution path. Worth noting that at 162 ns a function-levelfrom X import Yis nowhere near as expensive as #563's write-up assumed;import_moduleat 436 ns was the costly one.Checks
mypy --follow-imports=silent --ignore-missing-imports pcapkit/corekit/module.py— clean.pylintwith the project'sMakefileflags — 10.00/10.isort -l100 -ppcapkit --check-only— clean.vermin— minimum 3.6, unchanged.origin/main; one commit.CHANGELOG.mdis generated, so the entry is written indocs/source/changelog/1.5.0.rstandCHANGELOG.mdregenerated withpython util/changelog_md.py;--checkis in step. The first push failed that gate because the entry used:data:/:func:/:class:/:file:roles, which the six conversion rules do not cover — rewritten in the supportedliteralsubset.