Skip to content

Memoise the default ModuleDescriptor on a registry miss, instead of re-resolving it per frame (from #563) #574

Description

@JarryShaw

The mechanism PR #563 described is real, and the saving it targets is real — it is only the implementation that cannot be taken as written. This issue captures the version worth building.

Credit: 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. This issue exists because the fix wants a different shape, not because the finding was wrong.

The real defect

On a registry miss, ProtocolBase._lookup_registry() falls back to a default ModuleDescriptor for Raw and does not write it back, so ModuleDescriptor.klass re-enters importlib.import_module for every unrecognised frame. A genuine registry hit is already memoised — registry[proto] = klass once proto in registry — which is why ordinary traffic never shows this.

Measured ModuleDescriptor.klass call counts on main:

capture frames klass calls of which Raw
http.pcap 1117 4 0
ipv4.pcap 4 7 4
many_interfaces.pcapng 64 52 48

So the miss path really does re-resolve per frame. ModuleDescriptor.klass costs ~463 ns against ~39 ns for a warm dict hit, and unlike the struct.calcsize case in #551 that cost is genuine — importlib.import_module retains real per-call work beyond a sys.modules lookup even for an already-imported module.

Why #563's implementation cannot be taken

It adds a separate class-level _MODULE_CACHE keyed on a bare f'{module}.{name}' string with no invalidation of any kind. Reproduced on its branch: warm the cache, importlib.reload the module, and the cache serves the pre-reload class forever — instances built from it then fail isinstance against the live class.

That is the same family of defect this release has already fixed twice: retained class-level dispatch state, at the protocol layer in #425/#428 and the schema layer in #560. _lookup_registry in that very file carries a comment explaining why a defaultdict miss must not be written back unconditionally.

ProtocolBase.register() is not exposed, since it resolves .klass fresh and stores the class rather than the descriptor. The exposure is the two hardcoded keys in _import_next_layer's fast paths and the ~90 class-body ModuleDescriptor(...) registrations resolved through _lookup_next_layer.

The shape to build instead

Memoise the resolved default into the registry on a miss, the way a hit already is, rather than adding a second cache beside it. That captures the same saving while inheriting whatever invalidation semantics the registry has, and keeps one place where dispatch state lives instead of two.

The design question to answer deliberately: a miss currently returns the Raw/NoPayload default without recording it, and #425/#428/#560 exist precisely because recording a miss caused bugs. So write-back must distinguish "this code resolved to the default" from "this code is registered as the default" — otherwise a later legitimate register_* call for that code warns it is already registered, which is the #426/#428 symptom exactly.

Honest scale

Worth saying plainly so nobody expects a large win: the absolute saving is 4 to 48 avoided calls per capture at ~424 ns each — microseconds against multi-hundred-millisecond extractions, and undetectable in whole-extract() wall clock against single-digit-millisecond run-to-run variance on this host. #563's headline "~40% of cumulative time" does not reproduce: _import_next_layer's cumulative time is 90.2% on http.pcap but its self time is 0.58%, and _lookup_registry's is 0.09% across 8,490 calls — it is a recursive-descent dispatcher, so cumulative time attributes the entire nested parse to it.

If the goal is real throughput, the profile points elsewhere: the largest self-time consumer on http.pcap is aenum.extend_enum at 16.7%, followed by schema and field unpack and 976k getattr calls. That is where a measurable gain lives, and it deserves its own issue.

Coverage

A test that a lookup miss resolves the default once rather than per frame, and — the important half — that a later legitimate registration for that code still succeeds without warning. Plus one asserting no stale class survives a module reload, which is the defect #563 would have introduced.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementperfPull requests that improve performance (perf: subject prefix)

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions