From 276ac36ba34cd764105e72bfc8e334cfeb500f2a Mon Sep 17 00:00:00 2001 From: Tom White Date: Mon, 30 Jul 2018 15:57:29 +0100 Subject: [PATCH 1/3] Synchronize update of shared genotype likelihood tables. --- .../walkers/genotyper/GenotypeLikelihoodCalculators.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java b/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java index f9076f5b7dd..ccdb0e52184 100644 --- a/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java +++ b/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java @@ -275,12 +275,11 @@ public GenotypeLikelihoodCalculator getInstance(final int ploidy, final int alle * @param requestedMaximumAllele the new requested maximum allele maximum. * @param requestedMaximumPloidy the new requested ploidy maximum. */ - private void ensureCapacity(final int requestedMaximumAllele, final int requestedMaximumPloidy) { + private synchronized void ensureCapacity(final int requestedMaximumAllele, final int requestedMaximumPloidy) { final boolean needsToExpandAlleleCapacity = requestedMaximumAllele > maximumAllele; final boolean needsToExpandPloidyCapacity = requestedMaximumPloidy > maximumPloidy; - // Double check with the lock on to avoid double work. if (!needsToExpandAlleleCapacity && !needsToExpandPloidyCapacity) { return; } @@ -383,9 +382,7 @@ public static int computeMaxAcceptableAlleleCount(final int ploidy, final int ma private int calculateGenotypeCountUsingTables(int ploidy, int alleleCount) { checkPloidyAndMaximumAllele(ploidy, alleleCount); - if (ploidy > maximumPloidy || alleleCount > maximumAllele) { - ensureCapacity(alleleCount, ploidy); - } + ensureCapacity(alleleCount, ploidy); return alleleFirstGenotypeOffsetByPloidy[ploidy][alleleCount]; } } \ No newline at end of file From 28d3a73e35047daa8aeae0770b852a4b20db7d0c Mon Sep 17 00:00:00 2001 From: Tom White Date: Tue, 4 Sep 2018 10:17:32 +0100 Subject: [PATCH 2/3] Add comment explaning synchronization. --- .../walkers/genotyper/GenotypeLikelihoodCalculators.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java b/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java index ccdb0e52184..97814a78332 100644 --- a/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java +++ b/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java @@ -270,7 +270,9 @@ public GenotypeLikelihoodCalculator getInstance(final int ploidy, final int alle } /** - * Update of shared tables + * Update of shared tables. Note that this method is synchronized to avoid race conditions in Spark. This is because + * in Spark each region has its own HaplotypeCallerEngine instance (so there can be multiple instances in a single + * JVM), but these share a single (static) instance of this class (via AlleleSubsettingUtils). * * @param requestedMaximumAllele the new requested maximum allele maximum. * @param requestedMaximumPloidy the new requested ploidy maximum. From 892ff975925f1d7229ee8f22493b18d0c53c203a Mon Sep 17 00:00:00 2001 From: Tom White Date: Tue, 2 Oct 2018 16:50:19 +0100 Subject: [PATCH 3/3] Synchronize for reads as well as writes to shared state. --- .../genotyper/GenotypeLikelihoodCalculators.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java b/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java index 97814a78332..8b2008c1b63 100644 --- a/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java +++ b/src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypeLikelihoodCalculators.java @@ -9,7 +9,8 @@ import java.util.Arrays; /** - * Genotype likelihood calculator utility. + * Genotype likelihood calculator utility. This class is thread-safe since access to shared mutable state is + * synchronized. * *

* This class provide genotype likelihood calculators with any number of alleles able given an arbitrary ploidy and allele @@ -257,7 +258,7 @@ private static GenotypeAlleleCounts[] buildGenotypeAlleleCountsArray(final int p * * @return never {@code null}. */ - public GenotypeLikelihoodCalculator getInstance(final int ploidy, final int alleleCount) { + public synchronized GenotypeLikelihoodCalculator getInstance(final int ploidy, final int alleleCount) { checkPloidyAndMaximumAllele(ploidy, alleleCount); if (calculateGenotypeCountUsingTables(ploidy, alleleCount) == GENOTYPE_COUNT_OVERFLOW) { @@ -270,9 +271,7 @@ public GenotypeLikelihoodCalculator getInstance(final int ploidy, final int alle } /** - * Update of shared tables. Note that this method is synchronized to avoid race conditions in Spark. This is because - * in Spark each region has its own HaplotypeCallerEngine instance (so there can be multiple instances in a single - * JVM), but these share a single (static) instance of this class (via AlleleSubsettingUtils). + * Update of shared tables. * * @param requestedMaximumAllele the new requested maximum allele maximum. * @param requestedMaximumPloidy the new requested ploidy maximum. @@ -381,8 +380,7 @@ public static int computeMaxAcceptableAlleleCount(final int ploidy, final int ma throw new GATKException("Code should never reach here."); } - - private int calculateGenotypeCountUsingTables(int ploidy, int alleleCount) { + private synchronized int calculateGenotypeCountUsingTables(int ploidy, int alleleCount) { checkPloidyAndMaximumAllele(ploidy, alleleCount); ensureCapacity(alleleCount, ploidy); return alleleFirstGenotypeOffsetByPloidy[ploidy][alleleCount];