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

Rust wrapper for SplinterDB #583

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Rust wrapper for SplinterDB #583

wants to merge 11 commits into from

Conversation

etwest
Copy link
Contributor

@etwest etwest commented Jun 6, 2023

A rust wrapper for SplinterDB. Implements simple key/value operations by default and allows for more complicated update behavior with user defined rust functions.

The wrapper is an iterative improvement to a previous version created by @rosenhouse.

Changes from the previous version of the wrapper:

  • Automatic splinter binding (splinterdb-sys) generation when building the rust library.
  • More tests of the rust wrapper (splinterdb-rs).
  • Support for new data_config layout and other changes to SplinterDB.
  • Ability to callback to rust functions via the SplinterDB data_config. These callbacks are defined by the SdbRustDataFuncs trait.

@netlify
Copy link

netlify bot commented Jun 6, 2023

Deploy Preview for splinterdb canceled.

Name Link
🔨 Latest commit 9d10ce8
🔍 Latest deploy log https://app.netlify.com/sites/splinterdb/deploys/64cd27321fc9cb0008957d1c

@etwest
Copy link
Contributor Author

etwest commented Jun 6, 2023

This pull request does not yet incorporate the rust wrapper tests into the SplinterDB tests. @rtjohnso suggested making the tests check if the user has installed rust, and if so, to perform the rust wrapper tests.

@etwest etwest requested review from rtjohnso and ajhconway August 3, 2023 19:51
self.sdb_cfg.filename = path.as_ptr();
self.sdb_cfg.cache_size = cfg.cache_size_bytes as u64;
self.sdb_cfg.disk_size = cfg.disk_size_bytes as u64;
self.sdb_cfg.data_cfg = &mut self.data_cfg;

Choose a reason for hiding this comment

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

This line creates a self referential struct, which is not move safe. We should either make the struct pinned, or move self.data_cfg to heap, e.g., Box<data_cfg>.

The bug can be reproduced by first creating a SpinterDB instance, then assign (move) it to a different variable.
The self.sdb_cfg.data_cfg would still point to the old address which is no longer valid.

Sorry for jumping into this pull request. I'm looking for ways to use SplinterDB in Rust and can't wait to try this wrapper :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Xiangpeng,

Thank you for catching this! Would you be willing to add a commit to this PR fixing the bug? I think for now pinning it would be most in the spirit of how this struct should be used. But I'm open to your suggestions, since I'm not a rust expert.

Choose a reason for hiding this comment

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

Sure I'm happy to fix the bug. It seems I don't have push permission on the pull request. Below is my patch, feel free to append it to the pull request!

I go with the second option as pinning on the struct requires quite a bit change on how we use the wrapper, while boxing the variable is more straightforward to me (impose no runtime overhead as well).

From 4857a9898cded6a8704b711e08455ea0daa292d8 Mon Sep 17 00:00:00 2001
From: Xiangpeng <[email protected]>
Date: Mon, 15 Jan 2024 15:00:29 -0700
Subject: [PATCH] box the data_cfg to prevent self referential struct

---
 rust/splinterdb-rs/src/lib.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rust/splinterdb-rs/src/lib.rs b/rust/splinterdb-rs/src/lib.rs
index 3eab876..16fe991 100644
--- a/rust/splinterdb-rs/src/lib.rs
+++ b/rust/splinterdb-rs/src/lib.rs
@@ -18,7 +18,7 @@ pub struct DBConfig {
 pub struct SplinterDB {
     _inner: *mut splinterdb_sys::splinterdb,
     sdb_cfg: splinterdb_sys::splinterdb_config,
-    data_cfg: splinterdb_sys::data_config,
+    data_cfg: Box<splinterdb_sys::data_config>,
 }
 
 unsafe impl Sync for SplinterDB {}
@@ -192,7 +192,7 @@ impl SplinterDB
         SplinterDB {
             _inner: std::ptr::null_mut(),
             sdb_cfg: unsafe { std::mem::zeroed() },
-            data_cfg: new_sdb_data_config::<T>(0),
+            data_cfg: Box::new(new_sdb_data_config::<T>(0)),
         }
     }
 
@@ -208,7 +208,7 @@ impl SplinterDB
         self.sdb_cfg.filename = path.as_ptr();
         self.sdb_cfg.cache_size = cfg.cache_size_bytes as u64;
         self.sdb_cfg.disk_size = cfg.disk_size_bytes as u64;
-        self.sdb_cfg.data_cfg = &mut self.data_cfg;
+        self.sdb_cfg.data_cfg = self.data_cfg.as_mut();
 
         // set key bytes
         self.data_cfg.max_key_size = cfg.max_key_size as u64;
-- 
2.34.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants