Change offset columns to 64 bit.

[jeromekelleher]

Following the discussion in #343, we've decided that metadata and other ragged columns should be 64 bit. This should be done for C API 1.0.

I think we can either change the tsk_size_t typedef, or create a new type tsk_offset_t (or something) which is used for just the offset columns. Let's discuss the pros and cons of that here, as well as the details involved in this transition.

[petrelharp]

With some grep'ping, it doesn't look like there's very many places we use tsk_size_t in arrays that aren't offset columns. I looked for ones that might have performance impacts we might care about, but actually the best candidate is in canonical sort for individuals... but this is no big deal. I think this argues for keeping things simple and not introducing a new one.

[benjeffery]

In terms of readability and maintainability it would be nice to have tsk_offset_t, even if it resolves to the same type as tsk_size_t, although - what is the use case for tsk_size_t if we did that?

[jeromekelleher]

I think we need to clarify what the size type is for in either case. I think a reasonable definition is the size in bytes of any tskit array - in which case, the offset type is redundant, as it should already be 64 bit. Plus, offset types are usually signed, which would further confuse I think (see off_t).

[jeromekelleher]

Another thing to consider - perhaps we should just drop the tsk_size_t typedef entirely and use size_t instead? I've just tried compiling the 64 bit version on my (32 bit) laptop a there's a lot of warnings because size_t is 32 bit there. In this case, what's the point of having a 64 bit tsk_size_t?

I think this is probably the right thing to do actually, the tsk_size_t typedef is superfluous and we should just get rid of it.

[petrelharp]

Sounds plausible to me, but I totally don't know what I'm doing here...

[jeromekelleher]

I've been playing with the various options in some branches, and the short version is, this is easy if we want to just support 64 bit machines, but really quite tricky if we want to support both 32 and 64 bit machines. For arcane reasons, I work on a 32 bit machine reasonably regularly, so I'd really like to be able to keep working on tskit (and downstream projects) on this machine.

Here are the options:

• typedef tsk_size_t to uint64_t. This has many advantages, and makes the code for reasoning about how we handle different sized columns as input quite straightforward. The cons are that lots of noise from the compiler on 32 bit where we're using tsk_size_t as arguments to (e.g.) malloc, because our type is bigger than size_t. Disabling the errors leads to a number of segfaults, but I'm sure these could be fixed. The fix would be put in a cast to (size_t) basically anywhere we calling malloc() etc.

• typedef tsk_size_t to size_t (or, get rid of tsk_size_t and use size_t instead). This is tempting, because there's no point in having 64 bit offset columns on a 32 bit platform. However, it leads to quite convoluted logic on reading in and out data (we'd need functions for casting up/down to/from uint32/64 to size_t), which would need preprocessor logic to implement. It would also be quite tricky to do without assuming that size_t is either 4 bytes or 8 bytes, which is not guaranteed by the stardard - so, we could end up with a lot of #if/else statements lying around, which is never nice. (Also quite hard to test the 32 bit branches of those tests.) That's not to mention the ickiness of this at the Python-C API layer, which would definitely be much more straightforward with fixed size types.

So, having done some tinkering I'm of the opinion that

(a) Replacing tsk_size_t with size_t would be a mistake - fixed with types are a big win.
(b) Following on from this we should typedef tsk_size_t to uint64_t.

It depends on how much people would object to me making the noisy changes required to keep 32 bit platforms up and running, though?

[jeromekelleher]

Hmm, so we could compile the code in 32 bit mode using the gcc-multiarch package and adding the -m32 option to gcc. It's a little bit tricky because we'd also need to install 32 bit versions of things like CUnit, but I'm guessing this can be done and probably wouldn't be too painful if we set a particular CI job up for it.

The question is, should we? Do folks think we should just drop any pretence of supporting 32 bit machines? It's possible that people will want to run tskit in production on 32 bit machines at some point in the future, but beyond my laptop and low end ARM chips like the Raspberry PI, there probably isn't much of it out there any more.

[benjeffery]

Thanks for looking into the options, I agree that using a fixed width avoids a lot of headaches.

I think we need to support 32bit unless it is a large burden, I doubt you are the only one.

For CI I think it is possible to run a 32bit docker container on a 64bit host to avoid special casing the compile and install of 32bit dependencies. E.g. https://hub.docker.com/r/i386/python

[petrelharp]

I think we need to support 32bit unless it is a large burden, I doubt you are the only one.

Again, I totally don't have the big picture here, but I agree with this sentiment.