WIP: api tuning - #1961
Open
glennhickey wants to merge 2 commits into
Open
Conversation
Picks up two changes to the pinch graph, plus a small API addition:
- the per-thread AVL index over segments is replaced by a coarse coordinate
index sized by segment density. This drops ~48 bytes of heap per segment
(an avl_node plus its malloc overhead) and makes lookups cheaper, and the
density-based sizing is what keeps it cheap on the shattered graphs high
divergence alignments produce rather than only on pangenomes
- the block orientation is packed into the low bit of the block pointer,
taking stPinchSegment from 56 bytes to 48
- stPinchBlock_setNumSupportingHomologies, so a client can hang one value off
each block without paying for a hash table (hal2vg uses it for node ids)
Correctness: stPinchesAndCactiTests (46, including two new ones covering the
index against a brute force walk), stCafTests (17) and cactus_barTests (17)
all pass. hal2vg produces byte identical output through all of this on yeast,
GRCh38 chrY and 16 yeast chromosomes, at 46% less peak memory.
WHAT IS NOT YET VALIDATED, and why this is a work in progress:
The speedup is measured on a synthetic workload only: repeated random pinches
followed by joinTrivialBoundaries over 20 threads, where it runs 1.5x to 2.1x
faster than the AVL across pinch lengths from 2 to 150 bases. Real caf pinches
are structured and locally correlated, and thread lengths are skewed, so this
needs confirming on real data.
evolverMammals cannot confirm it: caf accounts for 1 second of the 159 that
cactus_consolidated spends on the mr job, the rest being bar/poa. Old and new
differed by about 8% on peak RSS there, but three runs of the *same* binary
varied by 19%, so that measurement was noise. A caf dominated workload is
needed instead.
Two things noticed along the way that are worth separate attention:
- cactus_consolidated is not deterministic even at --threads 1, consistent
with sets keyed on pointer values. Note that this change moves allocation
patterns, so it will shift pointer values and hash iteration order, and
hence output, without any behavioural difference
- cactus builds with assertions enabled (include.mk adds -UNDEBUG over
sonLib's -DNDEBUG), unlike hal2vg's build of the same library. Some of the
asserts here are in hot paths and their cost has not been measured
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Trying to pull in some changes from ComparativeGenomicsToolkit/hal2vg#73 into cactus. But don't have time now to test properly, so will leave this PR open to remind me until I do. What's in there so far:
After testing this at scale, will explore similar changes within the cactus graph structures. Will also see if I can get Claude to work on adding determinism...