PR: perf: serve read_state from the in-memory record maps #173 - #180
Conversation
metasmile
left a comment
metasmile
left a comment
There was a problem hiding this comment.
Six negative findings, no blockers asserted; the merge decision stays with the author. The most actionable are the filtered/scan sort-before-filter regression (store.rs:1670) and the scheduler's silent protocol-mismatch degradation (main.rs:151).
| let Ok(state) = client.read_state().await else { continue; }; | ||
| // Structured read only: the heartbeat check needs ids, | ||
| // workers, and timestamps, not content materialization. | ||
| let Ok(state) = client.read_state_struct().await else { continue; }; |
There was a problem hiding this comment.
else { continue } swallows the RPC error. In a mixed deployment (new nexd against an older nex-server that lacks read_state_struct), every tick fails with method-not-found and the scheduler silently stops releasing stale heartbeats, with no log. At minimum the error should be logged at warn level; a fallback to read_state or a protocol version check would be more robust.
| let mut fact_recs: Vec<(&String, &FactRecord)> = recs.iter().collect(); | ||
| fact_recs.sort_by(|a, b| a.0.cmp(b.0)); | ||
| for (id, r) in fact_recs { | ||
| let content_hash = Self::hex_blob_hash(&r.blob_hash).unwrap_or(FihHash([0u8; 32])); |
There was a problem hiding this comment.
An unparseable blob hash now silently yields FihHash([0u8; 32]) together with empty content. The pre-#173 read_state recomputed SHA-256 from the loaded content, which self-healed legacy or corrupt records. This change silently degrades such records to a zero hash and empty payload with no log and no error path. If the zero fallback is intentional, it should at least be logged once per offending record.
| .or_insert(0); | ||
| let (new_failures, should_respawn) = respawn_decision(*failures, rapid); | ||
| *failures = new_failures; | ||
| if should_respawn { |
There was a problem hiding this comment.
Once the circuit breaker trips (5 rapid exits), respawn stays disabled for the whole nexd session: the counter only resets when a child survives the rapid-exit window, but after tripping there is no child left to survive. If the crash cause is transient and gets fixed while nexd keeps running, nex-server stays down until nexd restarts. A manual reset path (an RPC) or a periodic decay would avoid the latch.
| for (id, r) in fact_recs.iter() { | ||
| let recs = self.fact_records.borrow(); | ||
| let mut fact_recs: Vec<(&String, &FactRecord)> = recs.iter().collect(); | ||
| fact_recs.sort_by(|a, b| a.0.cmp(b.0)); |
There was a problem hiding this comment.
Sorting the full record map before applying the predicates adds O(N log N) to every filtered read, even when the filter matches a handful of records. rem's search path goes through read_state_filtered; a selective origin+creator+time query over a large store now pays the full-map sort. The sort should apply to the matching subset (filter first, then sort), keeping selective queries at O(N) scan plus O(k log k) sort.
| state.intents[idx].description = String::from_utf8_lossy(&content.data).to_string(); | ||
| } | ||
| for (hash, content) in loaded { | ||
| self.cache_blob(hash, content); |
There was a problem hiding this comment.
The FIFO cap at 10,000 entries makes workloads with a larger distinct-blob working set thrash: every read evicts the oldest entry and reloads it from io, which is precisely the cost the cache was introduced to remove, plus cache-maintenance overhead. The stress and cache tests stay under the cap; there is no measurement for the greater-than-10k regime. Consider documenting the expected crossover or making the cap configurable.
| // stays small, so cache misses below are limited to genuinely | ||
| // new blobs. A failed flush is logged: the signature has no | ||
| // error channel. | ||
| if let Err(e) = self.flush_pending().await { |
There was a problem hiding this comment.
Restoring the flush inside read_state makes a read API perform io writes: every read_state on a non-auto-flush store flushes the pending batch (write amplification for high-frequency readers) and a failed flush degrades the read with only a warn log. This is the pre-#173 behavior, but it should be stated explicitly as a deliberate tradeoff, or moved to the session boundary now that the returned state no longer depends on io being durable.
metasmile
left a comment
metasmile
left a comment
There was a problem hiding this comment.
Final review after the fix commit fdf384ab. All six negative findings from the previous review are addressed:
- Filter-before-sort (store.rs): the
sorted_matcheshelper filters first and sorts only the matching subset, so selective queries pay O(N) scan plus O(k log k) sort instead of a full-map sort. The id-sorted result order still matchesread_state(read_state_agreement passes). - Zero-hash fallback (store.rs):
blob_hash_or_zerologs a warning when a persisted blob hash is unparseable instead of failing silently. - Scheduler silent degradation (main.rs): transition-based logging logs the first connect/
read_state_structfailure with its cause and the recovery, without per-tick spam. - Cache thrash (store.rs): the cap is now a per-store field with
set_blob_cache_cap; workloads with a distinct-blob working set over the default 10k can tune it. - Circuit breaker latch (manager.rs): a tripped command re-arms after
RESPAWN_COOLDOWN(60 s), covered by the extendedrespawn_decisionunit tests. - read_state flush side effect (store.rs): documented as a deliberate tradeoff, including the cost of the alternative (session-boundary flush would let pending grow unbounded).
Verification: workspace 417, nexd integration 28, manager 4; run.sh --core and run.sh --server pass; fmt and clippy clean.
Residual non-blocking observations:
blob_hash_or_zerowarns per enumeration call per corrupt record; repeatedly reading a corrupt store logs repeatedly.set_blob_cache_capto a smaller value reclaims memory lazily on the next insert, not immediately.respawn_tripped_atentries persist for commands no longer tracked; bounded by distinct command count.- The default
run.sh(wasm apps + playbooks) was not run; those areas are unaffected by this PR.
No blockers remain.
1 participant
Summary
Re-expresses
read_stateover the in-memory application-layer record maps instead of an O(N) io scan (list + read + decode + SHA-256 per record), with a bounded content cache and a structure-only read for content-free consumers.Primary issue: #173
read_statenow serves from the record maps in id-sorted order (the pre-Re-express read_state over the in-memory 19-axis store (last open constraint of #170) #173 observable ordering contract is preserved), removing the per-call io record scan and the SHA-256 recompute (content_hash parses from blob_hash).blob_cache, FIFO-bounded at 10,000 entries) serves repeated reads from memory; evicted blobs reload from io, and the current read always resolves its own content (no eviction starvation).read_state_structreturns the record structure without blob-payload materialization (facts' content and intents' descriptions empty, hints' inline content present). Wired through the protocol (read_state_structRPC),NexClient, and the nexd scheduler, whose 100 ms heartbeat poll needs ids, workers, and timestamps only.read_state_filteredandscan_partitionenumerate the record maps in id order to matchread_state(identical states including order), covered bystorage/sim/tests/read_state_agreement.rs.read_stateso the pending batch stays small and cache misses stay limited to genuinely new blobs.Bundled in the same branch per the working instruction
ProcessManager::spawn_managedmarks the supervised nex-server as respawnable;try_reaprestarts it after a crash with a rapid-exit circuit breaker; shutdown sends SIGTERM with a bounded poll (short locks, no blocking hold) before force kill; reap polling is 1 s. Covered bytest_nex_server_restarts_after_crashand the extractedrespawn_decisionunit tests.docs/wire-protocol.mdformalizes the JSON-RPC contract (transport, methods, error codes,jsonrpcfield elision);nex-server/client/examples/two_agents.rsdemonstrates two agents sharing a blackboard through independent NexClient connections (verified live). refactor(nexd): replace proc-daemon dependency with lightweight internal rt.rs #137 was closed as not planned: the vendored daemon is retained.Verification
cargo test --workspace: 416 passedcargo test -p nexd --test integration -- --test-threads=1: 28 passed (includes SIGTERM graceful shutdown and the restart-on-crash supervision test)cargo fmt --checkandcargo clippy --all-targetsclean (pre-existingEntityStoreunused-import warnings in two storage/sim tests remain)a_stress_parallelat 10k events: 17.7 s before this work to ~9.0 s (stable across repeated runs); 5k events at 1.96 sNotes for review
read_state_structuretoread_state_structrename and a smallnex-server/client/src/lib.rscleanup (Update lib.rs).