`json+struct` codec should provide a memory alignment guarantee for the binary component

[bhaller]

Making a new issue for this at the request of @jeromekelleher since #3306 was merged and closed. Tagging @petrelharp since he'll be a client of this API too, in pyslim.

Copy-paste from #3306:

One more thing that occurs to me @benjeffery: it would perhaps be nice to provide a memory alignment guarantee for the binary portion (assuming that the metadata chunk as a whole is at an aligned address, which is guaranteed by malloc). On my end, when writing out the metadata, I could of course insert null bytes in between the JSON and the binary to align the binary portion; but this gets tricky because those null bytes would have to be described in the binary schema, and the number of null bytes is variable since the length of the JSON string is variable, so it makes everything a bit of a mess. (I guess SLiM would have eight different possible binary schemas that it uses, with from 0 to 7 null bytes at the start of the binary data as needed, eww.) I could put spaces at the end of the JSON string instead to produce alignment, but I'm worried that that would make the JSON string non-canonical and make tskit produce an error, so I think (?) that solution is out.

Having an alignment guarantee (I'd suggest 8-byte) would be nice because SLiM will be writing and reading lots of data directly to/from the binary blob, so if it's unaligned it just makes for additional headaches and lower performance in the read/write code. If this alignment guarantee were built into the format expected by the json+struct codec, it would smooth the path. Thoughts?

Addendum now:

I've implemented my own alignment bytes for the binary blob in SLiM, and it was indeed a pain in the tuchus. It requires eight different versions of the binary schema, specifying the number of alignment bytes used as being from 0 to 7 (to produce 8-byte alignment). Lots of extra faffing about at both write and read time, which pyslim and anybody else who cares will have to duplicate. I strongly recommend that automatic 8-byte alignment be added to this codec so clients don't have to worry about all this.

[jeromekelleher]

This is a good point, thanks @bhaller. I guess the simplest approach would be to provide some functionality in the schema that would add the necessary padding to align each overall record and subrecord on 8 byte boundaries? So by default it's packed tightly, but if you ask for this padding it gets put in?

[bhaller]

This is a good point, thanks @bhaller. I guess the simplest approach would be to provide some functionality in the schema that would add the necessary padding to align each overall record and subrecord on 8 byte boundaries? So by default it's packed tightly, but if you ask for this padding it gets put in?

That's not what I would recommend, no; that seems like big overkill, and I'm not sure it's a good idea anyway since, to solve the problem that exists here with the memory layout of this codec, the memory layout described by a schema would have to be dependent upon the actual memory address at which the schema is being read/written from; that seems like a BIG potential can of worms, and not at all a good idea. Clients know the memory layout of their own struct records, and can add the appropriate padding bytes in their schema if they want them; there is no need for further support for padding in the schema architecture. The issue is just that the number of bytes occupied by the JSON portion of this schema is variable, and leaves you at an unaligned address. We just need a mechanism for padding after that variable-length JSON.

I don't think this is a schema issue at all, I think it is a codec issue. At present this codec is defined as having a specific memory layout: magic bytes, header, JSON, binary. I am just proposing that that be modified to be: magic bytes, header, JSON, padding, binary, where "padding" is defined to be: n unused bytes following the JSON data, where n is an integer in [0, 7] such that the total number of bytes occupied by "magic bytes, header, JSON, padding" is evenly divisible by 8, thus aligning the start of the binary blob to an 8-byte boundary relative to the beginning of the codec's data. These padding bytes would not be specified by any schema, since the internal memory structure of the codec itself is not specified by a schema. They would just be a part of the (publicly specified) definition of the codec. Since writing data for this codec is not, and should not be, packaged up in a function in tskit like tsk_json_struct_metadata_set_blob(), the writing end of complying with this specification would be handled by the client (as it is now). All that is needed is for the reading end of complying with this specification to be handled by the client, by changing tsk_json_struct_metadata_get_blob() to expect and skip over those padding bytes (and corresponding changes to the test code). This should simply be built into the specification of this codec, and not be optional or user-controllable in any way.

[petrelharp]

We're working on this, FYI.