Improve performance of pileup iterator

Author: kojix2

lib/hts/bam/pileup.rb

@@ -129,25 +129,43 @@ def each
         pos_ptr = FFI::MemoryPointer.new(:long_long) # hts_pos_t
         n_ptr   = FFI::MemoryPointer.new(:int)
 
+        # Micro-optimizations:
+        # - Compute constant struct size once
+        # - Hoist header reference outside the loop
+        plp1_size    = HTS::LibHTS::BamPileup1.size
+        header_local = @header
+
         begin
-          while (base_ptr = HTS::LibHTS.bam_plp64_auto(@plp, tid_ptr, pos_ptr, n_ptr)) && !base_ptr.null?
+          loop do
+            base_ptr = HTS::LibHTS.bam_plp64_auto(@plp, tid_ptr, pos_ptr, n_ptr)
+
+            # When base_ptr is NULL, check n to distinguish EOF (n == 0) from error (n < 0)
+            if base_ptr.null?
+              n = n_ptr.read_int
+              raise "HTSlib pileup error (bam_plp64_auto)" if n < 0
+
+              break
+            end
+
             tid = tid_ptr.read_int
             pos = pos_ptr.read_long_long
             n   = n_ptr.read_int
 
-            # Construct alignment entries
-            alignments = if n.zero?
-                           []
-                         else
-                           size = HTS::LibHTS::BamPileup1.size
-                           n.times.map do |i|
-                             e_ptr = base_ptr + (i * size)
-                             entry = HTS::LibHTS::BamPileup1.new(e_ptr)
-                             PileupRecord.new(entry, @header)
-                           end
-                         end
-
-            yield PileupColumn.new(tid:, pos:, alignments:)
+            # Construct alignment entries with minimal allocations
+            if n.zero?
+              alignments = []
+            else
+              alignments = Array.new(n)
+              i = 0
+              while i < n
+                e_ptr = base_ptr + (i * plp1_size)
+                entry = HTS::LibHTS::BamPileup1.new(e_ptr)
+                alignments[i] = PileupRecord.new(entry, header_local)
+                i += 1
+              end
+            end
+
+            yield PileupColumn.new(tid: tid, pos: pos, alignments: alignments)
           end
         ensure
           close

test/bam/pileup_test.rb

@@ -17,6 +17,13 @@ def test_pileup_yields_columns
       assert_kind_of Array, got.alignments
       assert_kind_of Integer, got.depth
       assert_operator got.depth, :>=, 0
+
+      # depth must equal the number of alignments in the column
+      assert_equal got.alignments.length, got.depth
+
+      # tid must be a valid reference id within the header target count
+      assert_operator got.tid, :>=, 0
+      assert_operator got.tid, :<, bam.header.target_count
     end
   end
 
@@ -57,4 +64,40 @@ def test_pileup_record_persists_beyond_step
       assert_includes %w[= A C G T M R S V W Y H K D B N], kept.base(0)
     end
   end
+
+  # Calling record multiple times must return the same instance (idempotent lazy copy).
+  def test_record_idempotent
+    HTS::Bam.open(Fixtures["moo.bam"]) do |bam|
+      r1 = r2 = nil
+      bam.pileup do |col|
+        next if col.alignments.empty?
+
+        aln = col.alignments.first
+        r1 = aln.record
+        r2 = aln.record
+        break
+      end
+      refute_nil r1
+      assert_same r1, r2
+    end
+  end
+
+  # HTSlib contract: if is_refskip is set, is_del must also be set.
+  def test_refskip_implies_del
+    HTS::Bam.open(Fixtures["moo.bam"]) do |bam|
+      seen_refskip = false
+      checked = 0
+      bam.pileup do |col|
+        col.alignments.each do |aln|
+          if aln.refskip?
+            seen_refskip = true
+            assert aln.del?, "refskip implies del in bam_pileup1_t"
+          end
+        end
+        checked += 1
+        break if seen_refskip || checked >= 200
+      end
+      # If no refskip appears in the inspected window, we don't fail the test.
+    end
+  end
 end