Tskit: [C API] Copy columns out of kastore

[jeromekelleher]

Currently when a tsk_table_collection is read from file, it uses the kastore object as the backing memory. That is, we have a self->store pointer which we maintain for the lifetime of the table collection, and the table columns all point directly to the store's memory. This setup was motivated by a desire to use mmap within the kastore and therefore have zero copy semantics all the way down to the kernel for column memory. Turns out this isn't worth the complexity, as mmap use has a few major caveats and the files just aren't large enough to make this sort of optimisation worthwhile.

So, we should move to copying the columns out of the store. We should set this up so that we read the columns into a set of local pointers, and then call (e.g.) tsk_node_table_set_columns with those pointers as arguments. This will take care of all the allocation behaviour, and make sure that the NULL column semantics accepted for set_columns is directly reflected also in the default columns from file. It's probably a straightforward change where we provide pointers to local variables rather than the table columns to read_table_cols, and then call set_columns on those.

What do you think @benjeffery?

[benjeffery]

Yep, sounds good, had using set_columns in mind for this too. Will remove some complexity and doesn't seem a large cost. At a glance it looks like a very simple change.

[petrelharp]

I don't really understand the implications, but was one of the motivations for zero-copy to reduce memory usage?

And: Perhaps you'll give us pointers about how to modify the SLiM code that uses this API when you've got this sorted?

[jeromekelleher]

I don't really understand the implications, but was one of the motivations for zero-copy to reduce memory usage?

It would slightly increase memory usage in the short term as we'll have two copies of the data around while the kastore is open and we're loading in the files. However, I'm hoping we can ultimately avoid copying the table_collection_t object when we're creating a tree_sequence_t, by marking an input table collection as read only and owned by the tree sequence. This will be worth much more in terms of saving memory than the current approach.

And: Perhaps you'll give us pointers about how to modify the SLiM code that uses this API when you've got this sorted?

This will get rid of the need for the current workaround - you can update a read-in set of tables as you like. But yes, I'm very happy to help with updating SLiM to use any changed APIs, just point me in the right direction when the time comes to release the next C API update.

[jeromekelleher]

I'm having a go at this - this weekend is getting very long and I need some C coding therapy!