More excessively micro micro-optimizations #5616

jamesemery · 2019-01-28T21:48:54Z

These methods were showing up as time syncs on the profile for #5607

I ran these a few times vs. its root branch, its probably a small improvement in runtime over this chr15 snippet of an exome on my local machine:
#5607:

2m57.516s, 2m56.677s, 3m2.531s, 3m4.116s

This branch:

2m50.112s, 2m46.981s, 2m43.764s, 2m40.806s

Depends On #5607

codecov-io · 2019-01-28T22:34:47Z

Codecov Report

Merging #5616 into master will increase coverage by 0.003%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #5616       +/-   ##
===============================================
+ Coverage     87.035%   87.038%   +0.003%     
- Complexity     31726     31730        +4     
===============================================
  Files           1943      1943               
  Lines         146193    146199        +6     
  Branches       16141     16144        +3     
===============================================
+ Hits          127239    127249       +10     
+ Misses         13067     13064        -3     
+ Partials        5887      5886        -1

Impacted Files	Coverage Δ	Complexity Δ
...kers/haplotypecaller/ReferenceConfidenceModel.java	`92.632% <100%> (+0.24%)`	`70 <0> (+1)`	⬆️
...nder/tools/spark/pipelines/ReadsPipelineSpark.java	`90.741% <0%> (ø)`	`13% <0%> (ø)`	⬇️
...ithwaterman/SmithWatermanIntelAlignerUnitTest.java	`60% <0%> (ø)`	`2% <0%> (ø)`	⬇️
...lotypecaller/readthreading/ReadThreadingGraph.java	`88.861% <0%> (+0.253%)`	`145% <0%> (+1%)`	⬆️
...utils/smithwaterman/SmithWatermanIntelAligner.java	`80% <0%> (+30%)`	`3% <0%> (+2%)`	⬆️

…impact on the world

jamesemery · 2019-02-04T21:40:05Z

@droazen this branch wasn't STRICTLY dependent on #5607, so I removed it from this branch to make reviewing easier. Its worth noting that the performance numbers and observed speedup were seen when this branch did hang off of #5607.

droazen

@jamesemery back to you with my comments

droazen · 2019-02-07T20:36:18Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

@@ -193,18 +193,18 @@ public ReferenceConfidenceModel(final SampleList samples,
        final String sampleName = readLikelihoods.getSample(0);

        final int globalRefOffset = refSpan.getStart() - activeRegion.getExtendedSpan().getStart();
-        for ( final ReadPileup pileup : refPileups ) {
+        for (int i = 0, size = refPileups.size(); i < size; i++) {


Declare refPileupsSize as a final var before the loop instead of using multiple initialization within the loop.

droazen · 2019-02-07T20:36:57Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

@@ -193,18 +193,18 @@ public ReferenceConfidenceModel(final SampleList samples,
        final String sampleName = readLikelihoods.getSample(0);

        final int globalRefOffset = refSpan.getStart() - activeRegion.getExtendedSpan().getStart();
-        for ( final ReadPileup pileup : refPileups ) {
+        for (int i = 0, size = refPileups.size(); i < size; i++) {
+            ReadPileup pileup = refPileups.get(i);


droazen · 2019-02-07T20:39:04Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

        final int position = call != null ? call.getStart() : curPos.getStart();
-        return priorList.stream().filter(vc -> position == vc.getStart()).collect(Collectors.toList());
+        List<VariantContext> list = new ArrayList<>(priorList.size());


And choose a more descriptive name than list. How about matchingPriors?

droazen · 2019-02-07T20:40:20Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

        final int position = call != null ? call.getStart() : curPos.getStart();
-        return priorList.stream().filter(vc -> position == vc.getStart()).collect(Collectors.toList());
+        List<VariantContext> list = new ArrayList<>(priorList.size());
+        for (int i = 0, size = priorList.size(); i < size; i++) {


Declare priorListSize as a final var before the loop instead of using multiple initialization within the loop.

droazen · 2019-02-07T20:42:23Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

        final int position = call != null ? call.getStart() : curPos.getStart();
-        return priorList.stream().filter(vc -> position == vc.getStart()).collect(Collectors.toList());


Add a comment that this method is implemented with an old-school for loop instead of streaming for performance reasons (otherwise, someone will probably come along in 2 years and undo this).

jamesemery · 2019-02-07T21:11:58Z

@droazen Responded to comments, note that i added a further escape condition where getMatchingPriors is avoided altogether if the VCpriors list is empty. Remember that this method is in a performance sensitive part of the code so every little bit of speed counts.

droazen · 2019-02-08T20:00:26Z

@jamesemery You have a compiler error on the latest version of this branch:

:compileJava/home/travis/build/broadinstitute/gatk/src/main/java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java:428: error: ';' expected
        final int priorsListSize = priorList.size()
                                                   ^
1 error

jamesemery · 2019-02-08T20:04:26Z

@droazen fixed, sorry about that

droazen

Two minor additional comments -- back to @jamesemery. Merge after addressing them and tests pass.

droazen · 2019-02-08T20:02:29Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

@@ -193,18 +193,19 @@ public ReferenceConfidenceModel(final SampleList samples,
        final String sampleName = readLikelihoods.getSample(0);

        final int globalRefOffset = refSpan.getStart() - activeRegion.getExtendedSpan().getStart();
-        for ( final ReadPileup pileup : refPileups ) {
+        final int refPileupsSize = refPileups.size();
+        for (int i = 0; i < refPileupsSize; i++) {


Add comment here that the old-school for loop is intentional (for-each was found to show up on the profiler).

droazen · 2019-02-08T20:05:55Z

...va/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/ReferenceConfidenceModel.java

        final int position = call != null ? call.getStart() : curPos.getStart();
-        return priorList.stream().filter(vc -> position == vc.getStart()).collect(Collectors.toList());
+        final List<VariantContext> matchedPriors = new ArrayList<>(priorList.size());
+        // NOTE: a for loop is used here because this method ends up being called per-pileup, per-read and thus using faster alternates saves runtime


" thus using faster alternates saves runtime" -> "using a loop instead of streaming saves runtime"

jamesemery requested a review from droazen January 28, 2019 21:48

droazen self-assigned this Feb 4, 2019

replaced some iterators with indexed-for-lists in hopes of making an …

b194fab

…impact on the world

jamesemery force-pushed the je_je_HaplotypeCallerMicroOptimization4 branch from b3f376f to b194fab Compare February 4, 2019 21:38

droazen suggested changes Feb 7, 2019

View reviewed changes

droazen assigned jamesemery and unassigned droazen Feb 7, 2019

responded to comments

4b8941b

fixing the error

7881211

droazen approved these changes Feb 8, 2019

View reviewed changes

responding to final comments

73d6f78

jamesemery merged commit 5562b25 into master Feb 8, 2019

jamesemery deleted the je_je_HaplotypeCallerMicroOptimization4 branch February 8, 2019 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More excessively micro micro-optimizations #5616

More excessively micro micro-optimizations #5616

jamesemery commented Jan 28, 2019

codecov-io commented Jan 28, 2019 •

edited

Loading

jamesemery commented Feb 4, 2019

droazen left a comment

droazen Feb 7, 2019

droazen Feb 7, 2019

droazen Feb 7, 2019

droazen Feb 7, 2019

droazen Feb 7, 2019

droazen Feb 7, 2019

jamesemery commented Feb 7, 2019

droazen commented Feb 8, 2019

jamesemery commented Feb 8, 2019

droazen left a comment

droazen Feb 8, 2019

droazen Feb 8, 2019

		final int position = call != null ? call.getStart() : curPos.getStart();
		return priorList.stream().filter(vc -> position == vc.getStart()).collect(Collectors.toList());

More excessively micro micro-optimizations #5616

More excessively micro micro-optimizations #5616

Conversation

jamesemery commented Jan 28, 2019

codecov-io commented Jan 28, 2019 • edited Loading

Codecov Report

jamesemery commented Feb 4, 2019

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesemery commented Feb 7, 2019

droazen commented Feb 8, 2019

jamesemery commented Feb 8, 2019

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 28, 2019 •

edited

Loading