perf: determinism-neutral speedups + opt-in inference pipelining (and docs/tests)#3
Merged
BenjaminDEMAILLE merged 6 commits intoJun 24, 2026
Conversation
BenjaminDEMAILLE
force-pushed
the
apple-silicon-native-v2-pr
branch
from
0cd41f8 to
2724a87
Compare
|
Heads-up @nh13: I had to squash Side effect: this rewrote the base of your 4 open PRs (#3, #4, #6, #7), so they now show an inflated diff against Sorry for the churn, and thanks again for all the fixes. Your authorship is preserved via a |
BenjaminDEMAILLE
force-pushed
the
apple-silicon-native-v2-pr
branch
from
2724a87 to
a21e4d4
Compare
- parity_check.autoload silently swallowed any decode error and switched formats via a bare except; fall back to the minimal decoder only on a raised error or zero rows, log which decoder is used, and print a shape-mismatch reason instead of indexing absent numeric keys. - inception_v3_mil adds build-time shape asserts on the stem conv and the final linear so an HWIO->OIHW perm or dropped .T fails at convert time. - bench._decode_feature asserts the decoded element count matches (h,w,c). - test_savedmodel_reader asserted weights() raises NotImplementedError, but it is implemented now; assert it raises on a malformed bundle instead.
architecture.md's Phase-0 ADR named Core ML/MIL as the inference backend, but the shipped default is Metal MPSGraph FP32 (call_variants_main.cc --inference_backend=metal; coreml debug-only; ANE not engaged), matching validation.md and scientific_report.md. Mark it SUPERSEDED and correct the Decision, preserving the historical Phase-0 numbers. Fix the conversion README (nonexistent requirements-metal.txt; coremltools>=9.0 TF-free) and the vendored proto count (25->26, matching SOURCES.md).
Determinism-neutral CPU wins on the make_examples hot loop (output unchanged): - std::move the read vector into working_reads on the no-realigner default path instead of deep-copying every Read proto (repointing the one later use). - Hoist loop-invariant absl::GetFlag reads and the RealignerOptions proto to const locals computed once before the region loop. - Emit a periodic 'processed X/Y regions' progress line.
- Attach a 1 MiB streambuf to the reader before open (the writer already coalesces into 1 MiB), cutting per-record read() syscalls. Pure buffering; format and decoded bytes unchanged. - Add a WriteRecord(std::string_view) overload and pass the example payload by view from ExampleWriter::Add instead of materializing a std::string; on-disk framing is byte-identical.
- Add -mcpu=apple-m1 (ISA/scheduling only; fast-math stays off so the deterministic conv/bnns reductions remain bit-identical, guarded by the determinism microtests). - Delete neon_base_color.h + its microtest: built-but-unused in production and semantically divergent from the live FillBaseColorBatch (a latent trap). - Refresh the stale 'NOT YET WIRED' banner on neon_cigar_classify.h, which is in fact wired into allelecounter.cc.
Add --enable_inference_pipelining (default off) that overlaps the GPU MPSGraph backbone of batch N+1 with the CPU BNNS finalize + CVO build of batch N, for the two-stage Metal path only. A single finalize worker drains a FIFO queue and is the sole CVO producer (shared emit_cvos lambda), with a 2-slot double buffer (features/probs) and an at-most-one-in-flight invariant, so output order is identical to the serial path. Validated on chr20 (HG002, 4731 examples): record set, metadata, and order are byte-identical to serial, and the genotype-prob differences are exactly the default metal backend's own run-to-run nondeterminism envelope (serial-vs-serial == serial-vs-pipelined).
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
2 participants
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.
Stacked on #2 — review after it merges, when this diff reduces to its unique commits. Default runtime behavior is unchanged (both new paths are opt-in, default off). Build green;
ctestall pass.Performance (default behavior unchanged)
-mcpu=apple-m1(ISA/scheduling only; fast-math stays off, so the deterministic conv/BNNS reductions remain bit-identical — guarded by the determinism microtests), plusstd::move/reserve/flag-hoisting inmake_examples, TFRecord reader buffering, astring_viewWriteRecordoverload, and removal of a dead/divergent SIMD header.--enable_inference_pipelining(default off): overlaps the GPU MPSGraph backbone of batch N+1 with the CPU BNNS finalize + CVO build of batch N, via a single FIFO finalize worker and a 2-slot double buffer. Validated on chr20 (HG002, 4731 examples): record set, metadata, and order are byte-identical to serial, and the genotype-probability differences equal the defaultmetalbackend's own run-to-run nondeterminism envelope (serial-vs-serial == serial-vs-pipelined).Docs / tests
docs/architecture.md) as superseded — the shipped backend is Metal MPSGraph FP32,coremlis debug-only, and the ANE is not engaged for the 7-channel input.test_savedmodel_readerassertion.Self-review fixes folded in
emit_cvosnow checks bothParseFromStringreturn values instead of silently emitting a half-parsed record.WriterJoinerRAII guard so the async writer thread is joined on every early-return path (it previously hitstd::terminate()on a joinable thread); ordered to destruct after the pipelined finalize worker so its queued records are still drained.