Optimize hts_Stats hot path - #273
Conversation
2cd209a to
44d4bc0
Compare
427cd26 to
c27814e
Compare
joe-angell
left a comment
joe-angell
left a comment
There was a problem hiding this comment.
I like the improvements to counting stats but I see an issue with the do_read1_read2_files function. It seems to be redundant as it loads the input and does all the counting, then do_app does the same thing over again afaik. Maybe I'm missing something, it's been a while since I looked at this code.
| const bool no_output = vm["no-output"].as<bool>(); | ||
| WriterHelper writer(pe, se, false); | ||
| if (vm["number-of-threads"].as<size_t>() > 1) { | ||
| do_parallel_stats(reader, no_output ? nullptr : &writer, counters, vm); |
There was a problem hiding this comment.
number-of-threads defaults to 1, do we have a test for this code path?
| } | ||
|
|
||
| static bool load_fastq_record(std::istream &input, std::string &seq, std::string &qual) { | ||
| std::string id; |
There was a problem hiding this comment.
I'm a bit concerned about duplicating the code to read records
| bool do_read1_read2_files(const std::vector<std::string> &read1_files, const std::vector<std::string> &read2_files, | ||
| std::shared_ptr<OutputWriter>, std::shared_ptr<OutputWriter>, | ||
| StatsCounters& counters, const po::variables_map &vm) { | ||
| if (!vm["no-output"].as<bool>()) { |
There was a problem hiding this comment.
I think this function should be removed. When multithreaded this will read the input files and construct the counters, then when do_app is called the files will be opened and read again, not sure how this could even work when piped...
2 participants
Summary
qual-offsetonce perStatsCountersinstance instead of reading fromvariables_mapfor every basestd::vectorrows with fixed-widthstd::arrayrows for base and quality matricesBenchmarks
Sample:
/Users/biomsettles/projects/Archive/Alliance_MiSeq_Bacterial_Data/00-Original/sample1with 805,327 paired fragments.Same command, stdout redirected to
/dev/null:hts_Stats: 15.16s real, 15.54s user, 1.68s syshts_Stats: 3.70s real, 3.69s user, 1.64s sysThat is roughly a 4.1x speedup for the standalone
hts_Statsrun on this sample.Stacked pipeline timing:
--compression-level 1: 16.89s realhts_Stats | hts_SuperDeduper | hts_LengthFilter --compression-level 1: 5.32s realThat is roughly a 3.2x speedup for this streamed pipeline after the stats bottleneck is removed.
Validation
cmake --build build -j8ctest --test-dir build --output-on-failure(13/13 passed)cd regression && ./regression.sh ../build testgzip -tpassed for the benchmark pipeline output