Add multi-threading to samtools consensus. Needs htslib#1875#2174
Merged
Conversation
c4c747e to
82db445
Compare
Contributor
Author
|
Draft while I do some final checking. Some benchmarks as it currently stands: Corresponding to 13x speed up with 16 threads and 3.8x with 4 threads. |
b88ee03 to
6095b9d
Compare
5f9204f to
70ce4ed
Compare
Contributor
Author
|
Also added a trivial one line change to fix #2167, as the code has moved and it'd just cause a conflict to make a new PR. We can delay it until later if desired though. (Plus documented a strange quirk of |
f65e3de to
0b0e2f2
Compare
It's a major refactoring, but paves the way for similar work in mpileup. When an index is present, this breaks the queries down into continguous regions and performs consensus fasta/fastq/pileup generation in parallel. Each thread independently operates on its own file descriptor and iterator. This gives considerably higher multi-threading capabilities than the old system which simply performed parallel file decompression. 16 threads gives 12-13x speed increase. More work needs to be done on reference sequence handling, but this is probably a task for htslib. Ideally we need a dedicated thread-safe reference pool that permits ref-incr and ref-decr calls to share references between multiple threads and tracks those in active usage. (This exists in CRAM, but needs generalising.) A few subtle changes occurred during the refactoring too. - Mpileup and fasta/fastq consensus are now consistent in for heterozygous gaps that fail to meet the consensus cutoff. We prefer to call the most likely base now, but will keep quality 0. Previously this differed between pileup (which used N) and fasta/fastq (which used the most likely non-* base). - The combination of region queries and whether or not to pad out with preceeding Ns for zero coverage is now consistent. In some cases it used to always count from 1 (or region start) irrespective of where data coverage was, but did stop early if coverage didn't extend to the end. This sort of half-way -a (all) mode no longer exists. Sequence reported is always first observed base to last observed, unless -a is used in which case it is 1 to chr length.
02c69e8 to
ba88d45
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It's a major refactoring, but paves the way for similar work in mpileup. When an index is present, this breaks the queries down into continguous regions and performs consensus fasta/fastq/pileup generation in parallel. Each thread independently operates on its own file descriptor and iterator.
This gives considerably higher multi-threading capabilities than the old system which simply performed parallel file decompression. 16 threads gives 12-13x speed increase.
More work needs to be done on reference sequence handling, but this is probably a task for htslib. Ideally we need a dedicated thread-safe reference pool that permits ref-incr and ref-decr calls to share references between multiple threads and tracks those in active usage. (This exists in CRAM, but needs generalising.)
A few subtle changes occurred during the refactoring too.
Mpileup and fasta/fastq consensus are now consistent in for heterozygous gaps that fail to meet the consensus cutoff. We prefer to call the most likely base now, but will keep quality 0.
The combination of region queries and whether or not to pad out with preceeding Ns for zero coverage is now consistent. In some cases it used to always count from 1 (or region start) irrespective of where data coverage was, but did stop early if coverage didn't extend to the end. This sort of half-way -a (all) mode no longer exists.
Sequence reported is always first observed base to last observed, unless -a is used in which case it is 1 to chr length.