Repository navigation
[query] Cache the last contig index for canonical loci - #15881
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Cached contig-index lookup hail/hail/src/is/hail/variant/ReferenceGenome.scala, hail/hail/src/is/hail/variant/ContigIndexCache.scala, hail/hail/src/is/hail/types/physical/stypes/concrete/SCanonicalLocusPointer.scala, hail/hail/src/is/hail/expr/ir/functions/LocusFunctions.scala |
ContigIndexCache reuses an index when the contig bytes match its cached entry. Otherwise, it decodes the bytes and calls ReferenceGenome.getContigIndex. Canonical locus lookup uses the cache, and missing contigs produce a reference-specific error. |
Contig lookup coverage hail/hail/test/src/is/hail/variant/ContigIndexCacheSuite.scala, hail/hail/test/src/is/hail/expr/ir/LocusFunctionsSuite.scala, hail/hail/test/src/is/hail/variant/ReferenceGenomeSuite.scala |
Tests cover built-in and custom reference genomes, cache behavior with reused region memory, generated-code lookups, contig-index results, and missing-contig errors. |
Stable encoding test contexts hail/hail/test/src/is/hail/TestUtils.scala, hail/hail/test/src/is/hail/expr/ir/TableIRSuite.scala, hail/hail/test/src/is/hail/io/bgen/BgenIndexSuite.scala, hail/hail/test/src/is/hail/types/encoded/ETypeSuite.scala |
withStableEncodings runs callbacks with unstable encodings disabled. Stable baseline setup in table, BGEN index, and encoded-type tests now uses explicit stable-encoding contexts. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor
Sequence Diagram(s)
sequenceDiagram
participant SLocusValue
participant ContigIndexCache
participant ReferenceGenome
SLocusValue->>ContigIndexCache: Pass contig byte address and length
ContigIndexCache->>ContigIndexCache: Compare bytes with cached contig
ContigIndexCache->>ReferenceGenome: Look up decoded contig on cache miss
ReferenceGenome-->>ContigIndexCache: Return contig index or report missing contig
ContigIndexCache-->>SLocusValue: Return contig index
Suggested reviewers: ehigham
Merge Risk: 
848bc
-
The cache preserves canonical locus contig-index results and missing-contig errors without introducing merge-blocking risk.
Security architecture risk: 96c8e
-
The inspected cache preserves reference-genome validation, owns a copy of the contig bytes, and remains local to generated execution instances. No material security risk introduced or worsened by this change was identified.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The demonstrated propagation is through Hail query contig ordinals, including contig_idx and packed-locus consumers. Cache corruption could affect query data correctness, but the inspected ownership path does not introduce shared cache state across concurrent partitions.
Trust Boundaries and Controls
- inferred — Input contig bytes cannot select a new ordinal merely by matching a prefix or length: every byte must match a previously resolved entry for the cache's reference genome. Other inputs return to the authoritative lookup, including its missing-contig rejection. No validation bypass was identified in this path.
-
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: caching the last contig index for canonical loci. Description check The description explains the change and justification, and it includes the required security assessment conclusion. The impact-rating, impact-description, and Appsec Review sections may be omitted bec… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the change and justification, and it includes the required security assessment conclusion. The impact-rating, impact-description, and Appsec Review sections may be omitted because the change is stated not to affect the Hail Batch instance.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
96c8e94 to
8b5ee71
Compare
| // Compares the contig's bytes in place against the last contig seen, so runs of loci on one | ||
| // contig load no String. | ||
| override def contigIdx(cb: EmitCodeBuilder): Value[Int] = { | ||
| val cache = cb.emb.getOrDefineLazyField[ContigIndexCache]( |
There was a problem hiding this comment.
Does this need to be defined on the module to survive method splitting?
There was a problem hiding this comment.
No. Method splitting, as a pass, fixes up references if they're moved around. Furthermore, I don't think that method splitting makes new classes. The fields stay on the same class, as to the methods. IIRC, we package locals into a field holding a container object, then when the split method is called, those locals are packed, then unpacked on each side of the caller.
| override def contigIdx(cb: EmitCodeBuilder): Value[Int] = { | ||
| val cache = cb.emb.getOrDefineLazyField[ContigIndexCache]( | ||
| Code.newInstance[ContigIndexCache, ReferenceGenome](cb.emb.getReferenceGenome(st.rg)), | ||
| ("contig_index_cache", st.rg), |
There was a problem hiding this comment.
I forget how fields are named and whether or not there's some amount of scrambling, but what is it possible a query operates on more than one reference? shouldn't the name include the rg to avoid collisions?
There was a problem hiding this comment.
I think you're right. This is easy to fix.
ehigham
left a comment
ehigham
left a comment
There was a problem hiding this comment.
see comments. Looking good otherwise
8b5ee71 to
eb68ae8
Compare
eb68ae8 to
848bc4e
Compare
3f71b69 to
3810194
Compare
b3f4f66 to
510b8fd
Compare
510b8fd to
61b0000
Compare
61b0000 to
9d93832
Compare
81af4d8 to
05624ac
Compare
fdf00c7 to
6566137
Compare
6566137 to
ad0d9d6
Compare
ad0d9d6 to
ee7f13a
Compare
ee7f13a to
7f0f929
Compare
Converting a canonical locus to its contig index loaded the contig as a String and looked it up in the reference genome's contig map every time. Loci are usually processed in key order, so consecutive lookups almost always hit the same contig. ContigIndexCache keeps a copy of the last contig's bytes and its index. A hit compares bytes in place and creates no String. A miss falls back to ReferenceGenome.getContigIndex, so results are unchanged. Generated code holds one instance per reference genome as a lazy field of the generated class, keeping the cache confined to one thread and off the shared ReferenceGenome.
getContigIndex unboxed the contig map's null for an unknown contig to 0, so an unknown contig silently became the reference genome's first contig. It now raises the same error as checkContig. Locus producers validate contigs, so well-formed data never reaches this path, but contig_idx, canonical contigIdx / packed, and unstaged packing of loci all go through it.
contig_idx loaded the contig as a String and looked it up in the reference genome, bypassing SLocusValue.contigIdx. Canonical loci now get the cached contig index lookup, and packed loci read the index directly from their integer.
Tests that check unstable encodings against stable ones took the session's flags as the stable baseline. With use_unstable_encodings set for the whole run, two of them failed, and others compared the unstable encoding with itself. Add withStableEncodings and use it for each stable half.
The cache's own suite checked the helper directly. These tests drive it through the canonical locus's generated contigIdx, where a cache could go wrong in ways a direct call does not show: - Region reuse: after the region is cleared, a different contig of the same length lands at the same address. A cache keyed on the address, or on a view of the region, returns the old index. - Unknown contigs: a failed lookup, repeated, leaves the cache giving correct indices for the previously cached contig and a new one. - Concurrency: many small partitions on the local backend, with contigs interleaved within each partition, write canonical loci under unstable encodings and read them back. Expected values come from ReferenceGenome.getContigIndex, the uncached lookup.
7f0f929 to
e1eaa2d
Compare
Converting a canonical locus to its contig index loaded the contig as a String and looked it up in the reference genome's contig map every time. Loci are usually processed in key order, so consecutive lookups almost always hit the same contig.
ContigIndexCache keeps a copy of the last contig's bytes and its index. A hit compares bytes in place and creates no String. A miss falls back to ReferenceGenome.getContigIndex, so results are unchanged. Generated code holds one instance per reference genome as a lazy field of the generated class, keeping the cache confined to one thread and off the shared ReferenceGenome.
Security Assessment
Stack created with GitHub Stacks CLI • Give Feedback 💬