Repository navigation
Server P11b-iii: shared result records, one context digest, a per-activation read table and an indexed restore (G30, G31, G32, G34) - #168
Conversation
…ivation read table and an indexed restore (G30, G31, G32, G34) - RuntimeCheckpoint.results and outbox hold Arc<PublishedValuation>; stored JSON is unchanged. A Live merge copies pointers, and a tick that values none keeps its results. - changed reads each pin's curves from a table built once per activation and frame, reused while the reference section is shared; same_inputs skips the record compare for a shared section. - ValuationContext derives its cache key once, when it is made; equality and Debug read the spec. The graph commit shares the result instead of deep-cloning it. - restore_results indexes records by subject once.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Stored valuation cache keys crates/convex-engine/src/context.rs, crates/convex-engine/src/cache.rs, crates/convex-engine/src/lib.rs, crates/convex-engine/src/snapshot.rs, crates/convex-engine/tests/snapshots.rs |
ValuationContext stores its cache key at construction, and callers use the borrowed key directly. Snapshot-result validation and cache-key tests use the stored key. |
Live input change detection crates/convex-ports/src/snapshot.rs, crates/convex-ports/tests/snapshot_contracts.rs, crates/convex-engine/src/runtime/worker.rs, crates/convex-engine/src/snapshot.rs, crates/convex-engine/RUNTIME.md |
SnapshotReferences::ptr_eq identifies shared reference sections, and same_inputs skips selected-reference comparisons when sections are shared. Baselines retain per-pin read curves for input checks. The Live tick documentation describes this behavior. |
Shared valuation result lifecycle crates/convex-ports/src/runtime.rs, crates/convex-engine/src/runtime/actor.rs, crates/convex-engine/src/runtime/mod.rs, crates/convex-engine/src/runtime/worker.rs, crates/convex-engine/tests/runtime.rs |
Runtime results and checkpoint records use Arc<PublishedValuation>. Calculation retains unchanged records, publication clones records for updates, and restoration indexes records by instrument and version. Tests check result equality and checkpoint JSON round-tripping. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor
Sequence Diagram(s)
sequenceDiagram
participant LiveTick
participant ActorCalculate
participant WorkerCalculate
participant WorkerChanged
participant ValuationCache
LiveTick->>ActorCalculate: Process tick
ActorCalculate->>WorkerCalculate: Calculate selected valuations
WorkerCalculate->>WorkerChanged: Compare baseline inputs
WorkerCalculate->>ValuationCache: Look up using context cache key
WorkerCalculate->>ActorCalculate: Return shared PublishedValuation records
Possibly related PRs
- sujitn/convex#163: Defines per-instrument valuation inputs and quality, which the changed baseline comparison uses.
- sujitn/convex#161: Introduces shared checkpoint result storage and Live inputs pinned across ticks, both extended by this change.
- sujitn/convex#164: Adds changed-only Live valuation and baseline comparison, which this change extends with shared result records and retained read curves.
Merge Risk: 
eb780
-
The change shares valuation records and reuses cached inputs. No material merge-blocking issue was established.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 64.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main changes: shared result records, one context digest, per-activation read tables, and indexed restore. It is specific and related to the pull request, although it is… 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: Title check
-
Explanation
The title clearly describes the main changes: shared result records, one context digest, per-activation read tables, and indexed restore. It is specific and related to the pull request, although it is longer than necessary.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 64.71% which is insufficient. 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 17 functions across 12 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 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.
Comment @coderabbitai help to get the list of available commands.
Summary
P11b-iii: the per-record costs of a Live tick (G30, G31, G32, G34). The step needed no new decisions.
P11a's timers put these at
burst1/256(one bond valued in 256):changed: a full valuation context for each of 256 pins (G31)At
burst512/256(about 204 bonds valued per tick),calculateis 60% of the tick at 47 µs a bond. Each valued bond computed its context digest (context JSON and SHA-256, about 3 µs) about eight times (G32).Change
RuntimeCheckpoint.resultsandoutboxholdArc<PublishedValuation>. serde'srcfeature is already on in the workspace, so stored JSON is unchanged. A Live merge copies 256 pointers, and a tick that values no bond keeps the results it has.runtime_results()and the update stream still hand outPublishedValuation, so the server is unchanged.changedreads the curves each pin's valuation reads from a table kept in the Live baseline. It is built throughcontextat the first tick of an activation or a new frame, so a pin thatcontextrefuses is still refused. It is reused while the frame holds and the snapshot shares the baseline's reference section (SnapshotReferences::ptr_eq). Quality is still derived for each pin at every tick.same_inputs. When two snapshots share one reference section, the reference compare is skipped, since it is the same section. A section copied to change it (make_mut) is compared record by record, as before.ValuationContextderives its cache key once, innew, from the same bytes as before: its own serialization, which carries no key.cache_key()returns&str. Equality andDebugread the spec alone. A deserialized context derives its key again, so a tamperedSnapshotValuationResult.cache_keyis still refused. The graph commit shares the valued result (Arc) instead of deep-cloning it.restore_resultsindexes the stored records by subject once (by_subject), so a restart is linear in the bonds. It refuses the same checkpoints: one record per pin, no duplicate subject, and as many records as pins.RUNTIME.mddescribes the shared records and the read table.Behaviour that changes
None. Ids, keys, stored bytes, refusals and their errors are unchanged.
Tests
a_context_key_is_the_digest_of_its_serialization: the key equals SHA-256 over the context's own serialization, the bytes it always hashed, and that serialization is the spec's alone. A context read back has the same key and is equal.resultsserialize as the plain records and deserialize equal.an_instrument_reads_the_same_inputs_until_its_own_changenow covers a shared section: it is the same, and a copy changed in place is not. The existing cases cover separately built but equal sections.results_account_for_each_measure_and_cannot_claim_another_context_or_generation, the restartcontext_keytamper) pass unchanged.Numbers
Criterion medians,
--profile ci, WSL. Two interleaved rounds againstmain(d90c5ef), swapping in itsconvex-engineandconvex-portssources and test support:mainlive_throughput/burst1/256(one-quote tick)live_throughput/burst512/256live_ticksnapshot/value_uncachedsnapshot/value_builtsnapshot/value_cachedsnapshot/verifysnapshot_assemble/256(Captured/Replayresolve)changedat 0.95 ms, so the rest of the tick holds about 0.2 ms of them. That is the 256 pointer copies, quality derived per pin, and the input compares.burst512/256falls by 1.4–1.6 ms (6%). The digests saved run inside the valuation jobs, which run concurrently, so the wall-clock gain is smaller than the work saved. The one digest left (incontext) runs on the actor, as the request id did before.snapshot/value_*benches make their context once, outside the loop. A valuation there hashed it seven times on a miss and three on a hit, and now hashes it none. Sovalue_cachedfalls from 5.0 to 1.0 µs, andvalue_uncachedis 9–11 µs belowmain. P11b-ii's unexplained 4–6 µsvalue_uncachedregression no longer shows. On the Live path a context is made for each valued bond, so eight digests become one.verify,curve_buildandsnapshot_assembledo not touch these paths, and are unchanged.live_tickcompleted in all four runs.Not done here
RuntimeCheckpoint::validateserializes every result subject, so a stored Captured or Replay commit is O(N) in the bonds. A Live tick stores only its reservation, which holds no results, so this is not on the Live path.Verification
WSL,
--locked:cargo fmt --check;cargo clippy --all-targets -- -D warningsforconvex-ports,convex-engine,convex-server,convex-ext-redb,convex-ext-file;RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps --all-featuresforconvex-portsandconvex-engine.cargo test -p convex-ports: 43 passed.cargo test -p convex-engine: 208 passed, 1 ignored (one new test).cargo test -p convex-ext-redb -p convex-ext-file -p convex-server -- --test-threads=1: 353 passed.tools/assurance/check.py, alone and with--base d90c5ef1: passed.Summary by CodeRabbit
Performance
Reliability