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

MergeFn that returns &[u8] and reduce copy? #410

Closed
xuchen-plus opened this issue Apr 23, 2020 · 2 comments · Fixed by #457
Closed

MergeFn that returns &[u8] and reduce copy? #410

xuchen-plus opened this issue Apr 23, 2020 · 2 comments · Fixed by #457

Comments

@xuchen-plus
Copy link
Contributor

In my case, I need to merge values with flatbuffers builder, which returns &[u8] that refers to inner object of the builder.
In order to pass this &[u8] to rocksdb, I have to copy it to a new Vec<[u8]>, which will be copied again in the full_merge_callback, and will be copied once more by rocksdb itself.
In this case, the buffer would be copied 3 times.
It would be better to allow MergeFn return just &[u8], and because rocksdb would immediately copy it into its memtable in an synchrounous way, the memory should be safe, IMHO.

@vitvakatu
Copy link

Hello

and because rocksdb would immediately copy it into its memtable in an synchrounous way, the memory should be safe, IMHO.

I don't think it is true. @aleksuss looks like UB, huh?

@xuchen-plus
Copy link
Contributor Author

Hello

and because rocksdb would immediately copy it into its memtable in an synchrounous way, the memory should be safe, IMHO.

I don't think it is true. @aleksuss looks like UB, huh?

Hi, thanks for your reply. Based on my reading on the rocksdb's C API impl (https://github.com/facebook/rocksdb/blob/master/db/c.cc#L370), the returned raw buffer will be copied into the MergeOperationOutput::new_value field (which is a std::string) immediately.

rleungx pushed a commit to rleungx/rust-rocksdb that referenced this issue Jun 17, 2020
Signed-off-by: Yi Wu <yiwu@pingcap.com>
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 a pull request may close this issue.

2 participants