Documentation for the internal rules governing allocation of table columns?

[bhaller]

Hi! Asked this to Ben J. on Slack, and he suggested I open a Discussion, so here I am. :->

Are the exact rules governing the internal allocation sizes of table columns in tskit spelled out somewhere? I am having to replace a table column in a node table with a new malloced block, because I’m having to rewrite all the metadata for the table in a certain situation, and the length (in bytes, not rows) is changing. I’m wondering what the safe/correct way to do that is. I’m having to set metadata_length and max_metadata_length, and I’m a bit nervous that if I don’t do things in exactly the way tskit expects, it will end up causing a crash in one way or another. I’m particularly unsure of the rules for the metadata_offset column, and what allocation size it is guaranteed to have, since I guess it always uses one slot more than num_rows; if there are 10 rows then there are 11 values in metadata_offset, as I understand it, the first of which is guaranteed to be zero. Does that mean that it is guaranteed to have one more slot allocated in it than max_rows? If max_rows is 10, then metadata_offset is guaranteed to have enough space allocated for 11 values? Or how exactly does that work? Anyhow, if you can point me towards guidance regarding the correct way to do this, I’d appreciate it. Thanks!

[bhaller]

Ben J. replied:

Can you not ask tskit to resize the block (e.g tsk_individual_table_expand_metadata) then assign into it rather than doing it yourself? If you can't do that then as long as max_metadata_length is the length of the buffer and metadata_length the length of the buffer that is used for data then all is good. As long as max_metadata_length >= metadata_length it doesn't matter the exact value.

And yes any _offset column is one bigger than max_rows.

[bhaller]

Hmm, perhaps I can use tsk_individual_table_expand_metadata(). The new metadata depends on the old metadata, so I'd need to copy off the old data into a side malloced block, then call tsk_individual_table_expand_metadata() and fix it, I guess. But all of the offsets are changing too. Seemed easier to just munge things to be the way I want them to be, but I'll look further at tsk_individual_table_expand_metadata() etc. to make sure.

As for the size of the offset column's malloced block, OK, good to know that I'm safe if I malloc a new one that has room for max_rows + 1. Is that rule written down anywhere, and the rule that the first offset value is always 0 (instead of the constant 0 value being missing, and the number of rows equalling max_rows, which would be the other obvious way to implement things)? Would be nice to have a few more comments in the header about how such things work, unless it's already there and I missed it.

[benjeffery]

You could also use the set_columns methods after getting everything how you want it. If possible using the documented API will give a zero chance of future breakage.

Is that rule written down anywhere

We wouldn't ever document that in the text documentation as we don't want to make forward promises as to how internal things such as this work, I guess we could put something in the header, but I'd be against that as it just has the possibility of getting out of date. The code is quite clear on the how big tables are when they are allocated as it all happens in the tsk_X_table_expand_main_columns and all the ragged ones use expand_ragged_column.

the constant 0 value being missing, and the number of rows equalling max_rows, which would be the other obvious way to implement things

While that way might be simpler on the allocation side, it complicates usage as you can't just do metadata[metadata_offset[i]].

[jeromekelleher]

I'm with @benjeffery here - documenting this sort of low-level implementation detail would mean freezing the implementation forever.

[bhaller]

Interesting; I guess we have different philosophies on that. To me, stating something in the actual documentation is a promise, unless you say "this might change in future" or some such. But a comment in the code is just a comment in the code – and commenting code is good. I don't regard code comments as promises. Vive la difference! :-> Anyhow, thanks for the advice above, I'll do some head-scratching and see if I can do it using the existing functionality you mention!

[jeromekelleher]

Like Ben J says, comments go out of date, and an inaccurate comment is much worse than no comment.

[jeromekelleher]

Consider future scenario, where someone new comes along with a great idea for improving table memory allocation. They change the code and make things much better. Tests pass, so they open a PR. Overstretched maintainer Y looks at the code and says "hey, this is great!", and because the comment describing old behaviour isn't in the diff for the change, it's forgotten about. Changes get shipped, it's not considered an externally breaking change so CHANGELOG isn't updated, comments are out of date, and sometime later SLiM gets broken in an obscure way.

[bhaller]

If that scenario really worries you, than I'd say (1) date your comments so it can be seen how old they are (providing some hint as to likely accuracy), and (2) mention variable names explicitly in comments, if you're commenting on the role/function/design of a given variable; and when you make a breaking change like this in something that's visible in the header, change the variable name. Then if the comment fails to get changed, the end user can clearly see that the comment refers to something obsolete. I love using a variable name change when making a breaking change; it's a really useful technique. Also, (3) when providing brief information on a variable, like "// contains one more row than max_rows", put that comment on the same line as the declaration of the variable itself; then it is always in the diffs for any change, and its visibility means that it will be kept up to date anyway. And also, (4) If a new diff makes a breaking change in the underlying implementation of something, a new comment should always be written at a key spot in the code (like where the variables that control the feature are declared) that describes the change that was made, and has a date on it. So even if there's an older comment there that doesn't get edited correctly, the new comment makes the current state of affairs crystal clear. All of those techniques would guard against the scenario you describe.

I'd offer the counter-scenario. Code is totally uncommented, and so the user interacting with that code has to guess at the design. (As I have to guess whether the first offset row is 0 with max_rows + 1 rows total, or the zero is implied, with max_rows rows total - and that's a pretty fundamental thing to have to guess about!) They make a guess, which might be correct or not. If not, they already have a bug. If they're correct, but then the code changes later on and their guess becomes wrong, their code breaks – and there's not even a change to a comment to give them a hint about what happened when their code stops working. To me, that is a much more likely scenario than the one you describe! Indeed, it's the scenario I'm worried about here (so hopefully I can figure out how to do what I want to do using the provided APIs).

But, like I said, we just have different philosophies on this. I comment my code up the wazoo, and I find it extremely helpful even though I'm mostly the only person in SLiM's code; my own comments often help me figure things out later, sometimes years down the road. It means that I spend a little extra time writing and maintaining the comments themselves, and no doubt there are some obsolete comments in the code (which I fix when I notice them), so there are pros and cons, but to me, it is absolutely worth the effort; it pays big dividends for me every day. And if I go kaput eventually, as I no doubt will, and somebody else inherits the SLiM code base, I suspect they will be very, very happy that I made the effort to comment the code! :-> But you guys are working on a framework used in other people's code (which SLiM is not, in that sense), so I certainly get that some practices need to change because your headers are much more public. If it were me, though, I'd still comment them – just perhaps more carefully and sparsely than I comment SLiM's code. :->