Skip to content

Add support for C bindings to the compaction V2 filter mechanism.#224

Merged
igorcanadi merged 2 commits intofacebook:masterfrom
cockroachdb:spencerkimball/compaction-filter-v2-c-bindings
Aug 7, 2014
Merged

Add support for C bindings to the compaction V2 filter mechanism.#224
igorcanadi merged 2 commits intofacebook:masterfrom
cockroachdb:spencerkimball/compaction-filter-v2-c-bindings

Conversation

@spencerkimball
Copy link
Copy Markdown
Contributor

@spencerkimball spencerkimball commented Aug 6, 2014

Some pressing bug fixes to the db_impl.cc v2 compaction pipeline. In particular, the compact object was keeping vectors of string copies for keys and values and also vectors of Slice objects references the underlying string copies. However, the vectors of the string copies are likely to resize, resulting in strings being reallocated and the slices becoming invalid. The fixes in this PR to db_impl.cc delay the creation of the SliceVectors until after the string copy vectors have been fully built.

Test Plan: make c_test && ./c_test

Add support for C bindings to the compaction V2 filter mechanism.
c1f588a 
Test Plan: make c_test && ./c_test

Some fixes after merge.
igorcanadi
igorcanadi reviewed Aug 6, 2014
View reviewed changes
db/db_impl.cc Outdated
Copy link
Copy Markdown
Contributor

@igorcanadi igorcanadi Aug 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

track :)

@igorcanadi
Copy link
Copy Markdown
Contributor

igorcanadi commented Aug 7, 2014

Wow, thanks for finding this bug. I'm surprised how it didn't blow up in production :)

igorcanadi added a commit that referenced this pull request Aug 7, 2014
@igorcanadi
Merge pull request #224 from cockroachdb/spencerkimball/compaction-fi…
f8d6a29 
…lter-v2-c-bindings

Add support for C bindings to the compaction V2 filter mechanism.
@igorcanadi igorcanadi merged commit f8d6a29 into facebook:master Aug 7, 2014
@spencerkimball
Copy link
Copy Markdown
Contributor Author

spencerkimball commented Aug 7, 2014

Yeah, I'm surprised too. It failed immediately from the c_test compaction filter v2 unittest I wrote. Thanks for the quick turnaround!

@igorcanadi
Copy link
Copy Markdown
Contributor

igorcanadi commented Aug 7, 2014

There was a compile error in c_test that I fixed with 323d6e3. Let me know if that's not the correct fix.

Thanks for contributing!

@siying
Copy link
Copy Markdown
Contributor

siying commented Aug 7, 2014

valgrind failure of c_test. Likely caused by one of those commits. @spencerkimball can you take a look?

Commit c1f588a by spencerk
Add support for C bindings to the compaction V2 filter mechanism.

Commit 38e8b72 by spencerk
Fix typo, add missing inclusion of state void* in invocation of
create_compaction_filter_v2_.

Commit 323d6e3 by icanadi
Fix c_test

==12920==
==12920== HEAP SUMMARY:
==12920== in use at exit: 214 bytes in 6 blocks
==12920== total heap usage: 92,480 allocs, 92,474 frees, 20,510,094 bytes allocated
==12920==
==12920== 134 (64 direct, 70 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 6
==12920== at 0x4C2B7D9: operator new(unsigned long) (in /usr/local/fbcode/gcc-4.8.1-glibc-2.17/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12920== by 0x410A63: rocksdb_slicetransform_create_fixed_prefix (c.cc:1942)
==12920== by 0x4092EC: main (c_test.c:586)
==12920==
==12920== LEAK SUMMARY:
==12920== definitely lost: 64 bytes in 1 blocks
==12920== indirectly lost: 70 bytes in 2 blocks
==12920== possibly lost: 0 bytes in 0 blocks
==12920== still reachable: 80 bytes in 3 blocks
==12920== suppressed: 0 bytes in 0 blocks
==12920== Reachable blocks (those to which a pointer was found) are not shown.
==12920== To see them, rerun with: --leak-check=full --show-reachable=yes
==12920==
==12920== For counts of detected and suppressed errors, rerun with: -v
==12920== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2)

@spencerkimball
Copy link
Copy Markdown
Contributor Author

spencerkimball commented Aug 7, 2014

Sure. Sent a PR to address this.

On Thu, Aug 7, 2014 at 5:53 PM, Siying Dong notifications@github.com
wrote:

valgrind failure of c_test. Likely caused by one of those commits.
@spencerkimball https://github.com/spencerkimball can you take a look?

Commit c1f588a
c1f588a
by spencerk
Add support for C bindings to the compaction V2 filter mechanism.

Commit 38e8b72
38e8b72
by spencerk
Fix typo, add missing inclusion of state void* in invocation of
create_compaction_filter_v2_.

Commit 323d6e3
323d6e3
by icanadi
Fix c_test

==12920==
==12920== HEAP SUMMARY:
==12920== in use at exit: 214 bytes in 6 blocks
==12920== total heap usage: 92,480 allocs, 92,474 frees, 20,510,094 bytes
allocated
==12920==
==12920== 134 (64 direct, 70 indirect) bytes in 1 blocks are definitely
lost in loss record 6 of 6
==12920== at 0x4C2B7D9: operator new(unsigned long) (in
/usr/local/fbcode/gcc-4.8.1-glibc-2.17/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12920== by 0x410A63: rocksdb_slicetransform_create_fixed_prefix
(c.cc:1942)
==12920== by 0x4092EC: main (c_test.c:586)
==12920==
==12920== LEAK SUMMARY:
==12920== definitely lost: 64 bytes in 1 blocks
==12920== indirectly lost: 70 bytes in 2 blocks
==12920== possibly lost: 0 bytes in 0 blocks
==12920== still reachable: 80 bytes in 3 blocks
==12920== suppressed: 0 bytes in 0 blocks
==12920== Reachable blocks (those to which a pointer was found) are not
shown.
==12920== To see them, rerun with: --leak-check=full --show-reachable=yes
==12920==
==12920== For counts of detected and suppressed errors, rerun with: -v
==12920== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2)

—
Reply to this email directly or view it on GitHub
#224 (comment).

@igorcanadi
Copy link
Copy Markdown
Contributor

igorcanadi commented Aug 7, 2014

@siying 894a77a

@siying
Copy link
Copy Markdown
Contributor

siying commented Aug 7, 2014

Thanks guys.

Little-Wallace pushed a commit to Little-Wallace/rocksdb that referenced this pull request Feb 5, 2021
@hunterlxt
Atomize Rename operation when encryption is enabled (facebook#224)
00874eb 
Signed-off-by: Xintao <hunterlxt@live.com>
BusyJay pushed a commit to BusyJay/rocksdb that referenced this pull request Jul 25, 2022
@hunterlxt @tabokie
Atomize Rename operation when encryption is enabled (facebook#224)
bbd27cf 
Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
acelyc111 pushed a commit to acelyc111/rocksdb that referenced this pull request Jul 21, 2023
@hunterlxt @acelyc111
Atomize Rename operation when encryption is enabled (facebook#224)
0791870 
Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
(cherry picked from commit bbd27cf)
acelyc111 pushed a commit to acelyc111/rocksdb that referenced this pull request Jul 25, 2023
@hunterlxt @acelyc111
Atomize Rename operation when encryption is enabled (facebook#224)
3eeaf44 
Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
(cherry picked from commit bbd27cf)
acelyc111 pushed a commit to acelyc111/rocksdb that referenced this pull request Aug 8, 2023
@hunterlxt @acelyc111
Atomize Rename operation when encryption is enabled (facebook#224)
3c46cdd 
Signed-off-by: Xintao <hunterlxt@live.com>
acelyc111 pushed a commit to acelyc111/rocksdb that referenced this pull request Aug 8, 2023
@hunterlxt @acelyc111
Atomize Rename operation when encryption is enabled (facebook#224)
0deb4f4 
Signed-off-by: Xintao <hunterlxt@live.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@spencerkimball @igorcanadi @siying