Skip to content

perf: call field.__copy__() directly instead of copy.copy(self) - #733

Merged
JarryShaw merged 2 commits into
mainfrom
perf/730-field-copy-dispatch
Sep 24, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
perf/730-field-copy-dispatch

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) -- scoped to the changed files; findings are pre-existing, verified against baseline
  • make test passes, and a test case covers the change -- ran pytest tests/corekit/ (199 passed, 1915 subtests) plus a broader protocol/integration set (81 passed, 1 skipped, 477 subtests); did not run the full suite
  • Added a changelog entry -- N/A, changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

  • perf — changes performance, not behaviour

Description

copy.copy(self) at the 8 per-field-per-packet __call__ sites still has to find FieldBase.__copy__ before calling it: getattr(cls, '__copy__', None). That lookup alone profiled at 55,846 calls (~1.7% of an extract() run) on examples/captures/http.pcap -- reproduced here exactly. copier = getattr(cls, '__copy__', None); copier(x) in copy.py is x.__copy__(), so calling it directly removes the lookup without changing when a field is copied or what the copy contains.

Skipping the copy itself (rather than just its dispatch) was considered and ruled out: NumberField/_TextField.__call__ unconditionally mutate the copy their super().__call__() returns, and OptionField.unpack mutates _option_padding on whatever __call__ handed back, so a field with no callback can still need a fresh copy for reasons unrelated to the callback.

Byte-identity verified across 17 classic-PCAP samples (1500 subtests, before and after, identical). End-to-end effect is below this host's noise floor (25-rep spread ~0.43s vs. a ~2ms expected shift); the isolated dispatch saving is real and reproducible in a tight microbenchmark (~0.58us -> ~0.42us per copy).

Fixes #730.

…lf) (#730)

- copy.copy(self) at the 8 per-field-per-packet __call__ sites in field.py,
  collections.py and misc.py still has to find FieldBase.__copy__ before
  calling it -- getattr(cls, '__copy__', None) -- and that lookup alone
  profiled at 55,846 calls (~1.7% of an extract() run) on http.pcap.
- copier = getattr(cls, '__copy__', None); copier(x) in copy.py is exactly
  x.__copy__(), so calling self.__copy__() directly removes the lookup
  without changing when a field is copied or what the copy contains.
- Verified: NumberField/_TextField's __call__ unconditionally mutate the
  copy their super().__call__() returns, and OptionField.unpack mutates
  _option_padding on whatever __call__ returned, so skipping the copy
  itself (rather than just how it dispatches) was ruled out as unsafe.
- Added byte-identity coverage across 17 classic-PCAP sample captures and
  a mechanism test proving copy.copy is no longer reached at any of the
  8 sites (fails without the fix).

Build: `pytest tests/corekit/` -- 199 passed, 1915 subtests. Broader
protocol/integration run -- 81 passed, 1 skipped, 477 subtests. mypy/pylint/
isort clean relative to baseline.
@JarryShaw JarryShaw added the perf Pull requests that improve performance (perf: subject prefix) label Sep 24, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ a4e0db9d6 — the swap is provably a no-op in semantics: CPython's copy.copy does copier = getattr(cls, "__copy__", None); return copier(x) (copy.py:80-82), and FieldBase.__copy__ (field.py:307) is the only __copy__ on any field, so calling it directly is what copy.copy already resolved to.

Cross-review ran on Opus against Sonnet-authored work and returned GOOD TO GO. Posted here by the coordinator: that reviewer was a read-only helper with no gh pr comment grant, so its verdict existed only in a hand-back report. Independently verified before posting:

Claim Result
copy.copy(self) ≡ self.__copy__() confirmed from installed CPython 3.14.7 copy.py:80-82
No field subclass overrides __copy__ confirmed — only field.py:307 and the unrelated multidict.py:432
8 call sites swapped, import copy dropped from all 3 files confirmed in the diff
Copy count unchanged this is a dispatch change, not a skip — every field is still copied as often as before

Not a "skip the copy when callbacks are trivial" change, and the author was right to reject that: NumberField.__call__ and _TextField.__call__ unconditionally rewrite _template on whatever super().__call__() returns, so returning self would corrupt the class-level Schema.__fields__ descriptor.

Two caveats carried forward honestly from the author rather than dropped:

  • End-to-end gain is unmeasurable, and the PR says so: ~2 ms mean difference against a 0.43 s run-to-run spread over 25 reps. The real figure is the isolated mechanism — 0.580 µs → 0.420 µs per copy, ~28%, ~9 ms aggregate over 55,846 copies.
  • Pre-existing inconsistency, deliberately untouched: ListField.__call__ passes the original self to _callback, not new_self, unlike every other site. Out of scope here; changing it would be a behaviour change rather than a dispatch one.

✅ GOOD TO MERGE @ a4e0db9d6 — pending its CI, which is still queued.

@JarryShaw JarryShaw added the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 24, 2026
@JarryShaw
JarryShaw merged commit 55513f6 into main Sep 24, 2026
22 checks passed
@JarryShaw
JarryShaw deleted the perf/730-field-copy-dispatch branch September 24, 2026 04:43
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 24, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf Pull requests that improve performance (perf: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

perf: copy.copy in the field __call__ path costs 55,846 getattr calls (~1.7% of extraction)

1 participant