chainbase: skip post_modify walk for non-key modifies#300
Open
heifner wants to merge 2 commits into
Open
Conversation
The original benchmark stack-allocated the undo_index. offset_node_base packs tree pointers as signed 42-bit offsets (<<2 => 8 TiB reach) relative to each node, so when the index header lives outside the segment the nodes live in, the node-to-header distance can exceed 8 TiB and the packed offset truncates, producing bogus parent pointers that SIGSEGV during AVL rebalancing. Effect was ASLR-dependent, showed up as ~50% crash rate. Mirror libraries/chaindb/test/undo_index.cpp's `undo_index_in_segment` wrapper so the index (and its header) lives alongside its nodes. Also split the driver into three scenarios targeted at specific optimization candidates, switch pinnable_mapped_file to heap mode with an 8 GiB region, and give stopwatch a label so per-scenario timings print meaningfully.
post_modify<N>() was called for every modify() on every secondary index,
doing iterator_to + two neighbor comparisons even when the modified
object's key for that index hadn't moved. For tables where modifies
usually touch non-indexed fields (account recv_sequence, resource
state, etc.), that's pure overhead.
Snapshot the secondary-index keys in modify() before invoking the
modifier, then in post_modify compare each index's post-modify key to
the saved pre-key; if unchanged, skip the fixup for that index
entirely.
The fast path is only engaged when it is provably safe: the index's
key extractor is a plain `boost::multi_index::member<T, F, PMF>` AND
the field type F is trivially copyable AND the copy is value-owning
(checked via an `is_shallow_copy` blacklist for pointers, pointer-to-
member, `std::reference_wrapper`, `std::string_view`, `std::span`,
and recursively through `std::array`, C arrays `T[N]`, `std::optional`,
and `std::variant`). This rules out `composite_key<...>` (whose
`composite_key_result` holds a reference to the source value, making
the "snapshot" alias the live node) and any extractor returning a
view, reference wrapper, raw pointer, or an aggregate containing one.
Ineligible indexes keep the original iterator_to + neighbor-compare
full walk.
The revert path still uses the original key-less post_modify because
after `node_ref = std::move(*backup)` the tree position may be wrong
while the keys match pre_keys.
Belt-and-suspenders: the trait and the `fast_path_eligible_v` alias
live in `chainbase::detail` so external call sites can static_assert
their own extractor types against it; a Debug-only (NDEBUG==0) runtime
assertion inside the fast path re-verifies the tree-local sort
invariant immediately after every skip, so any aliasing bug that
slipped past the trait would fire in CI.
Measured on chainbase_bench (heap mode, Release, 3 runs each):
scenario_original 23.25 -> 18.96 s (-18%)
scenario_two_idx_nonkey_modify 16.66 -> 12.74 s (-23%)
scenario_undo_session_churn 2.53 -> 2.52 s (unchanged; 1 idx)
Test coverage:
- libraries/chaindb/test/undo_index.cpp
- test_composite_key_modify: mutating a composite-key component
correctly reorders the secondary index (regresses with an
unconditional fast path).
- test_composite_key_modify_undo: under an undo session, modifying
a composite-key field stays sorted and undo restores ordering.
- test_mixed_member_and_composite: fast-path-eligible plain member
index coexists with a composite-key index; modifying a
non-indexed field touches neither, modifying the composite key
reorders only the composite index, modifying the plain member
reorders only that index.
- test_composite_key_modify_uniqueness_conflict: revert on
uniqueness violation under composite_key works correctly.
- unittests/chainbase_fast_path_tests.cpp: compile-time static_assert
that every chainbase-backed production secondary member<> extractor
(account_object.by_name, account_metadata_object.by_name,
resource_object.by_owner, resource_pending_object.by_owner) passes
fast_path_eligible_v. Future table changes that swap a trivially-
copyable field for an aliasing or non-trivial type fail the build.
chainbase_test, unit_test (sys-vm), plugin_test, contracts_unit_test
(sys-vm), test_fc all pass.
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
1 participant
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
chainbase::undo_index::modify()that skips the per-secondary-indexiterator_to+ neighbor-compare + fixup walk when the modifier didn't change that index's key.modify()snapshots secondary keys before calling the modifier;post_modifycompares post-modify key to pre-key; if equivalent under the index's comparator, the whole fixup for that index is skipped.boost::multi_index::member<T,F,PMF>ANDFis trivially copyable ANDFis not a shallow/aliasing type (is_shallow_copyblacklists pointers,reference_wrapper,string_view,span,array, C arrays,optional,variantand their recursive combinations).composite_key<...>and any extractor whose return captures a reference to the source falls through to the original full-walk. The revert path uses the original key-less post_modify unchanged.libraries/chaindb/benchmark/bench.cppused to stack-allocate the undo_index, butoffset_node_basepacks tree pointers as 42-bit self-relative offsets (+/-8 TiB). Stack-header vs heap-mapped-node distance can exceed that on some ASLR placements and SIGSEGVs in rebalance. Mirrorlibraries/chaindb/test/undo_index.cpp'sundo_index_in_segmentwrapper so the index sits next to its nodes. Also splits the driver into three scenarios for targeted measurement.Measurements
chainbase_bench (heap mode, Release, 3 runs each):
two_idx is the direct target (non-key modify across secondary indexes). Note: microbench only; real nodeop block-apply impact not yet measured.
Defense in depth
Three layers guard against the composite_key-style aliasing bug class (previously discovered and fixed during development of this PR):
chainbase::detail::fast_path_eligible_v+is_shallow_copy; static_asserts fingerprint the trait itself.unittests/chainbase_fast_path_tests.cpp): static_asserts against every chainbase-backed production secondarymember<>extractor. Any future table change that replaces a trivially-copyable field with an aliasing/non-trivial type fails the build.assertonCI only, elided in Release): after every fast-path skip, re-verify the tree-local sort order around the modified node. Any trait regression or unforeseen aliasing corrupts ordering and asserts on the next modify. Theassertonplatform at.cicd/platforms/asserton.Dockerfiledrops-DNDEBUGexplicitly, so this assert runs in every CI matrix cell that uses it.