161 paired block bloom bpk adjustment #163
Conversation
isaac-io
force-pushed
the
main
branch
3 times, most recently
from
8c57b5d
to
b2f914c
Compare
|
@noamhaham - It seems the branch with your actual changes hasn't made it to the remote server, or at least not under this name. |
|
@noamhaham @Guyme , the last commit to this branch is 3 months ago.. dont know what erez tested #161 (comment) , but theres nothing to check here.. this branch also needs rebasing on rocksdb 7.7.3 |
udi-speedb
force-pushed
the
161-paired-block-bloom-bpk-adjustment
branch
from
dc868c7
to
af7e251
Compare
|
I have pushed the branch with @noamhaham's changes. However I have made some changes to his. |
|
Hi it was updated in the remote server.
It was pushed under the name:
pared_bloom_filter_improved.
The branch: pared_bloom_filter-improved is the same one with the dbbench
changes for testing.
I just verified that the changes are there, last commit was last month.
…On Wed, Dec 14, 2022 at 8:36 AM udi-speedb ***@***.***> wrote:
@noamhaham <https://github.com/noamhaham> - It seems the branch with your
actual changes hasn't made it to the remote server, or at least not under
this name.
Once it does, please let me know and I would review it.
—
Reply to this email directly, view it on GitHub
<#163 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A2PIKTMMGACAYL7XE4XHWL3WNFTGZANCNFSM6AAAAAAQNMG5RQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes. I have already found the branch (thanks to @erez-speedb). |
|
QA passed on af7e251 . but it hasnt been reviewed yet |
There was a problem hiding this comment.
This generally looks fine to me. I can see the association between the constants. One question though is whether or not that is artificial. If kPairedBloomBatchSizeInBlocks is changed (to 64U or 256U for example) is the expectation that the code would still work? If not what would break?
If the relationship holds, great. If not, please add comments somewhere (probably where kInBatchIdxNumBits is defined) explaining the requirements and assumptions
| @@ -214,7 +214,7 @@ inline void BuildBlock::SetBlockBloomBits(uint32_t hash, uint32_t set_idx, | |||
| size_t hash_set_size) { | |||
| for (auto i = 0U; i < hash_set_size; ++i) { | |||
| int bitpos = GetBitPosInBlockForHash(hash, set_idx); | |||
| block_address_[bitpos >> 3] |= (char{1} << (bitpos & kInBatchIdxNumBits)); | |||
| block_address_[bitpos >> 3] |= (char{1} << (bitpos & 7)); | |||
There was a problem hiding this comment.
Can you please add a comment here on why 3?
There was a problem hiding this comment.
The expectation is that the code will work for any value that is <=8.
There is a comment there.
There are comments describing it (lines 41-42 of the source file) and static assertions to verify that the limit is 256 (the index must fit within a single byte, otherwise the code will break).
That is the reason for the >>3 and & 7
Per your request, I have added a comment to clarify the >> and & where they are used
Please see additional commit.
udi-speedb
force-pushed
the
161-paired-block-bloom-bpk-adjustment
branch
from
104080d
to
4841816
Compare
No description provided.