feat: implement bulk_load API for all ART tree modes (#636)#864
Open
thompsonbry wants to merge 22 commits into
Open
feat: implement bulk_load API for all ART tree modes (#636)#864thompsonbry wants to merge 22 commits into
thompsonbry wants to merge 22 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a ChangesART Bulk-load Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant db as db/olc_db/mutex_db
participant bulk_load_impl
participant subtree_builder
participant inode as create_bulk
Caller->>db: bulk_load(fork, max_tasks, first, last)
db->>db: check tree empty / range non-empty
db->>bulk_load_impl: bulk_load_impl(self, fork, max_tasks, first, last)
bulk_load_impl->>bulk_load_impl: partition_by_byte(range, dispatch_depth)
alt fork provided
bulk_load_impl->>subtree_builder: fork parallel tasks per partition
else sequential
bulk_load_impl->>subtree_builder: build recursively inline
end
subtree_builder->>inode: create_bulk(children, value_mask)
inode-->>subtree_builder: node_ptr
subtree_builder-->>bulk_load_impl: build_result
bulk_load_impl->>db: assign self.root
bulk_load_impl-->>Caller: return (or clear() on exception)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@art_bulk_load_detail.hpp`:
- Around line 182-245: The bulk-inode assembly logic is duplicated between the
root and recursive paths, so refactor the repeated child-count branching, vmask
construction, stats accounting, and create_bulk() calls into a shared helper.
Use a single helper around make_inode / the bulk assembly code to build inode_4,
inode_16, inode_48, and inode_256 consistently, then call it from both paths so
they do not drift. Keep the existing art_policy::can_eliminate_leaf handling and
UNODB_DETAIL_WITH_STATS accounting in that helper.
- Around line 172-175: The prefix chain depth logic is hard-coding the capacity
as 8 instead of using the existing prefix_cap value, so update the
chain_consumed calculation in both the sequential and parallel root assembly
paths to derive consumption from prefix_cap. Make the same change wherever this
pattern appears in art_bulk_load_detail.hpp, including the matching logic around
the other chain assembly block, so key_prefix_capacity changes do not produce
incorrect prefix lengths.
- Around line 18-19: The bulk-load path in art_bulk_load_detail.hpp can read
past the end of a key view when duplicate or terminal keys make
common_prefix_length() equal kv.size(), causing dispatch_depth to reach the end
before indexing kv[dd] in the bulk-load logic. Update the bulk-loading flow
around the detail routine that computes dispatch_depth and accesses kv[dd] to
reject duplicate/terminal keys up front or to handle terminal leaves explicitly
before any indexing occurs, using the existing bulk-load helpers rather than
assuming all inputs are strictly non-terminal and unique.
- Around line 288-310: The async subtree build loop in the builder logic is not
exception-safe, so a throw from futures[i].get() or later in the guard/children
assembly can leak raw node memory held by build_result. Update the future result
handling in this section to catch exceptions, drain any remaining std::future
results into a temporary guard collection before rethrowing, and only then
proceed with constructing bulk_child_t and bulk_subtree_guard state. Use the
existing builder lambda, build_result_t, guard_t, and children assembly as the
anchor points for the fix.
In `@art_internal_impl.hpp`:
- Line 3063: Remove the extra trailing blank lines at the end of each
create_bulk block so the linter stops flagging redundant whitespace; check the
create_bulk-related sections in art_internal_impl.hpp around the identified
spots and trim the final empty line(s) without changing any logic.
In `@benchmark/CMakeLists.txt`:
- Line 13: The *_quick_arg benchmark filter variables in
benchmark/CMakeLists.txt are using escaped quotes, so add_custom_target passes
the quotes literally and Google Benchmark never matches the intended subset.
Update the relevant set(...) definitions, including
micro_benchmark_bulk_load_quick_arg and the other *_quick_arg symbols in this
file, to use unquoted filter values like --benchmark_filter=/1024 so the quick
benchmarks actually run the smaller selection.
In `@benchmark/micro_benchmark_bulk_load.cpp`:
- Around line 28-30: The type alias for kv_mutex_db is inconsistent with kv_db
and kv_olc_db, which already use unodb::benchmark aliases. In
micro_benchmark_bulk_load.cpp, update the kv_mutex_db declaration to use the
existing unodb::benchmark::kv_mutex_db alias if it exists, so the alias style
matches the surrounding declarations; if that alias is not available, leave it
as-is.
In `@examples/example_bulk_load.cpp`:
- Around line 7-26: The example in make_sorted_data uses std::array without
including its defining header, so add the missing <array> include alongside the
other standard headers at the top of example_bulk_load.cpp. Keep the fix
localized to the includes so the dependency on std::array is explicit and no
longer relies on transitive includes from art.hpp or art_common.hpp.
In `@test/CMakeLists.txt`:
- Around line 62-64: The new bulk-load targets are registered via
add_db_test_target, but they are not included in the aggregate coverage and
valgrind lists, so they can bypass those checks. Update the test/CMakeLists.txt
aggregates that define tests_for_coverage and valgrind_tests to include
test_art_bulk_load, test_art_bulk_load_structural, and test_art_bulk_load_ops,
matching the existing pattern used for other db tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3c0f1c0-e50e-4793-a31c-da32e2ef4694
📒 Files selected for processing (13)
art.hppart_bulk_load_detail.hppart_internal_impl.hppbenchmark/CMakeLists.txtbenchmark/micro_benchmark_bulk_load.cppexamples/CMakeLists.txtexamples/example_bulk_load.cppmutex_art.hppolc_art.hpptest/CMakeLists.txttest/test_art_bulk_load.cpptest/test_art_bulk_load_ops.cpptest/test_art_bulk_load_structural.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
art.hpp (1)
352-362: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale
db::bulk_loadexecution-policy docs.Line 357 says the policy is ignored for
db, but Line 2736 forwards it intodetail::bulk_load_impl, and the PR objective/tests expectstd::execution::parto enable bulk construction. Update the comment so users don’t assume the parallel overload is a no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art.hpp` around lines 352 - 362, The db::bulk_load execution-policy comment is stale and incorrectly says the policy is ignored, even though bulk_load forwards the policy into detail::bulk_load_impl and parallel construction is expected. Update the documentation on bulk_load to describe that the execution policy is used, and remove the “ignored for db” wording so the behavior matches the implementation and std::execution::par support.test/test_art_bulk_load_ops.cpp (1)
133-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test does not verify “no growth events.”
The body only checks that all keys are readable after
bulk_load. A regression that reintroduced incremental inode growth would still pass here. Either assert an observable no-growth invariant under stats, or rename/drop this test so it does not claim coverage it does not provide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_art_bulk_load_ops.cpp` around lines 133 - 150, The BulkLoadOps.NoGrowthEvents test currently only verifies that keys can be read after bulk_load and does not actually assert the no-growth invariant it claims. Update the test to check an observable stats-based condition from the bulk_load path (for example via existing growth/shrink counters or inode stats), using the relevant bulk_load and stats APIs, or else rename/remove the test so its name and comments do not overstate coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_art_oom.cpp`:
- Around line 672-676: The OOM regression check is too weak because `empty()`
only verifies the root is unreachable, not that all internal state and
allocations were fully cleaned up after `bulk_load` fails. In the
`std::bad_alloc` catch block, update the assertion to use the stronger
full-empty verification for `test_db` so this test also catches leaked nodes or
stale memory/stat counters, keeping the strong-guarantee check tied to the
`bulk_load` failure path.
---
Outside diff comments:
In `@art.hpp`:
- Around line 352-362: The db::bulk_load execution-policy comment is stale and
incorrectly says the policy is ignored, even though bulk_load forwards the
policy into detail::bulk_load_impl and parallel construction is expected. Update
the documentation on bulk_load to describe that the execution policy is used,
and remove the “ignored for db” wording so the behavior matches the
implementation and std::execution::par support.
In `@test/test_art_bulk_load_ops.cpp`:
- Around line 133-150: The BulkLoadOps.NoGrowthEvents test currently only
verifies that keys can be read after bulk_load and does not actually assert the
no-growth invariant it claims. Update the test to check an observable
stats-based condition from the bulk_load path (for example via existing
growth/shrink counters or inode stats), using the relevant bulk_load and stats
APIs, or else rename/remove the test so its name and comments do not overstate
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf7fa448-aabe-4faf-9d22-0661c79a5745
📒 Files selected for processing (6)
art.hppbenchmark/micro_benchmark_bulk_load.cppolc_art.hpptest/CMakeLists.txttest/test_art_bulk_load_ops.cpptest/test_art_oom.cpp
added 6 commits
June 30, 2026 17:03
Add bulk_load(first, last) and bulk_load(std::execution::par, first, last) to db, mutex_db, and olc_db. Builds optimal-size internal nodes directly from sorted input, avoiding incremental insert overhead. Parallel execution dispatches one std::async task per root-level partition; subtree construction within each partition is sequential. API: tree.bulk_load(first, last); // sequential tree.bulk_load(std::execution::par, first, last); // parallel Performance (96-core, 1M random uint64 keys, Release): db: seq 11M/s, par 67M/s (6x) mutex_db: seq 11M/s, par 68M/s (6x) olc_db: seq 11M/s, par 65M/s (6x) Implementation: - art_bulk_load_detail.hpp: shared recursive algorithm template - art_internal_impl.hpp: create_bulk factories for inode_4/16/48/256 - art.hpp/olc_art.hpp/mutex_art.hpp: thin public API wrappers Tests: 33 new tests (boundary, structural, operational) across all modes. Benchmarks: BM_BulkLoad_seq/par for all tree types vs BM_Insert baseline.
- Add strong exception guarantee (try-catch + clear) to db::bulk_load and olc_db::bulk_load so tree is left empty on allocation failure - Add 5 OOM typed tests (x3 db types = 15 tests) exercising single key, inode4, inode16, inode48, and nested inode topologies - Add T44 OlcDbConcurrentReaders: after parallel bulk_load, spawn 4 qsbr_threads reading all 10K keys to verify lock initialization - Link test_art_bulk_load_ops against qsbr_test_utils - Fix clang-format-21 whitespace in benchmark file
- Add portability_execution.hpp with __cpp_lib_execution detection; provides fallback sequential/parallel policy types when <execution> is unavailable (Apple libc++, older libstdc++) - Replace all #include <execution> with portability header - Fix GCC -Wuseless-cast by using uint64_t loop variable in test_art_bulk_load.cpp and DISABLE_GCC_WARNING in structural test - Fix GCC -Wdangling-else by adding braces around EXPECT_GE/EXPECT_GT after bare if in test_art_bulk_load_structural.cpp
- Add UNODB_DETAIL_ASSERT bounds checks in partition_by_byte - Replace hardcoded 8 with (prefix_cap + 1) in chain_consumed - Extract make_bulk_inode shared lambda (eliminates duplication) - Add exception safety for parallel futures drain - Remove trailing blank lines in inode class closings - Add missing #include <array> in example_bulk_load.cpp - Wire bulk-load tests into coverage and valgrind targets - Strengthen OOM test with memory_use == 0 assertion
The array-based value_bitmask_field wraps elements in in_critical_section<T>, so direct array assignment fails. Use element-by-element assignment for inode_48 and inode_256 bulk_load_from_children.
thompsonbry
force-pushed
the
feat/bulk-load-636
branch
from
June 30, 2026 21:02
31bfbca to
6a18e57
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
test/test_art_oom.cpp (1)
672-678: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the full empty-state check after
std::bad_alloc.
empty()plus current memory use is still weaker than the repository’s own empty-tree contract intest/db_test_utils.hpp: stale node counters would pass here. Assert zero node counts too, or route this through the same stronger helper.Suggested assertion tightening
} catch (const std::bad_alloc&) { unodb::test::allocation_failure_injector::reset(); // Strong guarantee: tree must be empty after failed bulk_load UNODB_ASSERT_TRUE(test_db.empty()); `#ifdef` UNODB_DETAIL_WITH_STATS UNODB_ASSERT_EQ(test_db.get_current_memory_use(), 0); + UNODB_ASSERT_THAT(test_db.get_node_counts(), + ::testing::ElementsAre(0U, 0U, 0U, 0U, 0U)); `#endif` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_art_oom.cpp` around lines 672 - 678, The `std::bad_alloc` path in the bulk_load OOM test is only checking `test_db.empty()` and memory use, which is weaker than the repository’s full empty-tree contract. Update the assertion in the catch block to use the stronger empty-state verification from `test/db_test_utils.hpp` or add zero checks for the tree’s node counters alongside `test_db.empty()` so stale counters cannot slip through; use the existing `test_db`/`UNODB_ASSERT_*` test helpers to keep the check consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@art_bulk_load_detail.hpp`:
- Around line 306-320: The async launch path in the bulk-load builder can throw
after some futures have already been started, which leaves completed
build_result_t pointers uncollected. In the builder logic that creates the
std::future list, move the guards/children reserve work before launching tasks,
and wrap the std::async launch sequence so any successfully created futures are
drained through the existing get()/bulk_subtree_guard transfer path before
rethrowing. Keep the fix localized around the futures, guards, children, and
builder lambda handling.
- Around line 77-109: The build_prefix_chain lambda currently passes the raw
current child into inode_4::create() and then overwrites current, so an
exception during chain allocation can leak the child or a previously built
inode_4. Update build_prefix_chain() to transfer ownership of current into the
allocation path (for example via a guarded/owning local) until inode_4::create()
succeeds, then release it only after the new node_ptr_t is safely constructed.
Keep the same fix for both inode_4::create() calls and preserve the existing
stats accounting on the success path.
- Around line 311-313: The parallel worker path in the bulk-load builder is
updating shared db stats without synchronization, which races under
std::execution::par. In the code that launches the async work around builder,
make sure the lambdas that reach make_db_leaf_ptr() and build_chain() do not
mutate db<Key, Value>::current_memory_use, node_counts, or growing_inode_counts
unsafely for db/mutex_db; either route these types away from the parallel path
or guard those stats updates consistently, since olc_db already uses atomics but
this path does not.
In `@art_internal_impl.hpp`:
- Around line 3740-3742: The duplicated create_bulk declaration is making the
function signature invalid. In the create_bulk declarations referenced by
art_internal_impl.hpp, remove the extra repeated [[nodiscard]] static auto
create_bulk( line so each declaration appears only once, and apply the same
cleanup to the second create_bulk block as well.
In `@art.hpp`:
- Around line 2769-2774: The value_view upsert path in the update branch is
reusing the original value instead of the callback-mutated result from
fn(local). In the code around upsert_action::update and value_type/value_view
handling, make the reinsertion use the updated local value returned by the
lambda, not v. If local may alias leaf storage, copy it before calling
remove_internal(art_k) so the old leaf can be reclaimed safely, then pass that
copied updated value into insert_internal.
- Around line 363-373: The bulk_load policy handling in art.hpp is forwarding
std::execution::par into bulk_load_impl even though the ART/db allocator
contract says it is not thread-safe, so locate bulk_load and its policy dispatch
around bulk_load_impl and force the db path to use seq instead of async parallel
root partitions. If parallel behavior must remain, make the
allocator/thread-safety contract explicit and enforce it in the bulk_load policy
branch and any mutex_db-related path so concurrent allocations cannot occur.
In `@benchmark/micro_benchmark_bulk_load.cpp`:
- Around line 175-215: BM04 is mislabeled for the `db` and `mutex_db` cases
because `db::bulk_load` ignores the execution policy and `mutex_db::bulk_load`
just forwards it, so `BM_BulkLoad_par_db` and `BM_BulkLoad_par_mutex` are not
actually measuring parallel behavior. Update the benchmark family to keep the
`std::execution::par` test only for `olc_db`, and either remove the
`db`/`mutex_db` variants or rename them so they clearly measure
sequential/overhead-only behavior. Use the existing `BM_BulkLoad_par_db`,
`BM_BulkLoad_par_mutex`, and `BM_BulkLoad_par_olc` symbols to locate the
affected benchmark cases.
In `@examples/example_bulk_load.cpp`:
- Around line 50-69: The “parallel bulk_load” examples in example_bulk_load.cpp
are misleading because db::bulk_load ignores the execution policy and
mutex_db::bulk_load forwards to the same implementation, so neither actually
builds concurrently. Update the example to use olc_db if you want a true
parallel construction demo, or change the section comments and stderr labels in
the db and mutex_db blocks so they no longer claim parallel behavior. Keep the
references around bulk_load, db<std::uint64_t, unodb::value_view>, and
mutex_db<std::uint64_t, unodb::value_view> aligned with the actual semantics.
- Around line 26-33: make_sorted_data() is returning pairs that hold dangling
unodb::value_view references because the backing bytes are stack-allocated
inside the function; update the data construction so each value_view points to
storage that remains alive after make_sorted_data() returns, using the
make_sorted_data() helper and the kv vector as the key locations to adjust.
Ensure bulk_load() receives views backed by persistent memory, either by moving
the owned byte storage outside the function or by returning an owning container
alongside the views.
In `@olc_art.hpp`:
- Around line 2856-2879: In the replacement-leaf path inside the value_type ==
value_view branch, the new leaf is being created from the original insert
argument instead of the updated local value returned by fn(local), so the
callback’s requested change is lost. Update the leaf construction logic in this
branch (and the matching update branch at the other referenced location) to
build/copy the replacement leaf from local before reclaiming the old leaf, and
keep the pointer swap and reclaim flow in create_leaf_if_needed and the parent
write-lock section unchanged.
In `@portability_execution.hpp`:
- Around line 27-41: Move the fallback execution policy tags out of
std::execution and into a project-owned namespace in portability_execution.hpp,
since defining new standard-library types there is undefined behavior and may
clash with partial <execution> support. Update the fallback policy symbols
sequenced_policy, parallel_policy, parallel_unsequenced_policy, and the
seq/par/par_unseq constants to live under that namespace, then switch the
affected call sites that currently use seq or par to reference the new namespace
instead.
In `@test/test_art_bulk_load_ops.cpp`:
- Around line 133-151: The BulkLoadOps::NoGrowthEvents test currently only
verifies that keys can be read back, so it does not actually validate the
no-growth behavior it claims. Update the test in test_art_bulk_load_ops.cpp to
assert the relevant bulk_load growth/shrink counters or inode topology that
proves no growth events occurred during bulk loading, using the existing
db.bulk_load and db.get checks as needed; if those invariants are not available,
rename the test to reflect the actual behavior it verifies.
In `@test/test_art_oom.cpp`:
- Around line 653-728: Add an OOM regression test for the parallel bulk-load
path, since bulk_load_oom_test currently only exercises bulk_load(kv.begin(),
kv.end()). Introduce at least one case that drives the std::execution::par
overload and spans multiple root partitions so the std::async root-task setup is
covered. Reuse the existing ARTOOMTest helpers and add coverage for both db and
olc_db through the parallel bulk_load entry point to verify cleanup on
allocation failure.
---
Duplicate comments:
In `@test/test_art_oom.cpp`:
- Around line 672-678: The `std::bad_alloc` path in the bulk_load OOM test is
only checking `test_db.empty()` and memory use, which is weaker than the
repository’s full empty-tree contract. Update the assertion in the catch block
to use the stronger empty-state verification from `test/db_test_utils.hpp` or
add zero checks for the tree’s node counters alongside `test_db.empty()` so
stale counters cannot slip through; use the existing `test_db`/`UNODB_ASSERT_*`
test helpers to keep the check consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e6588f3-80c5-4e00-92f9-c37458ed8614
📒 Files selected for processing (15)
art.hppart_bulk_load_detail.hppart_internal_impl.hppbenchmark/CMakeLists.txtbenchmark/micro_benchmark_bulk_load.cppexamples/CMakeLists.txtexamples/example_bulk_load.cppmutex_art.hppolc_art.hppportability_execution.hpptest/CMakeLists.txttest/test_art_bulk_load.cpptest/test_art_bulk_load_ops.cpptest/test_art_bulk_load_structural.cpptest/test_art_oom.cpp
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
♻️ Duplicate comments (1)
test/test_art_oom.cpp (1)
672-678: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the full empty-state check after
std::bad_alloc.
empty()plus current memory use is still weaker than the repository’s own empty-tree contract intest/db_test_utils.hpp: stale node counters would pass here. Assert zero node counts too, or route this through the same stronger helper.Suggested assertion tightening
} catch (const std::bad_alloc&) { unodb::test::allocation_failure_injector::reset(); // Strong guarantee: tree must be empty after failed bulk_load UNODB_ASSERT_TRUE(test_db.empty()); `#ifdef` UNODB_DETAIL_WITH_STATS UNODB_ASSERT_EQ(test_db.get_current_memory_use(), 0); + UNODB_ASSERT_THAT(test_db.get_node_counts(), + ::testing::ElementsAre(0U, 0U, 0U, 0U, 0U)); `#endif` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_art_oom.cpp` around lines 672 - 678, The `std::bad_alloc` path in the bulk_load OOM test is only checking `test_db.empty()` and memory use, which is weaker than the repository’s full empty-tree contract. Update the assertion in the catch block to use the stronger empty-state verification from `test/db_test_utils.hpp` or add zero checks for the tree’s node counters alongside `test_db.empty()` so stale counters cannot slip through; use the existing `test_db`/`UNODB_ASSERT_*` test helpers to keep the check consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@art_bulk_load_detail.hpp`:
- Around line 306-320: The async launch path in the bulk-load builder can throw
after some futures have already been started, which leaves completed
build_result_t pointers uncollected. In the builder logic that creates the
std::future list, move the guards/children reserve work before launching tasks,
and wrap the std::async launch sequence so any successfully created futures are
drained through the existing get()/bulk_subtree_guard transfer path before
rethrowing. Keep the fix localized around the futures, guards, children, and
builder lambda handling.
- Around line 77-109: The build_prefix_chain lambda currently passes the raw
current child into inode_4::create() and then overwrites current, so an
exception during chain allocation can leak the child or a previously built
inode_4. Update build_prefix_chain() to transfer ownership of current into the
allocation path (for example via a guarded/owning local) until inode_4::create()
succeeds, then release it only after the new node_ptr_t is safely constructed.
Keep the same fix for both inode_4::create() calls and preserve the existing
stats accounting on the success path.
- Around line 311-313: The parallel worker path in the bulk-load builder is
updating shared db stats without synchronization, which races under
std::execution::par. In the code that launches the async work around builder,
make sure the lambdas that reach make_db_leaf_ptr() and build_chain() do not
mutate db<Key, Value>::current_memory_use, node_counts, or growing_inode_counts
unsafely for db/mutex_db; either route these types away from the parallel path
or guard those stats updates consistently, since olc_db already uses atomics but
this path does not.
In `@art_internal_impl.hpp`:
- Around line 3740-3742: The duplicated create_bulk declaration is making the
function signature invalid. In the create_bulk declarations referenced by
art_internal_impl.hpp, remove the extra repeated [[nodiscard]] static auto
create_bulk( line so each declaration appears only once, and apply the same
cleanup to the second create_bulk block as well.
In `@art.hpp`:
- Around line 2769-2774: The value_view upsert path in the update branch is
reusing the original value instead of the callback-mutated result from
fn(local). In the code around upsert_action::update and value_type/value_view
handling, make the reinsertion use the updated local value returned by the
lambda, not v. If local may alias leaf storage, copy it before calling
remove_internal(art_k) so the old leaf can be reclaimed safely, then pass that
copied updated value into insert_internal.
- Around line 363-373: The bulk_load policy handling in art.hpp is forwarding
std::execution::par into bulk_load_impl even though the ART/db allocator
contract says it is not thread-safe, so locate bulk_load and its policy dispatch
around bulk_load_impl and force the db path to use seq instead of async parallel
root partitions. If parallel behavior must remain, make the
allocator/thread-safety contract explicit and enforce it in the bulk_load policy
branch and any mutex_db-related path so concurrent allocations cannot occur.
In `@benchmark/micro_benchmark_bulk_load.cpp`:
- Around line 175-215: BM04 is mislabeled for the `db` and `mutex_db` cases
because `db::bulk_load` ignores the execution policy and `mutex_db::bulk_load`
just forwards it, so `BM_BulkLoad_par_db` and `BM_BulkLoad_par_mutex` are not
actually measuring parallel behavior. Update the benchmark family to keep the
`std::execution::par` test only for `olc_db`, and either remove the
`db`/`mutex_db` variants or rename them so they clearly measure
sequential/overhead-only behavior. Use the existing `BM_BulkLoad_par_db`,
`BM_BulkLoad_par_mutex`, and `BM_BulkLoad_par_olc` symbols to locate the
affected benchmark cases.
In `@examples/example_bulk_load.cpp`:
- Around line 50-69: The “parallel bulk_load” examples in example_bulk_load.cpp
are misleading because db::bulk_load ignores the execution policy and
mutex_db::bulk_load forwards to the same implementation, so neither actually
builds concurrently. Update the example to use olc_db if you want a true
parallel construction demo, or change the section comments and stderr labels in
the db and mutex_db blocks so they no longer claim parallel behavior. Keep the
references around bulk_load, db<std::uint64_t, unodb::value_view>, and
mutex_db<std::uint64_t, unodb::value_view> aligned with the actual semantics.
- Around line 26-33: make_sorted_data() is returning pairs that hold dangling
unodb::value_view references because the backing bytes are stack-allocated
inside the function; update the data construction so each value_view points to
storage that remains alive after make_sorted_data() returns, using the
make_sorted_data() helper and the kv vector as the key locations to adjust.
Ensure bulk_load() receives views backed by persistent memory, either by moving
the owned byte storage outside the function or by returning an owning container
alongside the views.
In `@olc_art.hpp`:
- Around line 2856-2879: In the replacement-leaf path inside the value_type ==
value_view branch, the new leaf is being created from the original insert
argument instead of the updated local value returned by fn(local), so the
callback’s requested change is lost. Update the leaf construction logic in this
branch (and the matching update branch at the other referenced location) to
build/copy the replacement leaf from local before reclaiming the old leaf, and
keep the pointer swap and reclaim flow in create_leaf_if_needed and the parent
write-lock section unchanged.
In `@portability_execution.hpp`:
- Around line 27-41: Move the fallback execution policy tags out of
std::execution and into a project-owned namespace in portability_execution.hpp,
since defining new standard-library types there is undefined behavior and may
clash with partial <execution> support. Update the fallback policy symbols
sequenced_policy, parallel_policy, parallel_unsequenced_policy, and the
seq/par/par_unseq constants to live under that namespace, then switch the
affected call sites that currently use seq or par to reference the new namespace
instead.
In `@test/test_art_bulk_load_ops.cpp`:
- Around line 133-151: The BulkLoadOps::NoGrowthEvents test currently only
verifies that keys can be read back, so it does not actually validate the
no-growth behavior it claims. Update the test in test_art_bulk_load_ops.cpp to
assert the relevant bulk_load growth/shrink counters or inode topology that
proves no growth events occurred during bulk loading, using the existing
db.bulk_load and db.get checks as needed; if those invariants are not available,
rename the test to reflect the actual behavior it verifies.
In `@test/test_art_oom.cpp`:
- Around line 653-728: Add an OOM regression test for the parallel bulk-load
path, since bulk_load_oom_test currently only exercises bulk_load(kv.begin(),
kv.end()). Introduce at least one case that drives the std::execution::par
overload and spans multiple root partitions so the std::async root-task setup is
covered. Reuse the existing ARTOOMTest helpers and add coverage for both db and
olc_db through the parallel bulk_load entry point to verify cleanup on
allocation failure.
---
Duplicate comments:
In `@test/test_art_oom.cpp`:
- Around line 672-678: The `std::bad_alloc` path in the bulk_load OOM test is
only checking `test_db.empty()` and memory use, which is weaker than the
repository’s full empty-tree contract. Update the assertion in the catch block
to use the stronger empty-state verification from `test/db_test_utils.hpp` or
add zero checks for the tree’s node counters alongside `test_db.empty()` so
stale counters cannot slip through; use the existing `test_db`/`UNODB_ASSERT_*`
test helpers to keep the check consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e6588f3-80c5-4e00-92f9-c37458ed8614
📒 Files selected for processing (15)
art.hppart_bulk_load_detail.hppart_internal_impl.hppbenchmark/CMakeLists.txtbenchmark/micro_benchmark_bulk_load.cppexamples/CMakeLists.txtexamples/example_bulk_load.cppmutex_art.hppolc_art.hppportability_execution.hpptest/CMakeLists.txttest/test_art_bulk_load.cpptest/test_art_bulk_load_ops.cpptest/test_art_bulk_load_structural.cpptest/test_art_oom.cpp
🛑 Comments failed to post (13)
art_bulk_load_detail.hpp (3)
77-109: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Make prefix-chain construction own
currentwhile allocating.
build_prefix_chain()repeatedly passes a rawcurrentchild intoinode_4::create(). If any chain allocation throws, the input child or the previously-created chain is no longer guarded and leaks, violating the bulk-load exception guarantee.Proposed direction
auto build_prefix_chain = [&self](art_key_type k, node_ptr_t child_inode, tree_depth_type start_depth, std::size_t end_depth) -> node_ptr_t { const auto full_key = k.get_key_view(); const auto start = static_cast<std::size_t>(start_depth); - auto current = child_inode; + guard_t current_guard{self}; + current_guard.ptr = child_inode; + current_guard.is_packed_value = false; std::size_t pos = end_depth; while (pos > start + prefix_cap) { const auto depth = pos - prefix_cap - 1; const auto dispatch = full_key[pos - 1]; auto remaining = k; remaining.shift_right(depth); auto chain{ inode_4::create(self, full_key, remaining, tree_depth_type{static_cast<std::uint32_t>(depth)}, - dispatch, current)}; - current = node_ptr_t{chain.release(), node_type::I4}; + dispatch, current_guard.ptr)}; + current_guard.release(); + current_guard.ptr = node_ptr_t{chain.release(), node_type::I4}; `#ifdef` UNODB_DETAIL_WITH_STATS self.template account_growing_inode<node_type::I4>(); `#endif` pos = depth; } if (pos > start) { const auto dispatch = full_key[pos - 1]; auto chain{inode_4::create( self, full_key, tree_depth_type{static_cast<std::uint32_t>(start)}, - static_cast<key_prefix_size>(pos - start - 1), dispatch, current)}; - current = node_ptr_t{chain.release(), node_type::I4}; + static_cast<key_prefix_size>(pos - start - 1), dispatch, + current_guard.ptr)}; + current_guard.release(); + current_guard.ptr = node_ptr_t{chain.release(), node_type::I4}; `#ifdef` UNODB_DETAIL_WITH_STATS self.template account_growing_inode<node_type::I4>(); `#endif` } - return current; + auto result = current_guard.ptr; + current_guard.release(); + return result; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.auto build_prefix_chain = [&self](art_key_type k, node_ptr_t child_inode, tree_depth_type start_depth, std::size_t end_depth) -> node_ptr_t { const auto full_key = k.get_key_view(); const auto start = static_cast<std::size_t>(start_depth); guard_t current_guard{self}; current_guard.ptr = child_inode; current_guard.is_packed_value = false; std::size_t pos = end_depth; while (pos > start + prefix_cap) { const auto depth = pos - prefix_cap - 1; const auto dispatch = full_key[pos - 1]; auto remaining = k; remaining.shift_right(depth); auto chain{ inode_4::create(self, full_key, remaining, tree_depth_type{static_cast<std::uint32_t>(depth)}, dispatch, current_guard.ptr)}; current_guard.release(); current_guard.ptr = node_ptr_t{chain.release(), node_type::I4}; `#ifdef` UNODB_DETAIL_WITH_STATS self.template account_growing_inode<node_type::I4>(); `#endif` pos = depth; } if (pos > start) { const auto dispatch = full_key[pos - 1]; auto chain{inode_4::create( self, full_key, tree_depth_type{static_cast<std::uint32_t>(start)}, static_cast<key_prefix_size>(pos - start - 1), dispatch, current_guard.ptr)}; current_guard.release(); current_guard.ptr = node_ptr_t{chain.release(), node_type::I4}; `#ifdef` UNODB_DETAIL_WITH_STATS self.template account_growing_inode<node_type::I4>(); `#endif` } auto result = current_guard.ptr; current_guard.release(); return result;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art_bulk_load_detail.hpp` around lines 77 - 109, The build_prefix_chain lambda currently passes the raw current child into inode_4::create() and then overwrites current, so an exception during chain allocation can leak the child or a previously built inode_4. Update build_prefix_chain() to transfer ownership of current into the allocation path (for example via a guarded/owning local) until inode_4::create() succeeds, then release it only after the new node_ptr_t is safely constructed. Keep the same fix for both inode_4::create() calls and preserve the existing stats accounting on the success path.
306-320: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Protect futures before every post-launch throw path.
After the first async task is launched,
std::async,guards.reserve(), orchildren.reserve()can still throw before the guardedget()loop starts. Destroying already-completed futures at that point dropsbuild_result_traw pointers without transferring them tobulk_subtree_guard.Reserve guard/child storage before launching tasks, and wrap the launch phase itself so any valid futures are drained before rethrowing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art_bulk_load_detail.hpp` around lines 306 - 320, The async launch path in the bulk-load builder can throw after some futures have already been started, which leaves completed build_result_t pointers uncollected. In the builder logic that creates the std::future list, move the guards/children reserve work before launching tasks, and wrap the std::async launch sequence so any successfully created futures are drained through the existing get()/bulk_subtree_guard transfer path before rethrowing. Keep the fix localized around the futures, guards, children, and builder lambda handling.
311-313: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect shared Db state touched by async bulk-load builders. rg -n -C3 'account_growing_inode|make_db_leaf_ptr|build_chain|bulk_load_impl' . rg -n -C3 'UNODB_DETAIL_WITH_STATS|struct.*stats|class.*stats|std::atomic|mutex' .Repository: unodb-dev/unodb
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the bulk-load implementation and the shared state it calls into. ast-grep outline art_bulk_load_detail.hpp --view expanded printf '\n--- art_bulk_load_detail.hpp (relevant slice) ---\n' sed -n '250,380p' art_bulk_load_detail.hpp printf '\n--- builder-related symbols ---\n' rg -n -C 2 'account_growing_inode|make_db_leaf_ptr|build_chain|bulk_load_impl|builder\(' art_bulk_load_detail.hpp art_internal_impl.hpp art_internal.hpp art.hpp mutex_art.hpp olc_art.hpp art_allocator.hpp qsbr.hpp printf '\n--- stats / synchronization in qsbr.hpp ---\n' rg -n -C 2 'dealloc_stats_lock|quiescent_state_stats_lock|std::mutex|std::atomic|register_dealloc_stats|register_quiescent_states' qsbr.hpp printf '\n--- db / mutex_db / olc_db bulk-load entry points ---\n' rg -n -C 2 'bulk_load\(' art.hpp mutex_art.hpp olc_art.hpp art_internal_impl.hppRepository: unodb-dev/unodb
Length of output: 40548
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- art_internal_impl.hpp: leaf allocation path ---\n' sed -n '520,620p' art_internal_impl.hpp printf '\n--- art_internal_impl.hpp: bulk-load helpers and build_chain context ---\n' sed -n '820,980p' art_internal_impl.hpp printf '\n--- art.hpp: build_chain / accounting definitions ---\n' sed -n '1688,1775p' art.hpp sed -n '2688,2715p' art.hpp printf '\n--- olc_art.hpp: build_chain / accounting definitions ---\n' sed -n '2368,2418p' olc_art.hpp sed -n '4698,4720p' olc_art.hpp printf '\n--- art_allocator.hpp: allocator thread-safety / shared state ---\n' sed -n '1,220p' art_allocator.hppRepository: unodb-dev/unodb
Length of output: 21058
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- art.hpp: leaf / memory accounting helpers ---\n' sed -n '2660,2745p' art.hpp printf '\n--- olc_art.hpp: leaf / memory accounting helpers ---\n' sed -n '4680,4745p' olc_art.hpp printf '\n--- art.hpp: stats fields ---\n' rg -n -C 2 'leaf_count|memory_use|node_counts|growing_inode_counts|shrinking_inode_counts|key_prefix_splits' art.hpp printf '\n--- olc_art.hpp: stats fields ---\n' rg -n -C 2 'leaf_count|memory_use|node_counts|growing_inode_counts|shrinking_inode_counts|key_prefix_splits' olc_art.hppRepository: unodb-dev/unodb
Length of output: 18038
db/mutex_dbrace here withstd::execution::par. The worker lambdas callmake_db_leaf_ptr()andbuild_chain(), which updatedb<Key, Value>::current_memory_use,node_counts, andgrowing_inode_countsas plain members; the outer mutex onmutex_dbdoes not protect those worker threads.olc_dbuses atomics, but this path is UB fordbwhenUNODB_DETAIL_WITH_STATSis on. Either disable the parallel path fordb/mutex_dbor synchronize those stats updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art_bulk_load_detail.hpp` around lines 311 - 313, The parallel worker path in the bulk-load builder is updating shared db stats without synchronization, which races under std::execution::par. In the code that launches the async work around builder, make sure the lambdas that reach make_db_leaf_ptr() and build_chain() do not mutate db<Key, Value>::current_memory_use, node_counts, or growing_inode_counts unsafely for db/mutex_db; either route these types away from the parallel path or guard those stats updates consistently, since olc_db already uses atomics but this path does not.art_internal_impl.hpp (1)
3740-3742: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicated
create_bulkdeclaration lines.The repeated
[[nodiscard]] static auto create_bulk(lines make these function declarations syntactically invalid. Delete the extra duplicate in both blocks.Proposed cleanup
UNODB_DETAIL_DISABLE_CLANG_21_WARNING("-Wnrvo") - [[nodiscard]] static auto create_bulk( [[nodiscard]] static auto create_bulk(Also applies to: 4553-4555
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art_internal_impl.hpp` around lines 3740 - 3742, The duplicated create_bulk declaration is making the function signature invalid. In the create_bulk declarations referenced by art_internal_impl.hpp, remove the extra repeated [[nodiscard]] static auto create_bulk( line so each declaration appears only once, and apply the same cleanup to the second create_bulk block as well.art.hpp (2)
363-373: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don’t forward
dbparallel policies into async construction while the allocator contract says it is not thread-safe.Line 368 says parallelism is ignored for
dbbecause the allocator is not thread-safe, but Line 2844 forwardsstd::execution::partobulk_load_impl, whose policy branch uses async parallel root partitions. That can concurrently allocate through the samedbinstance, including viamutex_db. Forceseqhere or make the allocator/thread-safety contract explicit and enforced.Also applies to: 2837-2845
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art.hpp` around lines 363 - 373, The bulk_load policy handling in art.hpp is forwarding std::execution::par into bulk_load_impl even though the ART/db allocator contract says it is not thread-safe, so locate bulk_load and its policy dispatch around bulk_load_impl and force the db path to use seq instead of async parallel root partitions. If parallel behavior must remain, make the allocator/thread-safety contract explicit and enforce it in the bulk_load policy branch and any mutex_db-related path so concurrent allocations cannot occur.
2769-2774: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the lambda-updated value for
value_viewupserts.This branch calls
fn(local)but then removes and reinsertsv, soupsert_action::updateignores the callback’s mutation. Reinsert fromlocal, and copy it before reclaiming the old leaf if it may alias leaf storage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@art.hpp` around lines 2769 - 2774, The value_view upsert path in the update branch is reusing the original value instead of the callback-mutated result from fn(local). In the code around upsert_action::update and value_type/value_view handling, make the reinsertion use the updated local value returned by the lambda, not v. If local may alias leaf storage, copy it before calling remove_internal(art_k) so the old leaf can be reclaimed safely, then pass that copied updated value into insert_internal.benchmark/micro_benchmark_bulk_load.cpp (1)
175-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
BM04is benchmarking fake parallelism fordbandmutex_db.
db::bulk_loadignores the execution policy, andmutex_db::bulk_loadonly forwards that same call. SoBM_BulkLoad_par_dbandBM_BulkLoad_par_mutexare just sequential runs mislabeled as parallel, which invalidates this benchmark family and any speedup claims derived from it. Keep BM04 onolc_dbonly, or rename the other cases to benchmark overload/policy overhead instead.Also applies to: 397-400
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/micro_benchmark_bulk_load.cpp` around lines 175 - 215, BM04 is mislabeled for the `db` and `mutex_db` cases because `db::bulk_load` ignores the execution policy and `mutex_db::bulk_load` just forwards it, so `BM_BulkLoad_par_db` and `BM_BulkLoad_par_mutex` are not actually measuring parallel behavior. Update the benchmark family to keep the `std::execution::par` test only for `olc_db`, and either remove the `db`/`mutex_db` variants or rename them so they clearly measure sequential/overhead-only behavior. Use the existing `BM_BulkLoad_par_db`, `BM_BulkLoad_par_mutex`, and `BM_BulkLoad_par_olc` symbols to locate the affected benchmark cases.examples/example_bulk_load.cpp (2)
26-33: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
make_sorted_data()returns danglingvalue_views.Line 27 allocates the backing bytes on the stack, then every element in
kvstores avalue_viewpointing at that buffer. Once the function returns, all those views dangle and the laterbulk_load()calls read dead memory.Fix the lifetime
namespace { constexpr std::size_t key_count = 100'000; +static constexpr std::array<std::byte, 8> value_bytes{}; /// Build sorted key-value pairs. The caller is responsible for /// pre-encoding and pre-sorting keys before calling bulk_load. [[nodiscard]] auto make_sorted_data() { - constexpr std::array<std::byte, 8> val{}; - const auto value = unodb::value_view{val}; + const auto value = unodb::value_view{value_bytes}; std::vector<std::pair<std::uint64_t, unodb::value_view>> kv; kv.reserve(key_count); for (std::size_t i = 0; i < key_count; ++i)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.namespace { constexpr std::size_t key_count = 100'000; static constexpr std::array<std::byte, 8> value_bytes{}; /// Build sorted key-value pairs. The caller is responsible for /// pre-encoding and pre-sorting keys before calling bulk_load. [[nodiscard]] auto make_sorted_data() { const auto value = unodb::value_view{value_bytes}; std::vector<std::pair<std::uint64_t, unodb::value_view>> kv; kv.reserve(key_count); for (std::size_t i = 0; i < key_count; ++i) kv.emplace_back(static_cast<std::uint64_t>(i), value); return kv; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/example_bulk_load.cpp` around lines 26 - 33, make_sorted_data() is returning pairs that hold dangling unodb::value_view references because the backing bytes are stack-allocated inside the function; update the data construction so each value_view points to storage that remains alive after make_sorted_data() returns, using the make_sorted_data() helper and the kv vector as the key locations to adjust. Ensure bulk_load() receives views backed by persistent memory, either by moving the owned byte storage outside the function or by returning an owning container alongside the views.
50-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
These “parallel” examples are wrong for
dbandmutex_db.
db::bulk_loaddocuments that the execution policy is ignored, andmutex_db::bulk_loadjust forwards to that same call. So this file is currently claiming concurrent construction where none happens. Useolc_dbfor the real parallel example, or rename the sections/output so they stop advertising parallel behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/example_bulk_load.cpp` around lines 50 - 69, The “parallel bulk_load” examples in example_bulk_load.cpp are misleading because db::bulk_load ignores the execution policy and mutex_db::bulk_load forwards to the same implementation, so neither actually builds concurrently. Update the example to use olc_db if you want a true parallel construction demo, or change the section comments and stderr labels in the db and mutex_db blocks so they no longer claim parallel behavior. Keep the references around bulk_load, db<std::uint64_t, unodb::value_view>, and mutex_db<std::uint64_t, unodb::value_view> aligned with the actual semantics.olc_art.hpp (1)
2856-2879: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Build replacement
value_viewleaves fromlocal, not the insert argument.Both update branches call
fn(local)and then create the replacement leaf fromv, discarding the callback’s requested update. Allocate/copy fromlocalbefore reclaiming the current leaf.Also applies to: 2931-2957
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@olc_art.hpp` around lines 2856 - 2879, In the replacement-leaf path inside the value_type == value_view branch, the new leaf is being created from the original insert argument instead of the updated local value returned by fn(local), so the callback’s requested change is lost. Update the leaf construction logic in this branch (and the matching update branch at the other referenced location) to build/copy the replacement leaf from local before reclaiming the old leaf, and keep the pointer swap and reclaim flow in create_leaf_if_needed and the parent write-lock section unchanged.portability_execution.hpp (1)
27-41: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect declared C++ targets/toolchains and all execution shim usage. rg -n "CMAKE_CXX_STANDARD|cxx_std_|__cpp_lib_execution|__has_include\\(<execution>\\)|namespace std::execution|UNODB_DETAIL_HAS_EXECUTION_POLICIES" .Repository: unodb-dev/unodb
Length of output: 1026
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== portability_execution.hpp ==" cat -n portability_execution.hpp echo echo "== usages of execution policy tags ==" rg -n "\b(std::execution::)?(seq|par|par_unseq)\b|UNODB_DETAIL_HAS_EXECUTION_POLICIES" .Repository: unodb-dev/unodb
Length of output: 4151
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== include sites for portability_execution.hpp ==" rg -n '`#include` "portability_execution\.hpp"|`#include` <portability_execution\.hpp>' . echo echo "== surrounding declarations in mutex_art.hpp and art.hpp ==" sed -n '130,190p' mutex_art.hpp echo sed -n '350,410p' art.hpp echo sed -n '300,340p' olc_art.hppRepository: unodb-dev/unodb
Length of output: 7134
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== include sites for portability_execution.hpp ==" rg -n '`#include` "portability_execution\.hpp"|`#include` <portability_execution\.hpp>' . echo echo "== surrounding declarations in mutex_art.hpp ==" sed -n '130,190p' mutex_art.hpp echo echo "== surrounding declarations in art.hpp ==" sed -n '350,410p' art.hpp echo echo "== surrounding declarations in olc_art.hpp ==" sed -n '300,340p' olc_art.hppRepository: unodb-dev/unodb
Length of output: 7210
Move the fallback execution tags out of
std::execution.portability_execution.hpp:27-41defines new policy types instd, which is undefined behavior and can collide with partial<execution>implementations. Put the fallback in a project namespace and switch the fewseq/parcall sites to that namespace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portability_execution.hpp` around lines 27 - 41, Move the fallback execution policy tags out of std::execution and into a project-owned namespace in portability_execution.hpp, since defining new standard-library types there is undefined behavior and may clash with partial <execution> support. Update the fallback policy symbols sequenced_policy, parallel_policy, parallel_unsequenced_policy, and the seq/par/par_unseq constants to live under that namespace, then switch the affected call sites that currently use seq or par to reference the new namespace instead.test/test_art_bulk_load_ops.cpp (1)
133-151: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test never checks “NoGrowthEvents”.
It only re-reads keys, so a regression that reintroduces grow/shrink behavior during bulk load would still pass. Either assert the relevant counters/topology here or rename the test to match what it actually verifies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_art_bulk_load_ops.cpp` around lines 133 - 151, The BulkLoadOps::NoGrowthEvents test currently only verifies that keys can be read back, so it does not actually validate the no-growth behavior it claims. Update the test in test_art_bulk_load_ops.cpp to assert the relevant bulk_load growth/shrink counters or inode topology that proves no growth events occurred during bulk loading, using the existing db.bulk_load and db.get checks as needed; if those invariants are not available, rename the test to reflect the actual behavior it verifies.test/test_art_oom.cpp (1)
653-728: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add an OOM case for the parallel overload.
These tests only hit
bulk_load(kv.begin(), kv.end()). The new failure mode in this PR is thestd::execution::parpath withstd::asyncroot-task setup, so cleanup bugs there can still ship untested. Add at least onedb/olc_dbcase that usesstd::execution::parwith input spanning multiple root partitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_art_oom.cpp` around lines 653 - 728, Add an OOM regression test for the parallel bulk-load path, since bulk_load_oom_test currently only exercises bulk_load(kv.begin(), kv.end()). Introduce at least one case that drives the std::execution::par overload and spans multiple root partitions so the std::async root-task setup is covered. Reuse the existing ARTOOMTest helpers and add coverage for both db and olc_db through the parallel bulk_load entry point to verify cleanup on allocation failure.
In the build_single_leaf lambda, the else branch (classic leaf without key/leaf elimination) does not use depth. Cast to void to silence MSVC warning-as-error.
- C4100: guards/children unused when can_eliminate_leaf is false - C4365: signed/unsigned mismatch in vmask index for inode_256
The bulk_load parallel API now takes a fork callable + max_tasks instead of std::execution::par. The library never spawns threads internally — the caller owns all threading decisions. API: tree.bulk_load(first, last); // sequential tree.bulk_load(fork, max_tasks, first, last); // parallel Where fork(Callable) -> Future<R> with .get(). Works with std::async, thread pools, fiber pools, QSBR threads, etc. The implementation forks up to min(partitions, max_tasks) subtrees via the callable; remaining partitions run inline on the calling thread. Thread-local stats accumulators ensure correctness with STATS=ON regardless of the executor type. Also removes portability_execution.hpp (now unused) and dead <future> includes from library headers.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@art_internal_impl.hpp`:
- Around line 4549-4582: The bulk constructor in create_bulk currently leaves
child_indexes default-initialized, which bypasses the inode48_type empty-child
sentinel. Update create_bulk to explicitly initialize all 256 child_indexes
entries to inode48_type::empty_child before assigning entries from
children_span, while keeping the existing child population logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 618f3d98-6783-405e-92de-57f7a69d73ac
📒 Files selected for processing (15)
art.hppart_bulk_load_detail.hppart_internal_impl.hppbenchmark/CMakeLists.txtbenchmark/micro_benchmark_bulk_load.cppexamples/CMakeLists.txtexamples/example_bulk_load.cppmutex_art.hppnode_type.hppolc_art.hpptest/CMakeLists.txttest/test_art_bulk_load.cpptest/test_art_bulk_load_ops.cpptest/test_art_bulk_load_structural.cpptest/test_art_oom.cpp
- Pass value_mask by const ref in Node48/Node256 create_bulk (cppcheck passedByValue for 6-byte and 32-byte arrays) - Suppress functionStatic on olc_db::merge_bulk_load_stats (intentionally non-static for API consistency)
- Remove child_indexes{} from inode48 bulk_load_tag constructor init
list so the default member initializer (all 0xFF / empty_child) is
used. The previous {} zero-initialized, leaving absent key slots as
0 instead of the empty_child sentinel. (CodeRabbit finding)
- Pass value_mask by const ref in Node48/Node256 create_bulk
(cppcheck passedByValue on macOS)
- Suppress cppcheck functionStatic on olc_db::merge_bulk_load_stats
(intentionally non-static for API consistency)
- Replace raw loop with std::transform for random_keys (cppcheck useStlAlgorithm on macOS) - Suppress useStlAlgorithm for key_view_set index loops (no iterator) - Add <iterator> for std::back_inserter - clang-format fix in inode48 bulk_load_tag constructor
Move duplicated bulk_load infrastructure from art.hpp and olc_art.hpp into shared ArtPolicy-parameterized templates in art_internal_impl.hpp: - bulk_build_result<ArtPolicy>: tagged return type (ptr + is_packed) - bulk_subtree_guard<ArtPolicy>: RAII guard with policy-aware cleanup - bulk_build_chain<ArtPolicy>(): prefix chain builder (was build_chain) db::build_chain and olc_db::build_chain now delegate to the shared implementation. This eliminates ~80 lines of duplication.
Contributor
Author
|
@laurynas-biveinis can you do a review pass on this one? I think it is green modulo the various build timeouts. |
laurynas-biveinis
requested changes
Jul 10, 2026
Comment on lines
+43
to
+64
| /// Generate sorted sequential keys [0..n). | ||
| [[nodiscard]] auto sequential_keys(std::int64_t n) { | ||
| std::vector<std::pair<std::uint64_t, unodb::value_view>> kv; | ||
| kv.reserve(static_cast<std::size_t>(n)); | ||
| for (std::int64_t i = 0; i < n; ++i) | ||
| kv.emplace_back(static_cast<std::uint64_t>(i), value); | ||
| return kv; | ||
| } | ||
|
|
||
| /// Generate sorted random unique keys. | ||
| [[nodiscard]] auto random_keys(std::int64_t n) { | ||
| std::mt19937_64 rng(42); // NOLINT(cert-msc32-c,cert-msc51-cpp) | ||
| std::vector<std::uint64_t> keys(static_cast<std::size_t>(n)); | ||
| std::generate(keys.begin(), keys.end(), rng); | ||
| std::sort(keys.begin(), keys.end()); | ||
| keys.erase(std::unique(keys.begin(), keys.end()), keys.end()); | ||
| std::vector<std::pair<std::uint64_t, unodb::value_view>> kv; | ||
| kv.reserve(keys.size()); | ||
| std::transform(keys.begin(), keys.end(), std::back_inserter(kv), | ||
| [](auto k) { return std::make_pair(k, value); }); | ||
| return kv; | ||
| } |
Collaborator
There was a problem hiding this comment.
Not duplicated elsewhere?
| tree.clear(); | ||
| } | ||
|
|
||
| // ─── mutex_db: same API ──────────────────────────────────────────────────── |
Collaborator
There was a problem hiding this comment.
IMHO mutex_db should not be emphasized in examples, olc_db should be preferred
|
|
||
| UNODB_DETAIL_RESTORE_MSVC_WARNINGS() | ||
|
|
||
| #endif // UNODB_DETAIL_WITH_STATS |
Collaborator
There was a problem hiding this comment.
The split between the three bulk load testcase files is rather arbitrary (a case can be made that all three files are "ops" and also all are "structural") - merge?
| void bulk_load_oom_test( | ||
| unsigned fail_limit, | ||
| std::vector<std::pair<std::uint64_t, unodb::value_view>> | ||
| kv) { // NOLINT(performance-unnecessary-value-param) |
Collaborator
There was a problem hiding this comment.
Why pass by value? it's large and not moved from. Pass by const reference?
|
|
||
| /// Forward declaration of shared bulk_load algorithm. | ||
| template <typename Db, typename Fork, typename RandomIt> | ||
| void bulk_load_impl(Db& self, Fork&& fork, std::size_t max_tasks, |
Collaborator
There was a problem hiding this comment.
Why do we need a forward declaration at all if it's a template and its header has to be included anyway?
Comment on lines
+83
to
+87
| UNODB_DETAIL_DISABLE_GCC_WARNING("-Wpessimizing-move") | ||
| UNODB_DETAIL_DISABLE_MSVC_WARNING(26479) | ||
| return std::move(parts); // NOLINT(performance-move-const-arg) | ||
| UNODB_DETAIL_RESTORE_MSVC_WARNINGS() | ||
| UNODB_DETAIL_RESTORE_GCC_WARNINGS() |
Collaborator
There was a problem hiding this comment.
This all goes away if parts is returned directly, also enabling NRVO
| UNODB_DETAIL_RESTORE_GCC_WARNINGS() | ||
| }; | ||
|
|
||
| auto build_prefix_chain = [&self](art_key_type k, node_ptr_t child_inode, |
Collaborator
There was a problem hiding this comment.
Pull-up to the top level this and everything pullable below
Comment on lines
+112
to
+114
| // LCOV_EXCL_START — chain_consumed is always a multiple of (prefix_cap+1), | ||
| // so the while loop always decrements pos to exactly start. | ||
| if (pos > start) { |
Collaborator
There was a problem hiding this comment.
Is LCOV_EXCL_START and the comment saying that this is an always-false condition?
Comment on lines
+404
to
+405
| } catch (...) { | ||
| } |
Collaborator
There was a problem hiding this comment.
This can only throw if emplace_back throws but that should not throw because guards got a reserve call? Can this be asserted as unreachable code?
Comment on lines
+195
to
+196
| : db_{other.db_}, ptr{other.ptr}, is_packed_value{other.is_packed_value} { | ||
| other.ptr = nullptr; |
Collaborator
There was a problem hiding this comment.
Idiomatic is ptr{std::exchange(other.ptr), nullptr}
- Pull all lambdas to bulk_load_helpers<Db, RandomIt> struct with named static methods (common_prefix_length, partition_by_byte, build_prefix_chain, build_single_leaf, make_inode, build_subtree) - partition_entry<RandomIt> struct at namespace scope - Use std::exchange in bulk_subtree_guard move ctor - Add ptr_type alias to bulk_build_result for cleaner signatures - Make art_key_type, tree_depth_type, build_result, bulk_subtree_guard public in db and olc_db (needed for struct method signatures) - Pass vector by const& in bulk_load_oom_test - Replace mutex_db with olc_db in example - Clarify LCOV_EXCL with UNODB_DETAIL_ASSERT(pos == start) - Comment on inner catch: future.get() may rethrow task exception - NRVO: return parts directly (explicit return type enables it)
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
2 participants
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
Implements
bulk_loadfor all three ART tree modes (db,mutex_db,olc_db) withstd::execution_policy-based parallel execution.API
Performance (96-core, 1M random uint64 keys, Release)
Design
std::asyncper root-level partitionart_bulk_load_detail.hpp(single copy for all modes)create_bulkstatic factories on inode_4/16/48/256What was tested
Blocked
Summary by CodeRabbit
New Features
Bug Fixes
Tests