perf(startpos): rewrite slots with 88% less allocation and 8% smaller files - #145
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Akron now reconstructs fixed real-map StartPos slots with lower allocation, snapshot size, working time, and peak RSS while preserving v10 format and slot compatibility.
WalkthroughThe reconstruction tests now verify that serialization and deserialization reject excessive parent-chain depth and diagnostic-path size. They also verify deserialization errors for missing field values and delegate-call targets. The changelog records reduced StartPos memory use and output size while preserving saved-state and slot compatibility. Merge Risk: ⚪ Minimal · up to This localized persistence optimization preserves the existing saved-state format, compatibility, validation limits, and fail-before-commit behavior while reducing allocation and snapshot size. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
Full details: Documentation ImpactExplanation No undocumented public contract change is introduced. The diff keeps 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 |
|
@codex review |
Add a manifest-verified corpus collector and a resource-isolated benchmark for 11 vanilla and 12 modded StartPos snapshots. The modded cohorts cover Spring Collab Cookie, Strawberry Jam Hyperlife, and Monika's D-Sides. The companion Kubernetes manifests define long-lived collection and guaranteed-resource grading pods backed by the corpus PVC. They contain no corpus payloads, credentials, secrets, or host paths. The reward is 75% weighted allocation and 25% compressed size; allocation weights corpus rewrites 9:1 over deterministic synthetic capture. Working time, CPU time, and peak RSS remain diagnostic metrics rather than optimization targets. Every run hard-fails unless all 23 inputs retain their manifest identity, exact 1,151,644,947-byte decompressed total, bounded readability, document shape, and synthetic exact restore. Per-cohort allocation, synthetic allocation, and compressed size have independent regression gates, so the scalar reward cannot buy a localized regression. The benchmark is opt-in: ordinary CI returns a passing no-op when neither corpus variable is configured; partial configuration fails loudly; scripts/akron-corpus/autoresearch.sh supplies both and also refuses a missing result. Calibrated baseline: 110,513,612,600 corpus-allocated bytes, 28,478,472 synthetic-allocated bytes, 40,304,236 compressed bytes, 220.103 seconds working time, 223.660 seconds CPU time, and 3,313,205,248 bytes peak RSS. No production behavior changes.
Make optional reconstruction-node collections lazy and traverse absent collections without allocating. Preserve the model's mutable public list behavior with lazy getters while routing production traversal through allocation-free *OrNull backings. Replace bounded-reader JsonReader.Path construction with ordinal array-element matching and depth-tracked recognition of the few record arrays that affect structural ceilings. The reader still enforces the same token, container, record, node, and expensive-record limits; it derives record context from token/depth state instead of materializing full JSON paths. Snapshot bytes and the v10 contract are unchanged. Canonical reader checkpoint versus baseline: - overall cost: 0.473838 (-52.6%) - corpus allocation: 25,832,071,112 bytes (-76.6%) - synthetic allocation: 25,083,800 bytes (-11.9%) - working time: 136.83 seconds (-37.8%) - compressed corpus: unchanged at 40,304,236 bytes The lazy mutable getters measured corpus allocation flat with exact bytes unchanged. Peak RSS was 1.066x baseline at the reader checkpoint; later pooling and scratch reuse lower it. All 23 snapshots passed.
Stop proving a staged snapshot by deserializing a second complete reconstruction graph that is immediately discarded. Validate the indexed in-memory document before write, stream the staged JSON through the bounded reader, and require its exact uncompressed SHA-256 to match the bytes handed to gzip. The stream still enforces format, type, token, string, container, record, node, and expensive-record ceilings. Header and graph invariants are checked against the exact document before serialization, while truncation, corruption, malformed JSON, and structural-limit failures still abort before commit. Keep optional delegate parameter metadata and non-array path indices nullable, copying them only when present. The final correctness commit restores derived diagnostic-path preflight, which a token stream cannot infer by itself; this stack should be merged as a unit. Canonical group-tip change versus the preceding reader checkpoint: - overall cost: 0.410675 (-13.3%) - corpus allocation: 20,375,773,616 bytes (-21.1%; -81.6% from baseline) - synthetic allocation: 13,754,232 bytes (-45.2%; -51.7% from baseline) - working time: 112.34 seconds (-17.9%; -49.0% from baseline) - compressed corpus: unchanged at 40,304,236 bytes All 23 snapshots and the focused size/structural guards passed.
Walk field, item, and delegate reachability directly instead of composing per-node LINQ iterators. Build one pre-sized node index during header validation and reuse it for parent edges, reachability, and diagnostic-path restoration; pre-size the remaining scratch collections from known graph counts. Validate serialized type-name indexes directly after the one full graph validation and reuse an already canonical type table. Keep nested default values, delegate targets, and gameplay payloads nullable rather than constructing throwaway objects. Make diagnostic paths cheap without weakening their limits: share exact child/field paths, reuse unresolved-node scratch, build field/array/delegate paths without intermediate strings, and validate path lengths and parent links eagerly while materializing strings only when observed. Canonical group-tip change versus streaming readback: - overall cost: 0.384396 (-6.4%) - corpus allocation: 16,732,912,096 bytes (-17.9%; -84.9% from baseline) - synthetic allocation: 12,224,312 bytes (-11.1%; -57.1% from baseline) - working time: 97.43 seconds (-13.3%; -55.7% from baseline) - peak RSS: 0.924x baseline Snapshot bytes and diagnostic-path accounting are unchanged. All 23 snapshots and the focused graph, path, header, action-state, delegate, and weak-reference guards passed.
Reuse per-graph parent-edge, reachability, serialization-index, and root-node validation scratch across sequential passes. Clear every reusable collection on exit and keep returned deserialize indexes independent so scratch lifetime does not leak into loaded documents. Prepopulate one per-graph Json.NET property-name table with the known v10 and auxiliary keys, interning repeated property tokens without retaining unknown names. Back Json.NET reader and writer character buffers with ArrayPool<char>.Shared through a stateless adapter. Canonical group-tip change versus the validation checkpoint: - overall cost: 0.358750 (-6.7%) - corpus allocation: 12,902,315,064 bytes (-22.9%; -88.3% from baseline) - synthetic allocation: 11,370,512 bytes (-7.0%; -60.1% from baseline) - peak RSS: 0.853x baseline (-7.7% versus the preceding checkpoint) - compressed corpus: unchanged at 40,304,236 bytes Exact decompressed bytes, bounded-reader behavior, and all 23 snapshot rewrites are unchanged.
Use one 8-KiB constant for JSON stream reader and writer buffers instead of 64-KiB backing arrays. UTF-8 output and bounded-reader behavior remain unchanged. Measured checkpoints on the canonical stack: - 16 KiB versus 64 KiB reduced synthetic allocation 3.45%, corpus allocation 0.36%, and peak RSS 6.1% - the final 8-KiB midpoint versus 16 KiB reduced synthetic allocation another 0.60% without hurting corpus balance or throughput All round-trip, exact-byte, and workload guards passed.
Write the existing single gzip member with CompressionLevel.SmallestSize. Pass capture parent-edge metadata as scalars, copy array indices only for newly created nodes, and keep the parent-edge validator static so the denser output does not carry avoidable capture overhead. SmallestSize is the only stored-byte change. At its isolated checkpoint it increased working time 22.3% over the preceding 16-KiB pre-compression candidate, trading some throughput for 3,327,726 fewer compressed bytes; the complete stack remains substantially faster than baseline. Canonical group-tip versus baseline: - overall cost: 0.336458 (-66.4%) - compressed corpus: 36,976,510 bytes (-8.3%) - synthetic allocation: 10,777,360 bytes (-62.2%) - corpus allocation: 12,887,797,520 bytes (-88.3%) - working time: 120.58 seconds (-45.2%) - CPU time: 121.73 seconds (-45.6%) - peak RSS: 0.872x baseline All 23 snapshots retained exactly 1,151,644,947 decompressed bytes.
Streaming staged readback proves byte integrity and enforces every JSON-level ceiling without allocating a second reconstruction graph. Derived diagnostic paths are not serialized, however, so a token stream cannot infer their parent-link, depth, cycle, or aggregate-size constraints. Before serialization, reset only the [JsonIgnore] diagnostic-path caches and rebuild them from the serialized parent edges through RestoreDiagnosticPaths. This restores the loader's exact path preflight on trusted SaveSnapshot input while retaining the same hostile-deserialization checks. Cached paths do not affect wire bytes. Cover both boundaries: trusted Serialize input now refuses oversized derived paths before commit, and hostile Deserialize input still refuses the same document shape. Exact rebased final-tree canonical result versus baseline: - overall cost: 0.336671 (-66.3%) - weighted allocation ratio: 0.143083 (-85.7%) - corpus allocation: 12,909,322,744 bytes (-88.3%) - synthetic allocation: 10,808,272 bytes (-62.0%) - compressed corpus: 36,976,510 bytes (-8.3%) - working time: 123.22 seconds (-44.0%) - CPU time: 124.57 seconds (-44.3%) - peak RSS: 2,955,034,624 bytes (-10.8%) - worst cohort allocation ratio: 0.170689 (-82.9%) - vanilla allocation ratio: 0.131987 (-86.8%) - Spring Collab Cookie allocation ratio: 0.156470 (-84.4%) - Strawberry Jam Hyperlife allocation ratio: 0.170689 (-82.9%) - Monika's D-Sides allocation ratio: 0.109662 (-89.0%) - source compressed bytes: 40,304,236 (unchanged) - decompressed bytes: 1,151,644,947 (unchanged) - snapshot count: 23 (unchanged) All 23 snapshots passed with the exact 1,151,644,947-byte decompressed total.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Require serialized field values and delegate targets so malformed v10 documents cannot turn omitted members into nulls. Cover both properties and keep streaming-ceiling fixtures valid under the stricter contract.
20077bf to
5ecfff1
Compare
|
@coderabbitai review @codex review |
|
|
|
@codex review |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
GitHub Actions
Failure
2 participants
Summary
This is the memory/size follow-up to the v10 work in #138. It changes how Akron builds, validates, and compresses reconstruction documents; it does not move
akron-reconstruction-v10or require players to reset slots.Why
A restart copy walked and serialized one reconstruction document, then proved the staged file by deserializing a second complete document that was immediately discarded. Large documents also paid for empty model collections, repeated
JsonReader.Pathstrings, eager diagnostic paths, fresh validation dictionaries/sets/stacks, repeated property-name strings, and oversized JSON buffers.That work is transient, but it drives cumulative allocation and GC pressure on the persistence worker. The released v10 format made snapshots much smaller on disk; this PR makes reading and rewriting that same format substantially cheaper.
Method
The branch adds a fixed, manifest-verified benchmark over 23 real StartPos snapshots:
The search reward is:
Working time, CPU time, and peak RSS are reported but are not reward terms.
Every candidate hard-fails unless:
The retained stack came from 40 measured candidate segments; discarded candidates are not part of this PR.
The checked-in collector and grader Kubernetes manifests make their public images, PVC, resource limits, and isolation requirements reviewable. They contain no corpus payloads, credentials, secrets, or host paths.
Results
Cumulative results for the exact final tree against the calibrated baseline:
The benchmark also emits workload and invariant metrics that are not all represented by the headline table:
Hyperlife is the worst allocation cohort at 0.1707x baseline; every cohort remains far below its non-regression gate. Source size, decompressed identity, and snapshot count are invariants rather than optimization targets, so their correct delta is zero.
What changed
No second reconstruction graph during staged readback
SaveSnapshotvalidates the exact indexed in-memory document before writing, streams the staged JSON through every bounded-reader structural ceiling, and requires the exact uncompressed SHA-256 to match every byte handed to gzip. It catches truncation, corruption, malformed JSON, and structural-limit failures before commit without materializing a discarded second graph.Less per-record and per-pass allocation
*OrNullbackings while public mutable list getters retain their existing behaviorJsonReader.PathArrayPool<char>.SharedSmaller stored snapshots
The single gzip member uses
CompressionLevel.SmallestSize. This is the only change that alters stored bytes: decompressed v10 JSON and its contract remain unchanged. It saves 3,327,726 bytes across the fixed corpus while the complete rewrite remains about 45% faster than baseline.Exact-state and safety boundaries
The save path still refuses a slot before commit if a later load would reject it.
Streaming JSON validation cannot infer derived diagnostic paths by itself, so serialization resets only
[JsonIgnore]path caches and rebuilds them from the serialized parent edges through the loader's existingRestoreDiagnosticPathsvalidator. Path-size, aggregate-size, unresolved-parent-depth, and cycle limits are therefore enforced on both trusted serialization and hostile deserialization.The tests cover both directions. The path caches are not serialized, so rebuilding them changes no wire state.
Review map
The eight commits are intentionally grouped by idea. The harness commit records the calibrated baseline, the buffer-size commit records its measured A/B deltas, and every other body records its measured autoresearch checkpoint; the final commit and tables below record the exact rebased tree:
The measured checkpoints are preserved for review, but commit 3 intentionally relies on commit 8 to close derived diagnostic-path preflight. Merge the stack as one unit; squash-merging is also safe.
Suggested review order:
SaveSnapshot,ValidateSerializedDocument, and hash handling: no second graph, same fail-before-commit boundaryPrepareForSerialization,ResetDiagnosticPaths, andRestoreDiagnosticPaths: derived path limits remain symmetric between save and loadAkronBoundedJsonTextReader: same ceilings withoutJsonReader.Path*OrNullaccessors: API compatibilityCompressionLevel.SmallestSize: the only stored-byte changeVerification
Checklist
CHANGELOG.mdrecords the notable memory and size improvement.CONTRIBUTING.md.Checks run
Results:
nobodyThe template's
dotnet formatcommand is not listed as a passing gate: this checkout has no formatter configuration, so the SDK default proposes a whole-file brace-style rewrite that conflicts with the repository's existing K&R style.git diff --checkis clean.Live verification
AI assistance disclosure
agent_name: Oh My Pi coding agentagent_version: model/version metadata not exposed by the harnessmodel_used:llmproxy/codex/gpt-5.6-solhuman_testing: No human-run live Celeste testing was performed for this PR. Automated verification run through the coding harness is listed above; the contributor should add any manual review or live testing they perform before marking the PR ready.contribution_summary: Built a measured StartPos persistence optimization, reviewed the correctness boundary, consolidated the retained changes into a metric-backed commit series, and drafted the PR explanation.