Skip to content

Allow for standalone tables #262

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

Closed
molpopgen opened this issue Jul 16, 2022 · 8 comments
Closed

Allow for standalone tables #262

molpopgen opened this issue Jul 16, 2022 · 8 comments
Milestone

Comments

@molpopgen
Copy link
Member

We currently only define table types as immutable
views of data owned by table collections/tree sequences.

To support standalone tables:

  • Define a new type, OwningXTable for row type X.
  • impl std::ops::Deref for each new type.
    The deref target is XTable.
  • The public API is add_row and add_row_with_metadata.
  • Probably also need copy function.

Thanks to @momolangenstein for suggesting the Deref pattern.

@molpopgen
Copy link
Member Author

After a bit of hacking:

  • This is much harder than I'd thought!
  • The lifetime MutationTable<'a> is the culprit.
  • One path may be to try to get rid of that lifetime,
    but then we probably want to refactor TableCollection
    to ensure that lifetimes are properly coupled.

@molpopgen
Copy link
Member Author

It is looking like this requires some significant
back end changes.

  • change metadata-related macros to work on pointers
    to tables rather than on references.

  • change all table types to store pointers rather than reference.
    This change allows dropping lifetime generics from the table types.

  • work out implications for code like this:

    let m = tables.mutations();
    drop(tables);
    m.num_rows(); // currently doesn't compile due to lifetime on tables

@molpopgen
Copy link
Member Author

See comments over in #265 re: a plan of action.

@molpopgen
Copy link
Member Author

#265 has a working pattern now.

There is no need for a new type abstracting out "do we own the pointer or not".
Rather, standalone tables contain both an MBox and an instance of the immutable table type.

There are, however, several side effects that affect the back end:

  • Initialization order of the C types becomes tricky.
    (This is not unexpected -- these are C structs w/many fields.)
  • Our current use of macros to "easily" build the rust
    wrappers for types will break.
    This is not a big deal -- the manual implementations are easy.
  • Our current table types do not allow initialization with
    std::ptr::null().
    To keep memory safety in place and to make the new code sane,
    these tables should get a new pub(crate) new_null() -> Self
    function that wraps a null pointer and a pub(crate) set_pointer(...)
    to set the pointer as needed.

Yay, progress.

@juntyr
Copy link
Contributor

juntyr commented Jul 18, 2022

Here are some further design options:

// START OF PRELUDE

use std::ops::Deref;
use std::ptr::NonNull;

mod ll_bindings {
    pub struct tsk_edge_table_t {}
    pub struct tsk_table_collection_t {
        pub edges: tsk_edge_table_t,
    }
}
type MBox<T> = Box<T>;

struct TableCollection {
    inner: MBox<ll_bindings::tsk_table_collection_t>,
}

struct EdgeTable<'a> {
    table_: &'a ll_bindings::tsk_edge_table_t,
}

trait TableAccess {
    fn edges(&self) -> EdgeTable;
}

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

impl<'a> EdgeTable<'a> {
    fn new_from_table(edges: &'a ll_bindings::tsk_edge_table_t) -> Self {
        EdgeTable { table_: edges }
    }
}

// END OF PRELUDE

// Option 1

struct OwnedEdgeTableV1 {
    edges: MBox<ll_bindings::tsk_edge_table_t>,
}

impl OwnedEdgeTableV1 {
    pub fn borrow(&self) -> EdgeTable<'_> { // returns EdgeTableV2<'a>
        EdgeTable::new_from_table(&self.edges)
    }
}

// Option 2

struct OwnedEdgeTableV2 {
    edges: MBox<ll_bindings::tsk_edge_table_t>,
}

#[repr(transparent)] // needed so the transmute on the newtype is safe
struct EdgeTableV2<'a> {
    table_: &'a ll_bindings::tsk_edge_table_t,
}

impl Deref for OwnedEdgeTableV2 {
    type Target = EdgeTableV2<'static>;
    
    fn deref(&self) -> &Self::Target { // returns &'a EdgeTableV2<'static>
        unsafe { std::mem::transmute(&self.edges) }
    }
}

// Option 3

#[repr(transparent)]
struct EdgeTableV3 {
    table_: NonNull<ll_bindings::tsk_edge_table_t>,
}

impl EdgeTableV3 {
    // this would be just for convenience to hide the unsafe
    /* private*/ fn as_ref(&self) -> &ll_bindings::tsk_edge_table_t {
        // Safety: only constructed with ref
        unsafe { self.table_.as_ref() }
    }
    
    pub(crate) fn ref_from_ref<'a>(edges: &'a ll_bindings::tsk_edge_table_t) -> &'a Self {
        // Safety: NonNull<T> has the layout of *mut T,
        //         matching the layout of *const T,
        //         matching the layout of &T
        unsafe { std::mem::transmute(edges) }
    }
}

struct OwnedEdgeTableV3 {
    edges: MBox<ll_bindings::tsk_edge_table_t>,
}

impl Deref for OwnedEdgeTableV3 {
    type Target = EdgeTableV3;
    
    fn deref(&self) -> &Self::Target { // returns &'_ EdgeTableV3
        EdgeTableV3::ref_from_ref(&self.edges)
    }
}

trait TableAccessV3 {
    fn edges(&self) -> &EdgeTableV3;
}

impl TableAccessV3 for TableCollection {
    fn edges(&self) -> &EdgeTableV3 { // returns &'_ EdgeTableV3
        EdgeTableV3::ref_from_ref(&(*self.inner).edges)
    }
}

@molpopgen
Copy link
Member Author

thanks @MomoLangenstein !

@molpopgen
Copy link
Member Author

The pattern 2 from @momolangenstein is implemented in #267.
This approach is definitely low-friction.

@molpopgen molpopgen added this to the 0.10.0 milestone Jul 18, 2022
@molpopgen
Copy link
Member Author

Closed by:

#269
#278
#279
#282
#268
#267
#273
#281
#283
#284
#285
#286

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

No branches or pull requests

2 participants