perf(retain): store-owned backend writes facts once with entities inline - #3496
Open
nicoloboschi wants to merge 1 commit into
Open
perf(retain): store-owned backend writes facts once with entities inline#3496nicoloboschi wants to merge 1 commit into
nicoloboschi 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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
For a memories store that owns its rows (external backend), the connection-free store phase of retain writes each memory twice:
insert_factsstages it without entities, thenrecord_unit_entitiesreads the just-written records back (full vectors) and re-upserts them with entity ids — because entity ids resolve onto real unit ids only after those ids 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 — which is the dominant cost of retain (measured ~2.9 s p50 / ~11 s p99 per write-group under load).
Change
Use the deferred single-write path the provider already supports:
insert_facts_batch(defer_index=True)— mint unit ids, no writeindex_facts(..., unit_entity_ids=…, txn=ext_txn)— one write with entities inline, tagged with the write-group txn (commits atomically with the group; postings are now witness-covered instead of riding an uncovered seam)Co-occurrence accumulation still runs —
record_unit_entity_postingsgainsstore_write=False(co-occurrence only, since the row is already correct).Scope: streaming ext path only; delta path unchanged; no-op for the Postgres store.
Effect
Store-owned retain object-store round-trips per doc ~6 → ~4 (removes a full-vector GET + a re-upsert). Targets the retain write-group latency directly.
Tests
test_retain_ext_writegroup.pyupdated + green: the single write viaindex_factsis asserted connection-free and txn-tagged; the posting runsstore_write=False; delta path stillstore_write=True.