Do not allow unused trailing bytes in BAM records #71
Conversation
Previously, the .data field of BAM records could have unused bytes near the end. This is acceptable, because the length of the actually used bytes is known from the fixed fields of the record. It had the advantage of preventing frequent resizes, which was slow in Julia. However, from Julia 1.11 onwards, resizing is fast. Promising no non-coding bytes in the BAM records have the following advantages: * It may simplify some code in XAM * It allows an immutable struct holding a reference to the vector to resize the vector, signalling a change in the variable-length data.
| @@ -20,7 +20,7 @@ mutable struct Record <: XAMRecord | |||
| next_refid::Int32 | |||
| next_pos::Int32 | |||
| tlen::Int32 | |||
| # variable length data | |||
| # This vector never has unused bytes at the end | |||
There was a problem hiding this comment.
This comment may be confusing outside the context of the previous design, maybe
| # This vector never has unused bytes at the end | |
| # This vector used to have unused bytes at the end, but no longer does |
or something?
There was a problem hiding this comment.
The original comment intended to distinguish the fields that could vary in length from those that do not. From that standpoint, it does not need changing. If you'd like to add a comment specific to a field, perhaps it should go on the same line as the field.
|
LGTM. Do we also want to change the julia compat with this change to be 1.11+? I suppose it works with older versions, but may be slower? |
| @@ -20,7 +20,7 @@ mutable struct Record <: XAMRecord | |||
| next_refid::Int32 | |||
| next_pos::Int32 | |||
| tlen::Int32 | |||
| # variable length data | |||
| # This vector never has unused bytes at the end | |||
There was a problem hiding this comment.
The original comment intended to distinguish the fields that could vary in length from those that do not. From that standpoint, it does not need changing. If you'd like to add a comment specific to a field, perhaps it should go on the same line as the field.
| @@ -41,7 +41,7 @@ function Base.convert(::Type{Record}, data::Vector{UInt8}) | |||
| record = Record() | |||
| dst_pointer = Ptr{UInt8}(pointer_from_objref(record)) | |||
| unsafe_copyto!(dst_pointer, pointer(data), FIXED_FIELDS_BYTES) | |||
| dsize = data_size(record) | |||
| dsize = record.block_size - FIXED_FIELDS_BYTES + sizeof(record.block_size) | |||
There was a problem hiding this comment.
This calculation occurs more than once. It deserves its own function, which may be separate from data_size.
Previously, the .data field of BAM records could have unused bytes near the end. This is acceptable, because the length of the actually used bytes is known from the fixed fields of the record.
It had the advantage of preventing frequent resizes, which was slow in Julia. However, from Julia 1.11 onwards, resizing is fast. Promising no non-coding bytes in the BAM records have the following advantages:
This change is not user-facing and should be ready to merge.