Repository navigation
perf(types): reduce canonical creative serialization overhead - #1521
Conversation
There was a problem hiding this comment.
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_scopewas dead everywhere; scalar short-circuits in strip/scan are equivalent to the prior recursion. model_dump/model_dump_jsonsafely skip re-stripping only when the inherited wrap serializer ran, falling back when a subclass replaces it._model_boundary_valuesis 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 viapg_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.
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_dumpfell from 0.585 ms to 0.237 ms; validating a parentProductwith 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-storagepackage 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