tikv / rust-rocksdb

rust wrapper for rocksdb
Apache License 2.0
276 stars 155 forks source link

port post write callback #731

Closed tabokie closed 1 year ago

tabokie commented 1 year ago

Compared to https://github.com/BusyJay/tirocks/pull/24, it uses C assertion for memory safety.

Signed-off-by: tabokie xy.tao@outlook.com

BusyJay commented 1 year ago

I think you can copy the definition from tirocks and imiplement it the same way.

tabokie commented 1 year ago

@BusyJay In this repo the C bindings are generated by hand. C++ bindings are too complicated to be done this way.

BusyJay commented 1 year ago

What matters is just the align and size. The definition you need are just:

#[repr(C)]
pub struct rocksdb_PostWriteCallback__bindgen_vtable(libc::c_void);
#[repr(C)]
pub struct rocksdb_PostWriteCallback {
    pub vtable_: *const rocksdb_PostWriteCallback__bindgen_vtable,
}
#[repr(C)]
pub struct SimplePostWriteCallback {
    pub _base: rocksdb_PostWriteCallback,
    pub state: *mut libc::c_void,
    pub cb: ::std::option::Option<
        unsafe extern "C" fn(arg1: *mut libc::c_void, arg2: u64),
    >,
}
pub type on_post_write_callback_cb = ::std::option::Option<
    unsafe extern "C" fn(arg1: *mut libc::c_void, arg2: u64),
>;
extern "C" {
    pub fn crocksdb_simple_post_write_callback_init(
        callback: *mut SimplePostWriteCallback,
        state: *mut libc::c_void,
        cb: on_post_write_callback_cb,
    );
}
tabokie commented 1 year ago

No, I don't trust anyone to review this code without looking at the generated version from tirocks.

BusyJay commented 1 year ago

The code is just copied from tirocks.

tabokie commented 1 year ago

Exactly. The idea of using generated code from an unrelated repo as the review basis is unheard of. Let alone there's the potential hazard of extending this code pattern towards other more complex virtual classes that no one can tell if it's strictly correct without using tirocks to generate them first.

BusyJay commented 1 year ago

I'm not suggesting change all the codes but only this struct. The motivation is to reduce the cost before we actually landing tirocks. If you can prove this won't hurt performance, I'm also OK with this implementation.

tabokie commented 1 year ago

Added a stack version that uses runtime checks for correctness. Microbench shows the heap version has a stable 70ns overhead, no overhead from the stack version.