perf(portmap): collect the socket table outside the refresh lock#50
Merged
Merged
Conversation
The capture thread takes PortTable._lock too - name_of(cheap=True) -> info() in engine._process_for - so whatever refresh() holds it for, the packet path can be made to wait for. It held it across four iphlpapi calls, the port->pid dict build AND the departed-pid diff, all O(number of sockets). Measured before touching anything, because "the hot path waits N ms" is the kind of claim convention 5 says to measure (Win11, CPython 3.14, elevated, medians with a control): - hold with everything inside: 0.495 ms median at 119 sockets, and the Python-side work scales linearly - 0.303 ms at 10 000 sockets, 3.555 ms at 100 000 (syscalls excluded, so a floor) - the capture thread's own lookup is 0.4 us, and its median/p95/p99 did NOT move with a refresher hammering alongside; 2 calls in 20 000 waited, worst 1.5 ms - so at desktop scale this was not observable, and "measured, rejected" was a legitimate outcome. What decided it was the scaling: a network tester is the thing that gets pointed at 100 000 connections, and this project's history is a series of "it was fine until someone ran a load test" After: the hold is FLAT at ~0.018 ms whatever the table size - 188x shorter at 100 000 sockets and no longer a function of n. The whole call costs the same; the work left the lock rather than disappearing. - collection and the departed diff now run unlocked; only the reference swap, _last, the _info pops and _expire_info stay under it - overlapping refreshes cannot move the map BACKWARDS: each call takes a generation before it starts and installs only if nothing newer landed. A counter and not a timestamp, because time.monotonic() has ~15 ms granularity on Windows and two refreshes in one tick would compare equal and both install. The hazard was real, not theoretical - without the guard the mutation installs the stale map - the native handle is captured per call and the "it stopped answering" flip is re-checked under the lock, so a concurrent refresh cannot have its conclusion undone - incidental finding: since chunk 2c the resolver resolves against the SocketWatcher, whose refresh() is a no-op, so in a real session only the watchdog refreshes this table (~3-5/s). The up-to-20/s case is the poller fallback, which has no real capture thread to stall - two new tests, both mutation-checked: the lock is probed from ANOTHER thread (_lock is an RLock, so a same-thread acquire would prove nothing), and the generation rule is driven by a gate rather than raced with sleeps - internal changelog only: nothing a tester can observe changes, and claiming a user-visible win would be unmeasured prose Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
The capture thread takes
PortTable._locktoo -name_of(cheap=True)->info()inengine._process_for- so whateverrefresh()holds that lock for, the packet path can be made towait for. It held it across four
iphlpapicalls, theport -> piddict build and thedeparted-pid diff, all O(number of sockets).
Measured before touching anything
"The hot path waits N ms" is exactly the claim convention 5 says to measure rather than estimate, so
this started as a measurement that could have ended in "rejected" (Win11, CPython 3.14, elevated,
medians with a control):
The hold is now flat - no longer a function of table size. The whole call costs the same; the
work left the lock rather than disappearing.
The honest part: at desktop scale this was not an observable problem. The capture thread's own
lookup is 0.4 us, and its median, p95 and p99 did not move with a thread hammering
refresh(force=True)alongside - only 2 calls in 20 000 waited at all, worst 1.5 ms, with the lockheld ~0.15% of the time. "Measured, rejected" was a legitimate outcome here. What decided it was the
scaling: a network tester is the thing that gets pointed at 100 000 connections, and this
project's history is a series of "it was fine until someone ran a load test" (the flow table once
settled at 3.2 million entries). That reasoning is written into the docstring so it is not
re-litigated as a perf win it never was.
What a reviewer should know
refreshes overlap, so each call takes a generation under the lock before it starts and installs
only if nothing newer landed. This hazard was real, not theoretical - the mutation test without
the guard installs the stale map. It is a counter and not a timestamp because
time.monotonic()has ~15 ms granularity on Windows, so two refreshes inside one tick would compare equal and both
install.
refresh()returns, the map is at least as new asthe moment the call started - either this call installed it, or a newer one already had.
nativehandle is captured per call and the "it stopped answering" flip is re-checked underthe lock (
self._native is native), so a concurrent refresh cannot have its conclusion undone.refresh()call, which equalled the hold time only while everything was inside the lock. Usedunchanged after the change it reported "3.6 ms, no improvement" - the right number for the wrong
quantity. The lock had to be instrumented and the hold measured directly.
SocketWatcher, whoserefresh()is a no-op, so in a real session only the watchdog refreshes this table (~3-5/s). Theup-to-20/s case is the poller fallback (
--simulate, tests, non-Windows), which has no realcapture thread to stall.
_lockis an RLock, so a same-threadacquire()would succeed while held and prove nothing - andthe generation rule is driven by a gate rather than raced with sleeps.
Checklist
python -m pytest testspasses locally. (669 tests, 0 failures, on an elevated shell, sothe two known non-admin failures are absent rather than excused. Verified through pytest's own
exit code and the collected count.
smoke_gui.py: OK.)tests/for the style). - two, both mutation-checked.lang/en.jsonandlang/pl.jsonupdated. -n/a: no UI text.
CHANGELOG.md; technical ones and new tests inCHANGELOG-INTERNAL.md, under[Unreleased]. - internal only, deliberately: nothing atester can observe changes, and claiming a user-visible win would be unmeasured prose.
type(scope): summary).VERSION.txt.🤖 Generated with Claude Code