The module-cache record is a third of its size and writes twice as fast: per-record tables for strings, types, files, modules and expression classes, delta-coded LineInfo, adaptive-size integers, an inline write path - #4021
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The review identified a few correctness issues in newly-added/modified code paths (test census value truncation, potential empty-string read UB, and flush behavior when writePos==0) that should be fixed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates daScript’s module-cache serialization format (version bump to 211) to significantly reduce record size and improve write performance by introducing per-record dedup tables (strings/types/files/modules/expression classes), delta-coding LineInfo, and adaptive-size integer encoding, plus an optional compile-time serialization profiling rail.
Changes:
- Redesign module-cache record encoding/decoding with per-record tables, varint/zigzag for 32-bit ints, and delta-coded
LineInfo; add explicitflush()points to ensure byte-exact buffers. - Add serialization profiling infrastructure (
DAS_SERIALIZE_PROFILE) and expose deserializer phase timings via RTTI (ast_serializer_finalize_usec,ast_serializer_setup_usec), with documentation updates. - Extend/retune module-cache tests and review checklists to cover the new invariants (record size ballast, position census, flush requirements, string lifetime rules).
File summaries
| File | Description |
|---|---|
| tests/REVIEW.md | Updates checklist routing language for module resolution/loading/cache tests. |
| tests/module_cache/test_rtti_serializer.das | Adds RTTI serializer round-trip test with phase-timer assertions. |
| tests/module_cache/test_default_cache_path.das | Retunes eviction test fixture by adding ballast requires and clarifying assertion text. |
| tests/module_cache/REVIEW.md | Refines module-cache test checklist wording (assertion literals, helper comparison fields, LINT030 policy). |
| tests-cpp/small/test_env_serializer.cpp | Adds a position census to validate LineInfo/type at stability across cold vs warm cache runs. |
| src/misc/REVIEW.md | Extends review rules around float classification and adds a flush-before-buffer handoff rule for C API serialization. |
| src/misc/daScriptC.cpp | Flushes serialization storage before returning pointer/size to callers. |
| src/builtin/REVIEW.md | Updates and expands serializer-related review rules (version bump, string lifetime, per-record table clearing, flush discipline). |
| src/builtin/module_builtin_rtti.cpp | Exposes new RTTI externs for deserializer finalize/setup timing accessors. |
| src/builtin/module_builtin_ast_serialize.cpp | Implements new encoding: per-record tables, varint decode fast-path, delta-coded LineInfo, profiling frames, and added flushes. |
| src/builtin/ARCHITECTURE.md | Documents the new module-cache record byte encoding and the profiling rail. |
| src/ast/REVIEW.md | Adjusts checklist text and adds a rule about serializeTemp for header-read string locals. |
| src/ast/ast_parse.cpp | Adds finalize-time accounting and per-record profiling rows; uses serializeTemp for header string reads. |
| src/ast/ARCHITECTURE.md | Documents the decode/finalize/setup timing split and how it’s reported. |
| skills/internal/build_and_debug.md | Adds instructions for enabling and interpreting serialization profiling output. |
| skills/daslang/references/everything.md | Documents new RTTI functions for finalize/setup microsecond counters. |
| include/daScript/ast/REVIEW.md | Clarifies checklist phrasing around bindings/field offsets and JIT cache versioning. |
| include/daScript/ast/ast_serializer.h | Adds flush()/asVector(), inline append path, per-record table state, profiling structs/macros, and bumps serializer version to 211. |
| doc/source/stdlib/handmade/function-rtti-ast_serializer_setup_usec-0xb312e3fc052a000e.rst | Adds stdlib doc stub for ast_serializer_setup_usec. |
| doc/source/stdlib/handmade/function-rtti-ast_serializer_finalize_usec-0x4aa223d6ebfe0792.rst | Adds stdlib doc stub for ast_serializer_finalize_usec. |
| doc/reflections/das2rst.das | Ensures RTTI docs generator groups the two new RTTI functions. |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
25c02f9 to
24d0a8b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new position census test currently packs LineInfo fields into a single 64-bit word without masking (risking collisions for larger columns/lines), and there is a verified typo in a new/modified diagnostic string.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
tests-cpp/small/test_env_serializer.cpp:56
- PositionCensus::record packs line/column ranges into a single 64-bit word without masking, so if any field exceeds its implicit bit budget (e.g., column/last_column >= 1024) the values overlap and the census can miss real mismatches. Store full 32-bit fields (or mask+assert) so the test reliably detects LineInfo corruption.
void record ( const LineInfo & at ) {
positions.push_back(uint64_t(at.line) << 40 | uint64_t(at.last_line) << 20 | uint64_t(at.column) << 10 | uint64_t(at.last_column));
positions.push_back(uint64_t(hash_blockz64((const uint8_t *) (at.fileInfo ? at.fileInfo->name.c_str() : ""))));
}
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
24d0a8b to
8daac72
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the core module-cache serialization format and timing/accounting across multiple subsystems, requiring careful human verification of compatibility and correctness beyond automated review.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 3
- Review effort level: Lite
…st: strings, types, files, modules and expression classes stream as per-record first-mention numbers, a LineInfo is coded against the previous one, 32-bit integers are adaptive sizes, and the writer appends through an inline path. A record of the JIT module graph (hello_world under -jit, 30 modules) goes from 9.2 MB to 2.4 MB, its write from 37 ms to 16 ms and its decode from 26 ms to 19 ms; the CI ser/deser sweep's stream from 265 MB to 133 MB. The string table holds pointers to strings that live in process memory for the record - the AST's own fields, which nothing changes while a record is written or read - so neither side copies them and neither side looks into the storage; a string with no home past the call (a computed mangled name, a lookup key, a map element, the policies local, the record header's locals) goes through serializeTemp, which parks one copy in the record's arena. The type table keys the live TypeDecl and hashes and compares its content recursively; a type carrying expressions streams inline; a type takes its number after its payload, because the payload can carry a same-module structure inline whose field holds an equal type, and the writer counts fresh entries apart from the map, because such an equal type registered from inside the payload would otherwise skip a number the reader pushes; the reader keeps the first decoded instance and deep-copies it per reference, so no two nodes share a type. The two findAlias cache bits are neither in the key nor in the stream. A FileInfo reference is a per-record number rather than an 8-byte stream offset, so a record never points into another and the skipped-record back-reference check is gone. A LineInfo is a shape byte (same file, one line, a six-bit line delta with an escape), the column and a zigzag span. The adaptive size is seven bits a byte. A Module and an expression class are per-record numbers; every hand-written module hash site goes through the one Module operator. The vector storage grows by doubling behind a write cursor and flush() trims it - consumers call it - and reading marks the finalizeModule and program-setup times apart from the decode: ast_serializer_finalize_usec / ast_serializer_setup_usec on the das handle, and -log-compile-time's cache line prints decode against finalize. DAS_SERIALIZE_PROFILE, off by default, is the size and time breakdown rail: every compound serialize opens a frame, and ModuleFileCache::finish prints self and inclusive bytes per frame, the frame tree, the string and type histograms and per-record sizes. Version 211. The module-cache eviction test's fixture requires strings_boost, fio and ast so a record sizes into the band its 1 MB limit needs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8daac72 to
def19b1
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The changes substantially alter a core serialization format and its performance-critical implementation, so a final human review is warranted despite added tests and documentation.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rebuild required: the module-cache stream format changes (version 211) - every binary that includes
ast_serializer.h(daslang,daslang-live, the C API, external modules) must be rebuilt together; older cache records are rejected and rewritten on first run.Why. A module-cache record was a third strings, a fifth 8-byte file offsets and two fifths type payloads, most of them repeats: the JIT module graph behind
hello_world -jittook 9.2 MB and 37 ms to write.What changes.
flush()trims it, and every consumer calls it.-log-compile-timecache line.DAS_SERIALIZE_PROFILE, off by default, prints a size and time breakdown of a stream per frame, with string and type histograms and per-record sizes.Observable behavior.
Where to look.
src/builtin/ARCHITECTURE.mdsections 6 and 7 carry the encoding; the type table's numbering after the payload and the fresh-entry count apart from the map inoperator<<(TypeDeclPtr&)are the two spots that bit during development, both pinned by the warm AOT generator test.Validation, claims, ledger
Validation
--serthen--deserovertests/) run locally: 1404 programs, 12650 tests, 0 failed, ondaslanganddaslang-liverebuilt from this tip.bin/daslang, not the PATH binary.Claims - stated, not tested
fixedDimExprortypeMacroExprstreams inline (number 1); no fixture counts how often the sweep reaches that arm.trySerializeProgramModulego throughserializeTemp; a plain read there is a use-after-free only an ASAN lane would show.ignoreEmptyExternaland mentioned again outside it fails the record; no fixture produces that order.Not done
DAS_SERIALIZE_PROFILE=1; the rail is compile-time and no lane defines it.typeTableableintoTypeDecl::isExprType(different question;isExprTypedrives inference), the first-mention skeleton into one template (three payload hooks on a hot path) andsameTypeContentintoisSameExactType(the serializer's relation countsmodule, inference's must not).src/miscjob_que wording,tests/REVIEW.mddasbind probe wording,tests/REVIEW.mdopening block) and the lint candidates the audits named: aREVIEW.dascheck that every per-record table clears inclearNodeIds, a token scan for the libc float classifiers inluau_float2string.cpp, aflush()-before-buffercheck indaScriptC.cpp.