Skip to content

perf(types): reduce canonical creative serialization overhead - #1521

Merged
bokelley merged 3 commits into
mainfrom
bokelley/adcp-client-python-issue-1519
Oct 16, 2026
Merged

bokelley merged 3 commits into
mainfrom
bokelley/adcp-client-python-issue-1519

Conversation

@bokelley

@bokelley bokelley commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Formats with macro resolution capabilities repeatedly traverse their serialized trees to remove legacy creative identity. Reuse the canonical wrap serializer result for ordinary public dumps, cache key classification, and recurse only into containers. Validation reads fields directly for ordinary models and retains Pydantic serialization for custom serializers, computed fields, excluded fields, and non-JSON values.

Keep each canonical serializer as an independent boundary, including reentrant TypeAdapter calls and subclasses that replace the inherited serializer. Add regression coverage for these extension cases and for avoiding nested-model dumps during product validation.

Local benchmark using the issue reproduction with 10 macro capabilities: JSON-mode model_dump fell from 0.585 ms to 0.237 ms; validating a parent Product with an existing declaration fell from 0.884 ms to 0.224 ms. These are local minimum timings, not a timing assertion in CI.

The cross-SDK interop driver also resolves row storage from the extracted @adcp/reporting-storage package when the JavaScript SDK no longer exports it from the ledger entry point. The real bidirectional database/object-store interop gate remains enabled. Stabilize the existing feed-consumer concurrency test by holding the newer transaction before commit and proving the stale writer is blocked by its PostgreSQL backend PID; retain both checkpoint and return-value assertions.

Validation: 750 canonical/compatibility tests passed with 2 skipped, Ruff, Black, full source mypy, and strict adopter type checks. The unchanged required interop file passed all 4 tests against real PostgreSQL 16 and current JavaScript main 6326b54. The feed-consumer file passed all 6 tests, the repaired concurrency case passed 50/50 repetitions, and an unconditional-upsert negative control failed with the expected stale-checkpoint assertion. Independent serialization and validation reviews completed before publication; CI results are attached below by GitHub.

Fixes #1519

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 16, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean performance PR on the canonical creative boundary.

Checked:

  • No Critical/High/Medium findings reported by the reviewer.
  • Boundary is preserved or tightened (fail-closed): removed _format_scope/format_scope was dead everywhere; scalar short-circuits in strip/scan are equivalent to the prior recursion.
  • model_dump/model_dump_json safely skip re-stripping only when the inherited wrap serializer ran, falling back when a subclass replaces it.
  • _model_boundary_values is guarded against custom serialization and non-JSON values, with 463 lines of targeted tests covering the new branches.

Decision table: no critical/high (row 1 no); gated_paths=false (row 2 no); high_risk=false (rows 3/5 no); no medium findings (rows 4/8 no); no prior escalation (row 6 no); no no-auto-approve team match (row 7 no). Falls through to row 9 → approve.

Note: review_decision is REVIEW_REQUIRED, but with gated_paths=false row 2 does not apply.

One Low (not blocking): format_scope is left dead-threaded in _legacy_creative_identity_path.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 16, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — test-only interop driver compatibility shim with no blocking findings.

This PR changes 4 files (canonical creative types/tests plus a cross-SDK interop driver). The reviewer's delta focus is a single compat shim in tests/interop/reporting_row_storage_js_driver.cjs that resolves ReportingRowStorageV1 / createFilesystemReportingRowObjectProviderV1 from @adcp/reporting-storage after SDK 16 moved them out of the ledger module, using a correct dual-export guard (L19-22) with matching use sites (L31, L40). Test-only change: no public adcp.* surface, no wire-shape impact, no generated-code hand-edits.

Checks:

  • No critical/high/medium findings reported.
  • No public-export/semver signal concerns (test-only).
  • No forward-compat, import-layering, or generated-code violations.
  • high_risk is false; gated_paths is false. Row 2 does not apply since no gated paths are touched (review_decision REVIEW_REQUIRED is irrelevant when gated_paths is false).
  • No no-auto-approve team match.
  • Prior decision was approve with no findings; this run is a net-clean continuation.

Decision table: rows 1–8 do not fire (no critical/high, gated_paths false, no deletions, no medium findings, no team gate). Falls through to row 9 → approve.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — test-only delta since the prior approve, no blocking or medium findings.

What I checked

  • The only change since the prior approve (head 23693e8) is a rewrite of one PG conformance test (tests/conformance/reporting/test_reporting_change_feed_consumers_pg.py) to prove stale-write ordering against the real newer writer's uncommitted transaction via pg_blocking_pids, replacing a synthetic third holder transaction.
  • The new version is bounded (wait_for timeouts, 400-iter poll cap), cleans up in finally (sets commit_newer, cancels + gathers tasks), and asserts genuine overlap before commit — no hang or leak risk.
  • No public-surface changes, no generated-code hand-edits, no import-layering violations, no disabled tests / silenced gates. CI floor (ruff/mypy/pytest) unaffected.

Decision path: No critical/high findings (row 1 n/a). gated_paths: false (row 2 n/a). high_risk: false (rows 3,5 n/a). No medium findings (rows 4,8 n/a). Prior decision was approve, not escalate (row 6 n/a). No no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.

@bokelley
bokelley merged commit bde770b into main Oct 16, 2026
48 checks passed
@bokelley
bokelley deleted the bokelley/adcp-client-python-issue-1519 branch October 16, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant