Skip to content

perf: speed up per-read validation hot loops, with profiling report and benchmark rig - #3

Draft
corneliusroemer-agent wants to merge 2 commits into
loculus-project:validate-clifrom
corneliusroemer-agent:perf/validation-speedup-with-benchmarks
Draft

perf: speed up per-read validation hot loops, with profiling report and benchmark rig#3
corneliusroemer-agent wants to merge 2 commits into
loculus-project:validate-clifrom
corneliusroemer-agent:perf/validation-speedup-with-benchmarks

Conversation

@corneliusroemer-agent

Copy link
Copy Markdown

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:

java -jar readtools.jar <mate1> <mate2> --format FASTQ

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.java the change itself, +66/−60
benchmarks/README.md how to build, generate inputs, and reproduce everything — including the setup traps
benchmarks/RESULTS.md the full profiling report: where the time goes, what was tried, what didn't work
benchmarks/harness/*.java four harnesses that call ValidatorWrapper directly
benchmarks/scripts/*.sh test-data generation, edge cases, equivalence checking, A/B benchmarking

benchmarks/ sits outside the Gradle source sets, so it is not compiled by ./gradlew build and not scanned by spotless. Verified: ./gradlew test spotlessCheck is unaffected by its presence.

The change

InsdcReadsValidator — the per-base IUPAC check was:

for (char base : effectiveBases.toUpperCase().toCharArray()) {
  if (iupacSet.contains(base)) { ... }   // iupacSet is a HashSet<Character>

Two allocations per read (an uppercased String and a char[]), then a boxed Character lookup per base. At 64bp × 100k reads × 2 mates that's ~12.8M boxed HashMap lookups — HashMap.getNode was the single hottest leaf frame in the profile. Replaced with two case-folded boolean[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) to S, 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 original toUpperCase() path; pure-ASCII reads take the fast path.

This is why scripts/gen-edgecases.sh exists — the divergence was caught by that test, not by reasoning.

Results

Profile of the steady state (JFR, 540 samples, 99.8% attributed):

share where
28.0% InsdcReadsValidator.validate — per-base IUPAC check
16.5% htsjdk FastqReader line reading
15.9% FastqReadsValidator.validateQualityScores
~22.7% Guava hashing for the pairing Bloom filter
11.9% PairedFastqReadsValidator read-name regexes

Effect, as steady-state CPU time (not wall clock — see below), over two runs on different JDKs:

  • JDK 21, loaded machine: 24-26% less CPU
  • JDK 17, idle machine, via 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.0 jar across all 20 comparisons check-equivalence.sh emits — 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.md because they're more valuable than this patch:

  • JVM startup is a red herring. Boot to main is 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.
  • Cost is flat above 100k reads, since quick mode stops there: a 500MB file costs the same as a 50MB one.
  • Things that didn't help: JDK 25 over 21, UseCompactObjectHeaders, and recompiling to Java 25 bytecode all measured as nothing. -XX:TieredStopAtLevel=1 is actively harmful here — best flag for an empty input, +51% worse on a real one.
  • Don't benchmark this with wall clock on a shared machine. Two runs of the same jar differed by 2.3x under load. The harnesses report ThreadMXBean CPU time for this reason.

Base branch

Based on validate-cli (where tag v1.0.0 lives) rather than master, because the benchmark rig drives the CLI and master has no ValidateCli, so a master build isn't runnable via java -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 against maven.imagej.net, which returns 503. Pin it to 2.15.1. That pin is deliberately not committed here — it's a local build workaround, not part of the change.

One CI note: ./gradlew spotlessCheck fails on this branch, but for a pre-existing violation in ValidateCli.java that is already present on validate-cli — verified untouched by this PR (git diff origin/validate-cli -- .../ValidateCli.java is empty). The two files this PR does change are spotless-clean. Not fixed here, to keep the diff to the point. ./gradlew test passes: 227 tests, 0 failures.

corneliusroemer-agent and others added 2 commits August 27, 2026 14:38
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant