remove caching of alleles in AbstractVCFCodec #1282

lbergelson · 2019-02-13T21:55:15Z

This replaces #1264 and responds to my own comments.

codecov-io · 2019-02-13T23:02:09Z

Codecov Report

Merging #1282 into master will increase coverage by 0.439%.
The diff coverage is 90%.

@@              Coverage Diff               @@
##             master     #1282       +/-   ##
==============================================
+ Coverage     67.49%   67.929%   +0.439%     
- Complexity     8150      8483      +333     
==============================================
  Files           558       561        +3     
  Lines         33365     34324      +959     
  Branches       5608      5848      +240     
==============================================
+ Hits          22518     23316      +798     
- Misses         8658      8796      +138     
- Partials       2189      2212       +23

Impacted Files	Coverage Δ	Complexity Δ
...main/java/htsjdk/variant/vcf/AbstractVCFCodec.java	`73.765% <100%> (+0.759%)`	`86 <0> (-4)`	⬇️
...ain/java/htsjdk/variant/variantcontext/Allele.java	`77.703% <86.667%> (+0.497%)`	`95 <9> (+7)`	⬆️
...c/main/java/htsjdk/samtools/util/SnappyLoader.java	`78.571% <0%> (-2.198%)`	`9% <0%> (ø)`
src/main/java/htsjdk/samtools/SamReader.java	`81.818% <0%> (-0.94%)`	`0% <0%> (ø)`
...va/htsjdk/samtools/SAMSequenceDictionaryCodec.java	`86.957% <0%> (-0.543%)`	`8% <0%> (+2%)`
src/main/java/htsjdk/samtools/util/CigarUtil.java	`30.303% <0%> (-0.466%)`	`14% <0%> (ø)`
...java/htsjdk/samtools/cram/ref/ReferenceSource.java	`46.364% <0%> (-0.425%)`	`19% <0%> (ø)`
...htsjdk/tribble/readers/LongLineBufferedReader.java	`29.944% <0%> (-0.342%)`	`15% <0%> (ø)`
...ain/java/htsjdk/samtools/util/MergingIterator.java	`85.417% <0%> (-0.298%)`	`18% <0%> (+7%)`
...ava/htsjdk/samtools/CRAMContainerStreamWriter.java	`70.612% <0%> (-0.289%)`	`54% <0%> (ø)`
... and 35 more

lbergelson · 2019-02-15T19:20:38Z

@pshapiro4broad Could you review this branch?

pshapiro4broad

After looking at the code, and seeing what it's trying to do, I wonder if there's a better way that can avoid this problem in the first place. The built in method String.intern() creates a canonical copy (aka an interned copy) of a String. This has no memory leak downsides as the String intern table has an upper limit and will GC strings as necessary. If we have a test that can exercise this code (which I assume we had at one point, when this string caching code was written) it would be interesting to run it using String.intern() and see if that's sufficient here.

src/main/java/htsjdk/variant/variantcontext/Allele.java

pshapiro4broad · 2019-02-20T20:08:57Z

src/main/java/htsjdk/variant/variantcontext/Allele.java

+    public static boolean wouldBeBreakpoint(final byte[] bases) {
+        if ( bases.length <= 1 )
+            return false;
+        else {


unnecessary else after return

pshapiro4broad · 2019-02-20T20:11:05Z

src/main/java/htsjdk/variant/variantcontext/Allele.java

+        if ( bases.length <= 1 )
+            return false;
+        else {
+            for (int i = 0; i < bases.length; i++) {


can use java foreach here

Suggested change

for (int i = 0; i < bases.length; i++) {

for (byte base : bases) {

We've been finding noticeable runtime costs for using foreach in hotspots (seemingly because it instantiates a new iterator each time which is much more costly than a fast for loop). Since Alleles are created in a lot of places I figured I'd stick with the for loop even though it's gross... That said, I haven't profiled this particular instance in any sensible way so I can't prove that this is any faster.

pshapiro4broad · 2019-02-20T20:12:02Z

src/main/java/htsjdk/variant/variantcontext/Allele.java

+        if ( bases.length <= 1 )
+            return false;
+        else {
+            return bases[0] == '.' || bases[bases.length - 1] == '.';


It would be nice to use constants instead of the magic characters > < [ ] .

I've extracted some... I can't decide if it's an improvement or not... You can take a look when I push the changes.

pshapiro4broad · 2019-02-20T20:17:19Z

src/main/java/htsjdk/variant/vcf/AbstractVCFCodec.java

        builder.log10PError(parseQual(parts[5]));

        final List<String> filters = parseFilters(getCachedString(parts[6]));
-        if ( filters != null ) builder.filters(new HashSet<String>(filters));
+        if ( filters != null ) builder.filters(new HashSet<>(filters));


may as well format the if across lines and use { } if you're changing this line

pshapiro4broad · 2019-02-20T20:26:34Z

src/main/java/htsjdk/variant/vcf/AbstractVCFCodec.java

@@ -577,7 +577,7 @@ private static void checkAllele(String allele, boolean isRef, int lineNo) {
            System.err.println(String.format("Allele detected with length %d exceeding max size %d at approximately line %d, likely resulting in degraded VCF processing performance", allele.length(), MAX_ALLELE_SIZE_BEFORE_WARNING, lineNo));
        }

-        if ( isSymbolicAllele(allele) ) {
+        if (Allele.wouldBeSymbolicAllele(allele.getBytes())) {


This check is done before calling the Allele constructor, but it appears to be ensuring that we create a valid Allele. Is there a reason why this code isn't part of the Allele constructor? We would have to wrap the constructor call in a try/catch to get a message printed out with the VCF line number, though.

Yeah, there's a big redundant section of allele checking code here that I believe is redundant with the constructor. I wanted to make this as small a change set as possible though. I started a follow up branch that cleans up some of this stuff but I figured I'd get this one in before that.

pshapiro4broad · 2019-02-20T20:27:58Z

src/main/java/htsjdk/variant/variantcontext/Allele.java

+     * @param bases  bases representing an allele
+     * @return true if the bases represent a symbolic allele in breakpoint notation, (ex: G]17:198982] or ]13:123456]T )
+     */
+    public static boolean wouldBeBreakpoint(final byte[] bases) {


The use of would here confuses me. If this is a breakpoint allele string, then why not is?

Yeah, it's weird, but it matches the existing methods. I think changing 1 of them is worse than having this weird convention. We could change all of them though...

It's ok, I didn't see the rest of them.

pshapiro4broad · 2019-02-20T20:31:08Z

src/test/java/htsjdk/variant/variantcontext/AlleleUnitTest.java

@@ -277,4 +277,38 @@ public void testExtend() {
        Assert.assertEquals("ATCGA", Allele.extend(Allele.create("AT"), "CGA".getBytes()).toString());
        Assert.assertEquals("ATCGA", Allele.extend(Allele.create("ATC"), "GA".getBytes()).toString());
    }
+    @Test
+    public void testWouldBeSymbolic() {


Could rewrite these tests using a testng data provider

Suggested change

public void testWouldBeSymbolic() {

public void testWouldBeSymbolic(String allele, boolean expectedSymbolic) {

Done, I didn't do all the tests in this file though, just the bottom ones..

spacing fix Co-Authored-By: lbergelson <[email protected]>

lbergelson requested a review from yfarjoun February 13, 2019 21:55

lbergelson mentioned this pull request Feb 13, 2019

Fixed memory leak in VCFCodec #1264

Closed

5 tasks

Fixed memory leak in VCFCodec

50b98a9

lbergelson force-pushed the lb_vcfcodec_memory_leak branch from ba32f98 to a9065a3 Compare February 14, 2019 16:03

lbergelson removed the request for review from yfarjoun February 14, 2019 18:27

resolving my own comments

5d993aa

lbergelson force-pushed the lb_vcfcodec_memory_leak branch from a9065a3 to 5d993aa Compare February 15, 2019 19:16

pshapiro4broad reviewed Feb 20, 2019

View reviewed changes

pshapiro4broad and others added 3 commits February 21, 2019 15:00

Apply suggestions from code review

77466a1

spacing fix Co-Authored-By: lbergelson <[email protected]>

responding to comments

ed5991e

fix stupid thing

2862c14

pshapiro4broad approved these changes Feb 25, 2019

View reviewed changes

lbergelson merged commit a6c5837 into master Feb 26, 2019

lbergelson deleted the lb_vcfcodec_memory_leak branch February 26, 2019 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove caching of alleles in AbstractVCFCodec #1282

remove caching of alleles in AbstractVCFCodec #1282

lbergelson commented Feb 13, 2019

codecov-io commented Feb 13, 2019 •

edited

Loading

lbergelson commented Feb 15, 2019

pshapiro4broad left a comment •

edited

Loading

pshapiro4broad Feb 20, 2019

lbergelson Feb 21, 2019

pshapiro4broad Feb 20, 2019

lbergelson Feb 20, 2019

pshapiro4broad Feb 20, 2019

lbergelson Feb 21, 2019

pshapiro4broad Feb 20, 2019

lbergelson Feb 21, 2019

pshapiro4broad Feb 20, 2019

lbergelson Feb 20, 2019

pshapiro4broad Feb 20, 2019

lbergelson Feb 20, 2019

pshapiro4broad Feb 25, 2019 •

edited

Loading

pshapiro4broad Feb 20, 2019

lbergelson Feb 21, 2019

	for (int i = 0; i < bases.length; i++) {
	for (byte base : bases) {

	public void testWouldBeSymbolic() {
	public void testWouldBeSymbolic(String allele, boolean expectedSymbolic) {

remove caching of alleles in AbstractVCFCodec #1282

remove caching of alleles in AbstractVCFCodec #1282

Conversation

lbergelson commented Feb 13, 2019

codecov-io commented Feb 13, 2019 • edited Loading

Codecov Report

lbergelson commented Feb 15, 2019

pshapiro4broad left a comment • edited Loading

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

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

pshapiro4broad Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 13, 2019 •

edited

Loading

pshapiro4broad left a comment •

edited

Loading

pshapiro4broad Feb 25, 2019 •

edited

Loading