Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flexible array member to the bam1_t struct to facilitate a BAM … #393

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nathanweeks
Copy link
Contributor

Add a flexible array member to the bam1_t struct to allow the variable-length data to be colocated in memory with the rest of the bam1_t. This should be backwards compatible, as the addition of the flexible array member shouldn't alter sizeof(bam1_t) in this case.

This is used by the samtools sort optimization described in samtools/samtools#593

@jkbonfield
Copy link
Contributor

I've done this sort of thing before, it's how "io_lib" works infact. You don't really need a separate variable as b.data = ((char *)&b) + sizeof(b); would work too, although I like the empty array idea better. The bigger problem is the all pervasive alloc/dealloc code of bam1_t structs that assumes it can realloc or free b.data. That's not just htslib, but samtools etc too as there's never really been an API to access these fields. As such it can only be used here and there when we want better optimisation, rather than a generic fix. :(

I fully support making use of reducing malloc overheads. I think we discussed sort in the office once and concluded that it could also be fixed by having one large single malloc block of memory that the data pointers are set to and using a pool-allocator (eg see htslib/cram/pooled_alloc.[ch]) for obtaining the bam1_t structs, turning it into just a handful of mallocs. I don't think anyone coded it up yet though so credit for tackling this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants