Repository navigation
perf: fresh-entity tx memo + cached schema/attr fast paths - #27
Merged
Merged
Conversation
Within apply_tx, entities allocated inside the transaction (e > the pre-tx max_eid) cannot exist in the pre-tx index, so their same-fact and entity/attr lookups are answered from a per-tx table instead of descending the index trees. Bulk seed transactions (imports) consist almost entirely of such datoms.
schema_attr_is_avet_accessible ran a List.assoc_opt scan of the whole schema assoc list (twice) for every datom inserted into the indexes. Bulk imports pay that ~500k times; each schema's accessible-attr set is now computed once and cached by schema-list physical identity, the same staleness check Schema_access.schema_attr already uses.
The (ns, name) pairwise order equals a byte compare on a normalized key (last '/' replaced by NUL, or NUL prepended when there is none), so compare_attr now resolves each attr to a memoized key once and runs a single String.compare instead of two rindex scans plus two per-char slice loops on every index comparator call.
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Devin resolved all 3 findings on f21006a
Fixed by Devin (3)
- Existing transaction entity retains old value
- Caught nested transaction corrupts outer writes
- Keyword comparisons retain every distinct value
View all findings in Devin Review
…tr keys - Memo freshness now requires e <= max_allocatable_entity_id: datoms on transaction entities (e >= tx0) can already carry index datoms from earlier transactions, so routing them through the empty memo bucket dropped the retraction a cardinality-one update needs. - apply_tx saves/restores the shared memo record (Fun.protect): a nested transaction via Call, or an exception raised mid-tx, no longer leaks this transaction's state into the outer one — the outer transaction also keeps its memo instead of losing it to the inner reset. - attr_key_tbl is capped (65536 entries): keyword values flow through compare_attr too, so an unbounded vocabulary would grow the table without bound; past the cap keys are recomputed per call. - Regression test: cardinality-one update on a past tx entity.
The memo table kept every datom of the transaction that just ran as dead state until the next transaction reset it — ~2.5MB retained heap in the memory benchmark (js_of_ocaml initial-open: 10.5MB -> 8.1MB).
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.
Summary
Three related transact/index fast paths, measured on a ~500k-datom bulk import (Logseq 4k-movies graph,
out_cli load-dir): the in-transact indexing phase drops from ~12.7s to ~6.4s.Transact.tx_memo— fresh-entity lookup memo. During a transaction, datoms for entities allocated inside that transaction (d.e > base_max_eid, i.e. they cannot exist in the pre-tx index) are recorded in a(e, a) -> datom listhashtable populated through therefresh_db_indexes_with_tx_datafunnel.add_active_datom_with_report_dbanswers its same-fact (find_eavt_exact) and cardinality-one (entity_attr_datoms_db) lookups from the memo instead of descending the index trees — two index probes perAddavoided. Import-style transactions consist almost entirely of fresh entities. Dedup equality usescompare_valueto match index semantics.Schema.schema_attr_is_avet_accessible— per-schema cached set. It ran twoList.assoc_optscans of the schema assoc list per call, once per index datom insert (~500k calls on import). The accessible-attr set is now computed once per schema and cached, keyed by the schema assoc list's physical identity (same staleness checkSchema_access.schema_attralready uses).Util.compare_attr— memoized normalized key. The upstream(ns, name)pairwise order is equivalent to a byte compare on a normalized key: last'/'replaced by'\x00', or'\x00'prepended when the attr has no'/'('\x00'sorts below every legal attr byte and below'/', so a shorter/prefix ns and a missing ns both order correctly). Keys are memoized in a content-keyed hashtable, so an attr compare is a==/String.equalfast path or a singleString.compare— this is the largest win sincecompare_attrleads everyaevt/avetindex comparator call.compare_slice(only used by the oldcompare_attr) is removed.All impl test exes pass (
test_datascript,test_upsert,test_db,test_storage,test_pull_*,test_lookup_refs,test_serialize, etc. — everything except thelein-dependent upstream JS bundle rule and thecross_runtimedriver that needs--input).Link to Devin session: https://app.devin.ai/sessions/87d07eb19d674693adeffdb53332c183
Open in Devin Desktop: https://app.devin.ai/desktop/session/87d07eb19d674693adeffdb53332c183?variant=devin
Requested by: @tiensonqin