Skip to content

perf(pipeline): copy stage state without dataclasses.replace - #546

Merged
derek73 merged 2 commits into
derek73:masterfrom
akamick86:perf/pipeline-state-copy
Sep 27, 2026
Merged

derek73 merged 2 commits into
derek73:masterfrom
akamick86:perf/pipeline-state-copy

Conversation

@akamick86

@akamick86 akamick86 commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

What

Every stage returns a copy of the frozen ParseState, and some stages also copy tokens one at a time. All of those copies go through dataclasses.replace, which walks fields() and calls __init__ each time. One parse of the benchmark's reference name makes 18 of them: six of the state and twelve of tokens, from classify and assign.

Change

copy_with in _pipeline/_state.py copies the fields directly. That builds the same object replace does for a dataclass that is decorated itself, keeps the generated __init__, and has no __post_init__ and no init=False field. _copyable_fields checks exactly those four, and a read-only table built at import runs it over WorkToken, PendingAmbiguity and ParseState. So a class that stops qualifying fails at import, and copy_with refuses anything else. An unknown field still raises TypeError.

For mypy, copy_with is from dataclasses import replace as copy_with, so the dataclass plugin keeps checking field names and types at every call site.

The 28 call sites in the stage modules switch over. Nothing outside _pipeline changes.

Numbers

Calls per parse of the reference name, uv run python tools/perf/call_count.py --against e0f1a2f, parse / facade:

master this branch
3.11 406 / 443 370 / 407
3.12 384 / 421 348 / 385
3.13, 3.14, 3.15 402 / 439 348 / 385

That is 18 copies times the frames each one saves: two on 3.11 and 3.12, three from 3.13. On wall clock I measured 8 to 13% per name on synthetic names.

Behavior

No change. The differential gate exits 0 at all five baselines, and its report is identical to master's line for line apart from the header that names the checkout path.

Docs & tests

  • _CALL_BASELINE rows lowered for 3.11 to 3.15, and _LINK_BASELINE for 3.11 (2587 to 2301), each measured on its own interpreter. The rows drop by 40 and 58 because master already sat 4 under every row.
  • decisions.md#parse-cost entry with the numbers and how to recompute them.
  • Release log bullet under 2.4.0, stated in calls.
  • tests/v2/pipeline/test_state.py: copy_with builds what replace builds for each pipeline class, rejects an unknown field and any other class, and the guard refuses four shapes a field copy would get wrong. That test records what replace builds and what an unguarded copy would build for each, and each guard clause fails its own row when removed.

Full suite: 9994 passed, 324 skipped, 3 xfailed on 3.11, and 9993 passed, 325 skipped, 3 xfailed on each of 3.12 to 3.15.

Every stage returns a copy of the frozen ParseState, and classify,
assign, group and post_rules also copy tokens one at a time. All of
those went through dataclasses.replace, which walks fields() and calls
__init__ on each copy: 18 copies per parse of the reference name, at
three frames each on 3.11/3.12 and four from 3.13.

copy_with in _pipeline/_state.py copies the fields directly. For a
dataclass whose generated __init__ only assigns its fields that builds
the same object, and WorkToken, ParseState and PendingAmbiguity are all
that kind. It checks this once per class and refuses a class with
__post_init__ or an init=False field.

Parse cost drops 36 calls on 3.11/3.12 and 54 from 3.13, so the
_CALL_BASELINE rows and the 3.11 _LINK_BASELINE row move down, each
re-measured on its own interpreter (decisions.md#parse-cost).
Differential gate output is unchanged at all five baselines.
@akamick86
akamick86 force-pushed the perf/pipeline-state-copy branch from 5c56d2e to d665457 Compare September 26, 2026 16:13
@codecov

codecov Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.79%. Comparing base (e0f1a2f) to head (a2edd3f).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #546   +/-   ##
=======================================
  Coverage   98.78%   98.79%           
=======================================
  Files          45       45           
  Lines        3703     3719   +16     
=======================================
+ Hits         3658     3674   +16     
  Misses         45       45           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@derek73 derek73 added this to the 2.4 milestone Sep 26, 2026
@derek73

derek73 commented Sep 26, 2026

Copy link
Copy Markdown
Owner

Thanks @akamick86, this is a nice find. The frame accounting holds up exactly: I reproduced every call count in your parse-cost bullet on 3.11 through 3.15, and the 3-versus-4 frames per dataclasses.replace call. The differential gate gives identical output at all five baselines, and a separate timing run showed 5–12% faster per parse. So the change itself looks good. The feedback below is mostly about keeping the checks that dataclasses.replace used to give for free, plus some wording in the docs.

Should fix

  1. mypy stops checking the call sites. mypy's dataclass plugin checks the keyword arguments to dataclasses.replace, but copy_with(obj, **changes: object) accepts anything. For example, replace(tok, rol=None) and replace(tok, role="nope") are both mypy errors, while the same two calls through copy_with pass. A misspelled field now fails only at runtime. A wrong-typed value fails nowhere, since these classes don't validate. A fix that costs no runtime frames is to give mypy the stdlib signature:

    if TYPE_CHECKING:
        copy_with = dataclasses.replace
    else:
        def copy_with(obj, /, **changes): ...

    Per-class @overloads would also work.

  2. _COPY_FIELDS is new module-level mutable state. AGENTS.md ("One sanctioned global") asks for a conventions amendment for that. There are two ways to avoid it, and neither adds a frame under call_count.py:

    • Put @functools.cache on _copy_fields. AGENTS.md already allows that form.
    • Or build a read-only table right after the three class definitions. That fails at import time and keeps copy_with limited to the pipeline's own classes.

    Either one also fixes a small wrinkle: a zero-field class caches (), which is falsy, so .get(cls) or ... misses every time.

  3. The guard lets through two kinds of class it claims to refuse. The docstring and the release bullet say the guard refuses "any class where it would not hold". It doesn't refuse a dataclass with its own validating __init__, where replace raises and copy_with silently builds the invalid object. It also doesn't refuse an undecorated subclass whose __init__ sets extra attributes. None of today's three classes is affected. Either tighten the check (for example, "__dataclass_params__" in cls.__dict__ plus checking that __init__ is the generated one), or narrow the wording to what it actually checks.

  4. "About 10% faster" in the release log needs a recipe. AGENTS.md's release-log rule wants a recompute recipe for a quantified claim. Your parse-cost bullet in docs/design/decisions.md, which the release note cites, has call counts (406→370 is 8.9%) and a per-stage table that adds up to about 20%. Nothing in the tree reproduces 10%. Either state it in calls ("36 fewer calls per parse on py3.11"), or add a whole-parse timing and its recipe to that bullet.

Smaller things

These are about your new 2026-09-26 bullet under parse-cost in docs/design/decisions.md, plus the tests:

  • The explanation of the 18 copies is off. The total is right, but for the reference name it's 6 state copies plus 12 token copies. extract_delimited returns the state unchanged when there's no delimiter, and script_segment returns early on ASCII input, so they copy nothing. Group and post_rules copy no tokens. Only classify and assign do.
  • The _CALL_BASELINE rows drop by 40 and 58, while the bullet explains 36 and 54. Master was already 4 frames under the old rows, inside the ±2% band. Since parse-cost says moving a row is a decision, a clause noting that would help.
  • The refusal test would still pass without the guard. _Validated(1) with value=2 gives the same result with or without the guard. Using value=-1 shows the harm, and it's worth recording what an unguarded copy produces (AGENTS.md's recorded negative control).
  • The test comment about recomputing the init=False field doesn't match _Derived, which has no __post_init__.
  • Cosmetic: some continuation lines are still indented for dataclasses.replace(, and _segment.py has a double blank line where the import was removed.

I'm happy to make these changes for you. If you'd like, reply here and I'll push a fix commit to your branch (edits by maintainers are enabled). Otherwise, feel free to take them on yourself.

… exact guard

- mypy sees copy_with as `from dataclasses import replace as copy_with`,
  so the dataclass plugin checks its keywords again. An assignment
  (`copy_with = dataclasses.replace`) does not: the plugin keys on the
  callee's full name, and a misspelled or wrong-typed field passes.
- _COPY_FIELDS is a read-only table built at import over the three
  pipeline classes, replacing the lazily filled dict. copy_with refuses
  any other class, and the zero-field falsy-cache wrinkle is gone.
- _copyable_fields checks the class's own __dataclass_params__ and a
  generated __init__, so a validating __init__ and an undecorated
  subclass are refused. The redundant params.init test is dropped.
- The refusal test records what replace builds and what an unguarded
  copy would build for each refused shape; each guard clause fails its
  own row when removed.
- Release log states the saving in calls; the decisions entry gives
  the 6 + 12 copy breakdown and why the rows drop by 40 and 58.
- Continuation lines realigned, _segment's double blank line removed.
@akamick86

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review, and for reproducing the counts. I've taken these on myself in a2edd3f.

Should fix

  1. mypy: one wrinkle here. copy_with = dataclasses.replace under TYPE_CHECKING doesn't bring the checks back, because the dataclass plugin hooks the call by the callee's full name, and an assignment creates a new name. I tried it, and copy_with(tok, rol=None) and copy_with(tok, role="nope") both still pass. from dataclasses import replace as copy_with keeps the original name, and with that both are errors again, the same ones dataclasses.replace gives. So I used the import, which also avoids per-class overloads that could drift.
  2. _COPY_FIELDS is now a read-only table built right after the classes, over the three pipeline classes, so there's no module-level mutable state. copy_with raises TypeError for any other class, and the falsy () problem goes away with the lazy dict.
  3. I tightened the guard rather than the wording. It checks the class's own __dict__ for __dataclass_params__ and requires the generated __init__, which dataclasses compiles from <string> on 3.11 through 3.15, so both of your cases are refused now. I dropped the params.init test since the generated-__init__ check already covers it.
  4. The release bullet now states the saving in calls only, with the recompute command.

Smaller things

  • The decisions entry now gives six state copies plus twelve token copies from classify and assign, and says why extract_delimited and script_segment copy nothing for this name.
  • It also says why the rows move by 40 and 58.
  • The refusal test is now a table with the negative control recorded. For each of the four shapes it records what replace builds and what an unguarded copy builds (_Validated with value=-1 among them), then asserts the guard refuses the class. I checked that each clause of the guard fails its own row when removed.
  • The init=False case records the real difference: replace resets the field to its default, and a field copy carries the old value.
  • Fixed the continuation lines and the double blank line.

Full suite is green on 3.11 to 3.15, call counts are unchanged from the first commit, and the gate still matches master at all five baselines. I've updated the PR description too.

@derek73
derek73 merged commit 952ecf5 into derek73:master Sep 27, 2026
11 checks passed
@derek73

derek73 commented Sep 27, 2026

Copy link
Copy Markdown
Owner

Merged. Looks great. thanks for the contribution @akamick86!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants