perf(retain): store-owned backend writes facts once with entities inline - #3554
perf(retain): store-owned backend writes facts once with entities inline#3554nicoloboschi wants to merge 1 commit into
Conversation
A memories store that owns its rows (external backend) retains a document in a connection-free store phase. That phase wrote each memory TWICE: insert_facts staged it without entities, then record_unit_entities read the just-written records back (full vectors) and re-upserted them with entity ids attached — because entity ids can only be resolved onto real unit ids after they exist. On an object-store-backed engine that reattach is a second full write per memory plus a read-back, doubling the round-trips on the slow path (the dominant cost of retain). It's avoidable: mint the ids without writing (insert_facts_batch with defer_index), remap the entities, then write once via index_facts with the entity ids already inline — tagged with the write-group txn so it commits atomically with the group (and, as a bonus, the postings are now witness-covered instead of riding an uncovered seam). Co-occurrence accumulation still runs (record_unit_entity_postings gains store_write=False: co-occurrence only, since the row is already correct). Streaming ext path only; the delta path is unchanged. Tests updated: the single write via index_facts is asserted connection-free and txn-tagged, and the posting runs store_write=False. No behavior change for the Postgres store (a no-op there).
|
Superseded by #3560 — this change is already in that branch. #3560 carries the same Both PRs touch the same four files ( Suggest closing this in favour of #3560, which is now retargeted to |
1 participant
A memories store that owns its rows (an external backend,
MemoriesExtension) retains a document in a connection-free store phase. That phase wrote every memory twice:insert_factsstaged the memory without entities, thenrecord_unit_entitiesread the just-written records back — full vectors — and re-upserted them with entity ids attached.The second write exists only because entity ids can only be remapped onto real unit ids once those ids exist. On an object-store-backed engine that reattach is a full extra write per memory plus a read-back, which doubles the round-trips on the slow path — and the slow path is the dominant cost of retain.
It is avoidable. Mint the ids without writing (
insert_facts_batchwithdefer_index), remap the entities against them, then write once viaindex_factswith the entity ids already inline.Two things fall out of it
record_unit_entity_postingsgainsstore_write=Falsefor this path — co-occurrence accumulation still runs (the entity-graph endpoint and resolution's disambiguation signal both need it, and it references onlyentities), but the redundant second store write is skipped because the row is already correct.Scope
Streaming extension path only. The delta path is unchanged and still writes-then-reposts, so its re-post remains a real store write that must ride the write-group — the tests assert both shapes separately, since they achieve group coverage in opposite ways.
No behaviour change for the Postgres store:
store_writeis a no-op there.Tests
tests/test_retain_ext_writegroup.py— the single write viaindex_factsis asserted connection-free and txn-tagged, and the posting is asserted to run withstore_write=Falseon the streaming path andstore_write=True+ the write-group handle on the delta path. All 6 pass.The rest of the suite is unchanged by this; the failures on my machine (
test_fact_extraction_quality.py,test_retain.py::test_*_extraction_mode) reproduce identically on unmodifiedmain— they need a live LLM.🤖 Generated with Claude Code