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

c ffi #39

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ keywords = ["trie", "patricia", "collection", "generic", "prefix"]
categories = ["data-structures", "text-processing"]

[dependencies]
libc = "0.2"
Copy link
Owner

Choose a reason for hiding this comment

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

Just a small thing, but it would be good if this was an optional dependency behind a feature flag, see: https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What feature name would you suggest?

Copy link
Owner

Choose a reason for hiding this comment

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

ffi or c-bindings or c_bindings would probably be good

nibble_vec = "~0.0.3"
"endian-type" = "0.1.2"
serde = { version = "1.0", optional = true }
Expand All @@ -23,5 +24,8 @@ quickcheck = "0.4"
rand = "0.3"
serde_test = "1.0"

[lib]
crate-type = ["lib", "cdylib"]

[badges]
travis-ci = { repository = "michaelsproul/rust_radix_trie" }
34 changes: 34 additions & 0 deletions src/c_ffi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use libc::{c_char,size_t};
use std::ffi::CStr;
use std::vec;

use super::Trie;
use super::trie_common::TrieCommon;

ffi_fn! {
fn radix_trie_create()-> *const Trie<Vec<u8>, usize>{
let trie = Trie::<Vec<u8>, usize>::new();
return Box::into_raw(Box::new(trie));
}
}

ffi_fn! {
fn radix_trie_free(trie_ptr: *const Trie<Vec<u8>, usize>){
unsafe { Box::from_raw(trie_ptr as *mut Trie<Vec<u8>, usize>); }
}
}

ffi_fn! {
fn radix_trie_insert(trie_ptr:*const Trie<Vec<u8>, usize>, key_ptr:*const c_char, value:usize){
let mut trie = unsafe { &mut *(trie_ptr as *mut Trie<Vec<u8>, usize>) };
let key = unsafe { CStr::from_ptr(key_ptr) }.to_bytes().to_vec();
Copy link
Owner

@michaelsproul michaelsproul Aug 30, 2018

Choose a reason for hiding this comment

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

We could optimise this (avoid a double allocation), by implementing TrieKey for CStr. Using a Vec<u8> as the key means that it gets cloned each time, see:

impl TrieKey for Vec<u8> {
fn encode_bytes(&self) -> Vec<u8> {
self.clone()
}
}

Copy link
Contributor Author

@hanabi1224 hanabi1224 Aug 30, 2018

Choose a reason for hiding this comment

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

Actually, I did this on purpose to tolerant unexpected string pointer deallocation from foreign languages, especially managed ones whose memory is not being handled explicitly, does that make sense?

Copy link
Owner

@michaelsproul michaelsproul Aug 30, 2018

Choose a reason for hiding this comment

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

Yeah, I agree that we want that behaviour, but the current code copies the string twice. The .to_bytes().to_vec() call will copy everything into a Vec<u8>, and when that Vec<u8> gets inserted in the trie it will get cloned. Instead, if we generate the Vec<u8> in the TrieKey::encode_bytes function, the string will only get copied once, like:

impl TrieKey for CStr {
    fn encode_bytes(&self) -> Vec<u8> {
        self.to_bytes().to_vec()
    }
}

Maybe the compiler is smart enough to optimise the double-copy away, but it doesn't hurt to give it some help (and impl TrieKey for CStr is generally useful anyway)

trie.insert(key, value);
}
}

ffi_fn! {
fn radix_trie_len(trie_ptr:*const Trie<Vec<u8>, usize>)->usize{
let trie = unsafe { &*trie_ptr };
return trie.len();
}
}
5 changes: 5 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ extern crate quickcheck;
#[cfg(test)]
extern crate rand;

extern crate libc;

pub use nibble_vec::NibbleVec;
Copy link
Owner

Choose a reason for hiding this comment

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

Duplicate import (delete this line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

pub use keys::TrieKey;
pub use trie_common::TrieCommon;
Expand All @@ -33,6 +35,9 @@ mod test;
#[cfg(test)]
mod qc_test;

mod c_ffi;
pub use c_ffi::*;

const BRANCH_FACTOR: usize = 16;

/// Data-structure for storing and querying string-like keys and associated values.
Expand Down
38 changes: 38 additions & 0 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,41 @@
macro_rules! id {
($e:item) => { $e }
}

// from https://github.com/rust-lang/regex/blob/master/regex-capi/src/macros.rs
macro_rules! ffi_fn {
(fn $name:ident($($arg:ident: $arg_ty:ty),*,) -> $ret:ty $body:block) => {
ffi_fn!(fn $name($($arg: $arg_ty),*) -> $ret $body);
};
(fn $name:ident($($arg:ident: $arg_ty:ty),*) -> $ret:ty $body:block) => {
#[no_mangle]
pub extern fn $name($($arg: $arg_ty),*) -> $ret {
use ::std::io::{self, Write};
use ::std::panic::{self, AssertUnwindSafe};
use ::libc::abort;
match panic::catch_unwind(AssertUnwindSafe(move || $body)) {
Ok(v) => v,
Err(err) => {
let msg = if let Some(&s) = err.downcast_ref::<&str>() {
s.to_owned()
} else if let Some(s) = err.downcast_ref::<String>() {
s.to_owned()
} else {
"UNABLE TO SHOW RESULT OF PANIC.".to_owned()
};
let _ = writeln!(
&mut io::stderr(),
"panic unwind caught, aborting: {:?}",
msg);
unsafe { abort() }
}
}
}
};
(fn $name:ident($($arg:ident: $arg_ty:ty),*,) $body:block) => {
ffi_fn!(fn $name($($arg: $arg_ty),*) -> () $body);
};
(fn $name:ident($($arg:ident: $arg_ty:ty),*) $body:block) => {
ffi_fn!(fn $name($($arg: $arg_ty),*) -> () $body);
};
}
15 changes: 15 additions & 0 deletions src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ use std::collections::HashSet;
use std::iter::FromIterator;
use {Trie, TrieCommon};
use keys::TrieKey;
use c_ffi::*;
use std::ffi::{CStr,CString};

const TEST_DATA: [(&'static str, u32); 7] = [("abcdefgh", 19),
("abcdef", 18),
Expand Down Expand Up @@ -440,3 +442,16 @@ fn test_prefix() {
assert_eq!(second.prefix(), [0x1].as_ref());
assert_eq!(third.prefix(), [0x2].as_ref());
}

#[test]
fn test_c_ffi_e2e(){
let t = radix_trie_create();
//let key = CString::new("key1").unwrap().as_ptr();
radix_trie_insert(t, CString::new("key1").unwrap().as_ptr(), 1);
radix_trie_insert(t, CString::new("key2").unwrap().as_ptr(), 2);

let len = radix_trie_len(t);
assert_eq!(len, 2);

radix_trie_free(t);
}