perf: speed up per-read validation hot loops, with profiling report and benchmark rig - #3
Draft
corneliusroemer-agent wants to merge 2 commits into
Conversation
Two hot-loop changes in the FASTQ validation path, no behaviour change. InsdcReadsValidator: the per-base IUPAC check ran effectiveBases.toUpperCase().toCharArray() and then looked each base up in a HashSet<Character>. That is two allocations per read plus a boxed HashMap lookup per base. Replaced with two case-folded boolean[128] tables indexed by the raw char. Bases containing non-ASCII fall back to the original String.toUpperCase() path, so exotic case folding (U+017F uppercases to 'S', a valid IUPAC code) still behaves exactly as before. PairedFastqReadsValidator: the read name and the pair index were derived from the same regex match but computed by two separate methods, so every read name was matched twice by both patterns. Match once and read both groups. Measured ~25% less CPU in steady state (~21% on a cold JVM) on 100k-read paired FASTQ. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records how the hot-loop changes were found and verified, so the numbers are reproducible rather than asserted. benchmarks/README.md setup, how to reproduce, and the methodology traps benchmarks/RESULTS.md full profiling report and negative results benchmarks/harness/ four harnesses calling ValidatorWrapper directly benchmarks/scripts/ test-data generation, edge cases, equivalence, A/B benchmarks/ is outside the Gradle source sets, so it is neither compiled by ./gradlew build nor scanned by spotless; the harnesses are compiled on demand by the scripts. The headline finding is not the patch: JVM boot is only 0.036 s, but a cold validation costs ~0.7-0.85 s more than a warm one, so most of what a fresh-process-per-entry model wastes is JIT warmup, not startup. 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
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.
Not for merging. This is a record of an experiment, kept as a PR so the patch, the measurements and the rig that produced them stay together and reviewable. It shows that a small, behaviour-preserving change makes readtools' FASTQ validation meaningfully faster — roughly a quarter to a third less CPU — in case that's useful later here or upstream in
enasequence/readtools.This supersedes #2, which contains the same two-file code change but none of the documentation or tooling. Close whichever is less useful.
Why
Loculus invokes readtools once per submitted entry, as a fresh process:
That was one of the larger per-entry costs in its raw-reads pipeline, and it was assumed to be dominated by JVM startup. Profiling says otherwise, and pointed at two cheap fixes.
What's here
src/.../InsdcReadsValidator.java,PairedFastqReadsValidator.javabenchmarks/README.mdbenchmarks/RESULTS.mdbenchmarks/harness/*.javaValidatorWrapperdirectlybenchmarks/scripts/*.shbenchmarks/sits outside the Gradle source sets, so it is not compiled by./gradlew buildand not scanned by spotless. Verified:./gradlew test spotlessCheckis unaffected by its presence.The change
InsdcReadsValidator— the per-base IUPAC check was:Two allocations per read (an uppercased
Stringand achar[]), then a boxedCharacterlookup per base. At 64bp × 100k reads × 2 mates that's ~12.8M boxedHashMaplookups —HashMap.getNodewas the single hottest leaf frame in the profile. Replaced with two case-foldedboolean[128]tables indexed by the raw char.PairedFastqReadsValidator— the read name and the pair index were derived from the same regex match but computed by two separate methods, each running both patterns, so every read name was matched twice over. Now matched once, with both capture groups read from the one match. The four helpers this made dead are removed.One subtlety worth reviewing
String.toUpperCase()folds U+017F (ſ, long s) toS, which is a valid IUPAC code — so the original accepts it. A plain lookup table rejects it, which would fail submissions ENA's own webin-cli accepts, since it drives this same class. Reads containing any non-ASCII character therefore fall back to the originaltoUpperCase()path; pure-ASCII reads take the fast path.This is why
scripts/gen-edgecases.shexists — the divergence was caught by that test, not by reasoning.Results
Profile of the steady state (JFR, 540 samples, 99.8% attributed):
InsdcReadsValidator.validate— per-base IUPAC checkFastqReaderline readingFastqReadsValidator.validateQualityScoresPairedFastqReadsValidatorread-name regexesEffect, as steady-state CPU time (not wall clock — see below), over two runs on different JDKs:
scripts/run-benchmark.sh: 27-40%, median 31%Call it ~25-31% in steady state, ~21-28% on a cold JVM. The two runs differ in both JDK and machine load, so they're not a controlled comparison; the range is the honest answer.
Correctness: byte-identical stdout+stderr and exit status against the released
v1.0.0jar across all 20 comparisonscheck-equivalence.shemits — sizes from 1 read to 500MB, gzipped and plain, four distinct pairs, single-end, and seven edge cases including two invalid-base files that must be rejected. The repository's 227 tests pass.Findings not acted on here
Documented in
RESULTS.mdbecause they're more valuable than this patch:mainis 0.036s. What a fresh process throws away is JIT warmup: a cold validation costs ~0.7-0.85s more than a warm one, 38-51% of the call. A reusable warm JVM is worth ~50% per validation — far more than this patch — and one warm JVM serving 4 concurrent validations reached ~0.31s each.UseCompactObjectHeaders, and recompiling to Java 25 bytecode all measured as nothing.-XX:TieredStopAtLevel=1is actively harmful here — best flag for an empty input, +51% worse on a real one.ThreadMXBeanCPU time for this reason.Base branch
Based on
validate-cli(where tagv1.0.0lives) rather thanmaster, because the benchmark rig drives the CLI andmasterhas noValidateCli, so amasterbuild isn't runnable viajava -jar. The two validator files are byte-identical on both branches, so the code change is unaffected by that choice.Note for anyone building this locally:
webin-cli-validator:2.+currently fails to resolve, because the floating range is looked up againstmaven.imagej.net, which returns 503. Pin it to2.15.1. That pin is deliberately not committed here — it's a local build workaround, not part of the change.One CI note:
./gradlew spotlessCheckfails on this branch, but for a pre-existing violation inValidateCli.javathat is already present onvalidate-cli— verified untouched by this PR (git diff origin/validate-cli -- .../ValidateCli.javais empty). The two files this PR does change are spotless-clean. Not fixed here, to keep the diff to the point../gradlew testpasses: 227 tests, 0 failures.