Skip to content

switch BGZFStreams for BGZFLib - #81

Merged
jakobnissen merged 12 commits into
BioJulia:developfrom
jonathanBieler:develop
Jul 27, 2026
Merged

switch BGZFStreams for BGZFLib#81
jakobnissen merged 12 commits into
BioJulia:developfrom
jonathanBieler:develop

Conversation

@jonathanBieler

@jonathanBieler jonathanBieler commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

I had some time so I've tried to replace BGZFStreams the new libdefalte based BGZFLib. I used Claude for this but it was struggling quite a bit so I also had to contribute as well... The code look ok I think and all tests pass. There's maybe some rough edges (e.g. _to_virtual_offset should be a method in BGZFLib?) but maybe that can be tackled later on, using this PR as a base.

I also added to methods for allocation free sequence and quality extraction.

The performance are pretty good, computing the mean base quality on a 3.0G BAM file, 3x faster, same result :

Before :

29.076704 seconds (2.61 M allocations: 180.409 MiB, 0.10% gc time)
39.3092853294488

After :

10.851930 seconds (3.68 M allocations: 256.426 MiB, 0.31% gc time, 85932 lock conflicts)
39.3092853294488

@kescobo kescobo added the enhancement New feature or request label Jun 12, 2026
@jakobnissen

jakobnissen commented Jun 14, 2026

Copy link
Copy Markdown
Member

This is very exciting! I'll look at it one of the following days when I get time. Some changes might need to be upstreamed to BufferIO.jl or BGZFLib.jl (I'll handle that).

I would prefer the PR is split into one which solely switches the BGZF library.
Could you (perhaps using a coding agent) focus this PR to only include the switching of the underlying BGZF library, and then make another PR, possibly built on top of this one, which adds the new features?

@jonathanBieler

jonathanBieler commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Done, it was easy to separate. I added these to try to reduce allocations to a minimum, I think the BGZFLib version allocates a bit more than BGZFStreams.

matrix:
julia-version:
- '1.6' # LTS
- '1.11' # lowest compatible release

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal to lose the current LTS, but I don't think it's a dealbreaker

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's MemoryViews that requires 1.11

Comment thread docs/src/man/hts-files.md
Comment thread docs/src/man/hts-files.md
Comment thread src/bam/bam.jl Outdated
Comment thread src/bam/overlap.jl
Comment thread src/bam/reader.jl
Comment thread src/bam/writer.jl Outdated
Comment thread test/runtests.jl Outdated
@jakobnissen

Copy link
Copy Markdown
Member

Let me know if you need any help with the PR or implementation

@jonathanBieler

Copy link
Copy Markdown
Contributor Author

I did another pass on this, benchmark still looks good, it allocates more but it's because I'm using the sequence and qualities :

12.025843 seconds (196.71 M allocations: 18.897 GiB, 24.67% gc time, 1953 lock conflicts, 0.26% compilation time)
[ Info: 39.3092853294488

@jakobnissen jakobnissen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're almost done! Just two more bugs I found - then it can be merged.

Comment thread src/bam/reader.jl Outdated
Comment thread src/bam/bam.jl Outdated
@jakobnissen
jakobnissen merged commit bec8714 into BioJulia:develop Jul 27, 2026
12 checks passed
@jakobnissen

Copy link
Copy Markdown
Member

Thank you!

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants