Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support pinnable slice get #1888

Merged
merged 4 commits into from
Nov 26, 2023
Merged

Conversation

chrisxu333
Copy link
Contributor

@chrisxu333 chrisxu333 commented Nov 9, 2023

This closes #1722
Support Get with rocksdb::PinnableSlice, so that relatively large objects like Bitmap and BloomFilter could avoid in-memory copy.

@chrisxu333 chrisxu333 force-pushed the pinnable_slice_support branch from e7794a3 to 021ac77 Compare November 11, 2023 20:34
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

General LGTM! Looking forward testing here!

@chrisxu333 chrisxu333 force-pushed the pinnable_slice_support branch from 0541850 to 84fe581 Compare November 17, 2023 19:15
@PragmaTwice
Copy link
Member

CI failed on macOS and ASAN build (use-after-free behaviors are found).

@chrisxu333
Copy link
Contributor Author

CI failed on macOS and ASAN build (use-after-free behaviors are found).

Okay I went through the code and I thought it might be related to pinnable slice copy into bf_data_list container.

@chrisxu333 chrisxu333 force-pushed the pinnable_slice_support branch from 88618d3 to dd5b5ae Compare November 19, 2023 20:10
@chrisxu333 chrisxu333 force-pushed the pinnable_slice_support branch 2 times, most recently from c80a370 to 899f79c Compare November 23, 2023 18:55
@chrisxu333
Copy link
Contributor Author

Hi @mapleFU , I saw all tests are passed for this pr. So do you mind helping merge this pr after I rebase it with the unstable branch

@mapleFU
Copy link
Member

mapleFU commented Nov 23, 2023

Generall LGTM. I'll take a carefully pass before merging tomorrow

mapleFU
mapleFU previously approved these changes Nov 24, 2023
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Will merge in one day if CI passed and other reviewer think it's ok

@git-hulk git-hulk merged commit 5a4ad7f into apache:unstable Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Get with rocksdb::PinnableSlice to decrease copy for BLOB
4 participants