Repository navigation
Conversation
…ad payload copy A backed L1 hit through AutoSerializer or OrjsonSerializer re-ran its integrity check on every hit and copied the payload: Auto's bytes(data) copied unwrap's zero-copy view, and orjson copied twice (bytes(data), data[8:]). - The kept L1 entry generalises: VerifiedMsgpack becomes VerifiedPayload (in serializers.base), and records the serializer class that verified it, so only that serializer decodes it. A keeping handler hands any other owner's entry to deserialize_data, which refuses it as a corrupt frame. - Each keeping serializer decides what it keeps (_deserialize_keeping returns None for anything else, which goes to deserialize_data unchanged): StandardSerializer as before; AutoSerializer only plain MessagePack envelopes, through the same retrieve and agreement checks deserialize runs (NumPy, Arrow, DataFrame and Series entries keep the envelope); orjson the JSON under its checksum, copied once on the first hit. - Auto and orjson read unwrap's view through immutable_buffer instead of copying it; orjson checksums and decodes a view of the JSON. - make perf-ir gates l1_hit_backed_auto, the Auto variant of l1_hit_backed.
…cord l1_hit_backed_auto budgets A direct deserialize() of bytes (the round-trip budgets) paid for the immutable_buffer call and, for orjson, a memoryview, where the bytes() it replaced was free: serializer_orjson +8.4%, serializer_auto +1.1%. bytes now skips the call and orjson slices what it was given, so unwrap's view stays a view and direct bytes callers cost what they did.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Verified payloads and serializer decode paths src/cachekit/serializers/base.py, src/cachekit/serializers/standard_serializer.py, src/cachekit/serializers/auto_serializer.py, src/cachekit/serializers/orjson_serializer.py |
A shared VerifiedPayload stores payload bytes and the serializer class that verified them. Serializer decode paths create and decode these payloads. Auto serialization retains eligible MessagePack payloads, while OrjsonSerializer retains verified JSON. |
L1 retention and decode integration src/cachekit/cache_handler.py, src/cachekit/decorators/wrapper.py, src/cachekit/l1_cache.py, tests/unit/test_l1_verified_msgpack.py, tests/unit/test_single_flight.py, docs/data-flow-architecture.md, docs/features/prometheus-metrics.md |
The L1 path retains eligible plaintext payloads and decodes them only through their owning serializer. Tests cover ownership, corrupt entries, serializer decode parity, and sync and async hits. Documentation describes retention and payload-size metrics. |
Performance coverage and behavior documentation tests/performance/*, tests/unit/test_ir_budget.py, docs/performance.md, .secrets.baseline |
A backed L1-hit workload and instruction budgets cover AutoSerializer. The performance guide describes the serializer-specific hit paths. The secrets baseline updates the finding line and generation timestamp. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Other
Sequence Diagram(s)
sequenceDiagram
participant Wrapper
participant CacheSerializationHandler
participant AutoSerializer
participant L1Cache
Wrapper->>CacheSerializationHandler: Deserialize L1 entry
CacheSerializationHandler->>AutoSerializer: Verify envelope and decode eligible payload
AutoSerializer-->>CacheSerializationHandler: Decoded value and owned VerifiedPayload
CacheSerializationHandler-->>Wrapper: Return decoded value and retained payload
Wrapper->>L1Cache: Retain eligible payload
Possibly related PRs
- cachekit-io/cachekit-py#662: Introduced verified MessagePack retention and the
deserialize_l1path that this change generalises. - cachekit-io/cachekit-py#671: Extended the retained L1-entry implementation and
deserialize_l1types, which this change reworks around serializer-owned payloads. - cachekit-io/cachekit-py#685: Added the backed L1-hit budget and tests that this change extends to AutoSerializer.
Merge Risk: 
32dd7
-
No actionable issue remains from this review; the long docstring line does not block the configured checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the main performance change: AutoSerializer and OrjsonSerializer L1 hits skip repeated verification and per-read copying. Description check The description is detailed and covers the change, motivation, implementation, measurements, documentation, testing scope, and backward compatibility. It does not use every template heading or include… Docstring Coverage Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed and covers the change, motivation, implementation, measurements, documentation, testing scope, and backward compatibility. It does not use every template heading or include the explicit security and checklist confirmations, but it is mostly complete and directly related to the pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 40 functions across 12 files. (5 skipped: 5 unsupported.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
Kody Code Review — 1 suggested fix. 🛠️ Open Agent Prompt |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Instruction budget: PASSHead
A path fails at +1% or more against the base (+2% for |
The metrics page said only the default serializer swaps an L1 envelope for its verified payload; auto (plain MessagePack) and orjson do too. Orjson keeps its JSON, which has no decompress step to skip, and auto never keeps NumPy, Arrow, DataFrame or Series entries. The auto keeping docstring now says its NUMPY_RAW test is broader than deserialize's on purpose.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
The 8-byte figure compared the kept JSON against the serializer's own envelope, but the L1 entry is the full frame, which also carries the frame header.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
…bc4a7 # Conflicts: # docs/performance.md
The free-threaded CI lanes install the test group only, so orjson is absent there and every orjson-parametrised case raised ImportError. Those parameter sets now carry the suite's usual "requires the [json] extra" skip. The checks fixture imported orjson_serializer only to reach the xxhash module, so the auto case errored too. It now patches xxhash directly: both serializers call xxh3_64_digest through that module, so the count is unchanged, and the auto case runs on the free-threaded build.
|
Resolved |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
Action performed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A backed L1 hit through the auto or orjson serializer now verifies an entry once, on its first hit, and then skips the integrity check, as the default serializer has since #662. It also no longer copies the payload on every read.
What changed
VerifiedMsgpackis renamedVerifiedPayloadand moves toserializers/base.py. It now records the serializer class that verified it (owner).deserialize_l1decodes a kept entry only when its owner is this handler's serializer; any other owner goes todeserialize_data, which refuses it as a corrupt frame. Before this change only the default serializer kept entries, so the type alone was enough._deserialize_keeping(payload, metadata)returnsNonefor an entry it does not keep, and the handler decodes that entry throughdeserialize_dataexactly as before.compressed.retrieve()and format-agreement checks thatdeserializeruns. Those checks are factored into_decode_verified_envelope, which both paths call. NumPy, Arrow, DataFrame and Series entries, and bytes holdingNUMPY_RAWat offset 0 or 8, keep their envelope and are verified on every hit.bytes(data)withimmutable_buffer, so unwrap's zero-copy view stays a view. Orjson checksums and decodes a slice of the view, where it used to make two copies. Abytesargument skips the coercion: on small values the function call costs more than the copy it saves, and the round-trip budgets caught that (serializer_orjson+8.4% before the fix).make perf-irgatesl1_hit_backed_auto, which isl1_hit_backedwithserializer="auto".Unchanged: a hit still runs its serializer's bounded decode. Encrypted handlers still keep ciphertext, and L2 reads still verify. A kept entry is still swapped in only when it is no longer than the envelope, so compressible auto values keep their envelope; for them this change removes only the copy.
Measurements
Instruction counts are main-thread Ir per call from
ir_budget.py --base, measured against the merge-base4b2c65con CPython 3.14.3 with both checkouts in one run:l1_hit_backed_auto(new)serializer_autoserializer_orjsonl1_hit_backedThe full gate's other 14 paths sat between −0.18% and +0.36%. I skipped the two real-data paths (
*_usgs) locally; CI's gate measures them.Thread CPU and minor page faults per backed L1 hit come from fresh processes, 10 randomised base/candidate pairs per shape, mean paired delta with a bootstrap 95% CI. The table is under glibc's default malloc. A second run with
MALLOC_MMAP_THRESHOLD_=4194304 MALLOC_TRIM_THRESHOLD_=16777216agreed in sign on every row.At 1 MB, page faults were already near zero for these hits under both malloc settings, so the saving is CPU, not faults. An orjson hit on record-heavy JSON is dominated by building the objects, so neither the copy nor the checksum shows there.
l1_hit_backed_auto's committed budgets (91,986 on 3.12, 92,950 on 3.14) were measured on a different machine from the other rows. On that machine the untouched paths read 3% to 30% below their committed budgets (l1_hit_backed3.4% below), so I did not re-record them. A non---basemake perf-iron the machine that recorded the other rows may therefore faill1_hit_backed_autowith no code change;--baseruns, which CI uses, report the new path asnew. Re-recording every row on one machine is tracked separately.Docs
docs/data-flow-architecture.md,docs/features/prometheus-metrics.md(the L1getsize after the first hit, per serializer) anddocs/performance.mdnow describe the auto and orjson kept entries, and the budgets table has the new row.Sibling paths
deserialize_l1, so they are covered.Closes LAB-12119