Benchmarks refactoring#48
Conversation
Malkovsky
commented
May 18, 2026
Malkovsky
commented
- Added optional benchmark page in docs
- Refactored Rmm, added sdsl backend
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
- Coverage 87.98% 87.19% -0.79%
==========================================
Files 13 14 +1
Lines 3378 3257 -121
Branches 603 607 +4
==========================================
- Hits 2972 2840 -132
- Misses 264 285 +21
+ Partials 142 132 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3426c627f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| explicit RmMTree(const std::vector<std::uint64_t>& words, | ||
| size_t bit_count, | ||
| const size_t& leaf_block_bits /*0=auto*/ = 0, | ||
| const float& max_overhead /*<0=off*/ = -1.0) { | ||
| build_from_words(words, bit_count, leaf_block_bits, max_overhead); | ||
| const float& max_overhead /*<0=off*/ = -1.0) | ||
| : RmMTree(std::span<const std::uint64_t>(words), | ||
| bit_count, | ||
| leaf_block_bits, | ||
| max_overhead) {} |
There was a problem hiding this comment.
When callers construct from a temporary vector, e.g. RmMTree(std::vector<uint64_t>{...}, n) or RmMTree(pack_words(bits), bits.size()), this overload binds the temporary to const& and then stores only a std::span into it. The vector is destroyed at the end of the full expression, so the first query reads freed memory; I reproduced this with ASan as a heap-use-after-free in rank1_in_block. This is a regression from the previous owning vector constructor and is easy to hit with helper functions that return packed words, so the rvalue overload should be deleted or this overload should keep ownership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d82c54d044
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (end_position >= size_) { | ||
| return zeros_; | ||
| } | ||
| return tree_.preceding_closing_parentheses(end_position); |
There was a problem hiding this comment.
When the SDSL backend is used for rank0 on prefixes that are not the full vector, this calls bp_support_sada::preceding_closing_parentheses(end_position), which in SDSL v3.0.3 is documented for zeros preceding a position and implements i - select(rank(i)) - 1, i.e. only the consecutive closing parentheses since the previous opening. For a prefix like 101 (end_position == 3), the expected rank0 is 1 but this returns 0, so bench_rmm_sdsl --ops=rank0 reports timings for incorrect queries/results; use end_position - rank1_impl(end_position) (or a real zero-rank support) instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65ba810ed0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (std::size_t i = 0; i < words.size(); ++i) { | ||
| std::uint64_t word = words[i]; | ||
| if (i + 1 == words.size() && (bit_count & 63) != 0) { | ||
| word &= (std::uint64_t{1} << (bit_count & 63)) - 1; | ||
| } | ||
| ones_ += std::popcount(word); |
There was a problem hiding this comment.
When SdslRmMTree is constructed with a span larger than ceil(bit_count/64) (for example a reused word buffer with extra capacity or padding words), this loop counts all words in the span and only masks the final span word. The actual bits_ vector is built from only size_ bits, so ones_ can exceed size_, zeros_ = size_ - ones_ underflows, and calls such as rank0(size_) or out-of-range select1 return corrupt results. Limit the count to the valid word count and mask the word containing bit_count.
Useful? React with 👍 / 👎.
| public: | ||
| static constexpr std::size_t npos = std::numeric_limits<std::size_t>::max(); | ||
|
|
||
| std::size_t size() const { return impl().size_impl(); } |
There was a problem hiding this comment.
Why do we call some "size_impl()" instead of size() itself?
There was a problem hiding this comment.
Wouldn't calling size() result in infinite recursion and immediate stack overflow on call?
There was a problem hiding this comment.
Refactored, implementation classes now have impelmentations inside _impl methods
There was a problem hiding this comment.
Wouldn't calling
size()result in infinite recursion and immediate stack overflow on call?
That does not happen for some reason. I tested it and everything worked well. However, both the old solution and the one of mine had to do something with vTables and it is great that you have eliminated this kind of behaviour. Current code does not use any vTables as I see
There was a problem hiding this comment.
I think vtables are not the issue here, no method was marked virtual and derived didn't override these method. But if at some point we have a virtual method in base and override it in derived, this double recursion base::method -> derived::method_impl -> derived::method would lead to infinite recursion
| std::size_t size() const { return impl().size_impl(); } | ||
|
|
||
| std::size_t rank1(std::size_t end_position) const { | ||
| return impl().rank1_impl(end_position); |
There was a problem hiding this comment.
Same question about _impl here
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8cb3e1c07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| const std::size_t close_position_zero_based = | ||
| close_position_one_based > 0 ? close_position_one_based - 1 : 0; | ||
| return tree_.find_open(close_position_zero_based); |
There was a problem hiding this comment.
open_impl converts the incoming close position from one-based to zero-based, but then returns find_open's zero-based answer directly; enclose_impl does the same with tree_.enclose. The rest of this interface (and RmMTree) uses one-based results for these operations, so every successful open/enclose result is shifted by -1 (e.g., open(2) on "10" yields 0 instead of 1), which corrupts BP navigation when swapping to this backend.
Useful? React with 👍 / 👎.
|
|
||
| std::size_t close_impl(std::size_t open_position) const { | ||
| if (size_ == 0) { | ||
| return 0; |
There was a problem hiding this comment.
These query methods return 0 when size_ == 0, but this type defines npos as the not-found sentinel and 0 is also a valid position in non-empty trees. Returning 0 on failure makes callers misinterpret empty/invalid queries as successful matches at the first bit, which breaks correctness checks that rely on npos.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3824a8dfa9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (std::size_t i = 0; i < valid_words; ++i) { | ||
| std::uint64_t word = words[i]; |
There was a problem hiding this comment.
This constructor indexes words[i] up to ceil(bit_count/64) without checking that the span is that long, so callers that pass a shorter buffer (e.g., reused metadata with stale bit_count) trigger out-of-bounds reads during construction and can crash or compute corrupted counts. RmMTree now explicitly rejects this case, so this backend should add the same guard before the loop.
Useful? React with 👍 / 👎.
| const std::size_t close_position = | ||
| pool.close_positions_zero_based | ||
| [sample_index % pool.close_positions_zero_based.size()]; | ||
| return dataset.tree.open(close_position); |
There was a problem hiding this comment.
The benchmark now feeds zero-based close positions into tree.open(...), but RmMTree::open_impl/enclose_impl still use the older one-based convention (they reject 0 and add +1 on return), so these measurements exercise shifted/invalid queries rather than the intended operation. That skews benchmark results and makes comparisons against previous runs or other backends unreliable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57b50ff44d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| TEST(RmMSdslEdgeCases, IgnoresWordsBeyondBitCount) { | ||
| std::vector<std::uint64_t> words = { | ||
| 0b101ull, std::numeric_limits<std::uint64_t>::max()}; | ||
| pixie::SdslRmMTree rm(std::span<const std::uint64_t>(words), |
There was a problem hiding this comment.
When PIXIE_THIRD_PARTY_BACKENDS is enabled, SDSL_SUPPORT turns on this test block, but test_rmm.cpp never includes <pixie/rmm_tree_sdsl.h>. pixie/rmm_tree.h does not declare pixie::SdslRmMTree, so this line causes test_rmm to fail to compile in third-party-backend builds with an unknown type error.
Useful? React with 👍 / 👎.
| if (size_ == 0) { | ||
| return 0; | ||
| } | ||
| return tree_.rmq(range_begin, range_end); |
There was a problem hiding this comment.
This implementation calls tree_.rmq(range_begin, range_end) without checking range_begin <= range_end and range_end < size_. The main RmMTree backend guards these cases and returns sentinels, but here invalid ranges are forwarded directly into SDSL preconditioned APIs, which can return corrupt results or assert/crash instead of returning npos for out-of-domain queries.
Useful? React with 👍 / 👎.
2 participants
