Skip to content

Commit

Permalink
refactor: remove MBox for tsk_treeseq_t storage (#306)
Browse files Browse the repository at this point in the history
  • Loading branch information
molpopgen authored Jul 31, 2022
1 parent 0877353 commit 78dda67
Showing 1 changed file with 35 additions and 13 deletions.
48 changes: 35 additions & 13 deletions src/trees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ use crate::TreeSequenceFlags;
use crate::TskReturnValue;
use crate::TskitTypeAccess;
use crate::{tsk_id_t, tsk_size_t, TableCollection};
use ll_bindings::{tsk_tree_free, tsk_treeseq_free};
use mbox::MBox;
use ll_bindings::tsk_tree_free;
use std::ptr::NonNull;

/// A Tree.
Expand Down Expand Up @@ -165,13 +164,36 @@ impl streaming_iterator::DoubleEndedStreamingIterator for Tree {
/// let treeseq = tables.tree_sequence(tskit::TreeSequenceFlags::default()).unwrap();
/// ```
pub struct TreeSequence {
pub(crate) inner: MBox<ll_bindings::tsk_treeseq_t>,
pub(crate) inner: ll_bindings::tsk_treeseq_t,
}

impl crate::ffi::WrapTskitType<ll_bindings::tsk_treeseq_t> for TreeSequence {
fn wrap() -> Self {
let inner = std::mem::MaybeUninit::<ll_bindings::tsk_treeseq_t>::uninit();

This comment has been minimized.

Copy link
@juntyr

juntyr Aug 1, 2022

Contributor

Where is the tree sequence C struct initialised? Using MaybeUninit::uninit().assume_init() in this case just boils down to allowing uninit bytes, which would be undefined behaviour, I think (and a tool like Miri should catch that)

This comment has been minimized.

Copy link
@molpopgen

molpopgen Aug 1, 2022

Author Member

It gets initialized in the new function. The C API requires that we support uninitialized tree sequences, else simplification will leak memory.

This comment has been minimized.

Copy link
@juntyr

juntyr Aug 1, 2022

Contributor

In that case, is there a way to stay inside a MaybeUninit while the value is uninitialised?

This comment has been minimized.

Copy link
@molpopgen

molpopgen Aug 1, 2022

Author Member

Yes, but that breaks the trait API. I'd have to either repeat code or make a new function to do that. I'll probably do the latter before releasing 0.10.0. I'm thinking that the trait itself has to go away, due to some of the vagaries of the C API (sometimes you cannot have an initialized variable). That'd be a breaking change, but not one that affects any downstream code at the moment--the trait is only used for setting up the rust types.

This comment has been minimized.

Copy link
@molpopgen

molpopgen Aug 1, 2022

Author Member

I should note that even with MBox, this function returned uninitialized structures. There's no change here in that regards. We are just putting things on the stack rather than the heap.

This comment has been minimized.

Copy link
@molpopgen

molpopgen Aug 1, 2022

Author Member

Oh -- the trait is actually private, so I can get rid of it. Forgot about that!

let inner = unsafe { inner.assume_init() };
Self { inner }
}
}

unsafe impl Send for TreeSequence {}
unsafe impl Sync for TreeSequence {}

build_tskit_type!(TreeSequence, ll_bindings::tsk_treeseq_t, tsk_treeseq_free);
impl TskitTypeAccess<ll_bindings::tsk_treeseq_t> for TreeSequence {
fn as_ptr(&self) -> *const ll_bindings::tsk_treeseq_t {
&self.inner
}

fn as_mut_ptr(&mut self) -> *mut ll_bindings::tsk_treeseq_t {
&mut self.inner
}
}

impl Drop for TreeSequence {
fn drop(&mut self) {
let rv = unsafe { ll_bindings::tsk_treeseq_free(&mut self.inner) };
assert_eq!(rv, 0);
}
}

impl TreeSequence {
/// Create a tree sequence from a [`TableCollection`].
Expand Down Expand Up @@ -475,7 +497,7 @@ impl TreeSequence {
let timestamp = humantime::format_rfc3339(std::time::SystemTime::now()).to_string();
let rv = unsafe {
ll_bindings::tsk_provenance_table_add_row(
&mut (*(*self.inner).tables).provenances,
&mut (*self.inner.tables).provenances,
timestamp.as_ptr() as *mut i8,
timestamp.len() as tsk_size_t,
record.as_ptr() as *mut i8,
Expand All @@ -496,37 +518,37 @@ impl TryFrom<TableCollection> for TreeSequence {

impl TableAccess for TreeSequence {
fn edges(&self) -> EdgeTable {
EdgeTable::new_from_table(unsafe { &(*(*self.inner).tables).edges })
EdgeTable::new_from_table(unsafe { &(*self.inner.tables).edges })
}

fn individuals(&self) -> IndividualTable {
IndividualTable::new_from_table(unsafe { &(*(*self.inner).tables).individuals })
IndividualTable::new_from_table(unsafe { &(*self.inner.tables).individuals })
}

fn migrations(&self) -> MigrationTable {
MigrationTable::new_from_table(unsafe { &(*(*self.inner).tables).migrations })
MigrationTable::new_from_table(unsafe { &(*self.inner.tables).migrations })
}

fn nodes(&self) -> NodeTable {
NodeTable::new_from_table(unsafe { &(*(*self.inner).tables).nodes })
NodeTable::new_from_table(unsafe { &(*self.inner.tables).nodes })
}

fn sites(&self) -> SiteTable {
SiteTable::new_from_table(unsafe { &(*(*self.inner).tables).sites })
SiteTable::new_from_table(unsafe { &(*self.inner.tables).sites })
}

fn mutations(&self) -> MutationTable {
MutationTable::new_from_table(unsafe { &(*(*self.inner).tables).mutations })
MutationTable::new_from_table(unsafe { &(*self.inner.tables).mutations })
}

fn populations(&self) -> PopulationTable {
PopulationTable::new_from_table(unsafe { &(*(*self.inner).tables).populations })
PopulationTable::new_from_table(unsafe { &(*self.inner.tables).populations })
}

#[cfg(any(feature = "provenance", doc))]
fn provenances(&self) -> crate::provenance::ProvenanceTable {
crate::provenance::ProvenanceTable::new_from_table(unsafe {
&(*(*self.inner).tables).provenances
&(*self.inner.tables).provenances
})
}
}
Expand Down

0 comments on commit 78dda67

Please sign in to comment.