perf(search): instrumentation + dead-sync removal + per-task buffers + ForkJoinPool toggle

.claude/plans/parameter-modernization-flag-inventory.md

@@ -0,0 +1,90 @@
+# MS-GF+ flag inventory (Phase 1 input)
+
+Snapshot of every flag registered by `ParamManager.addMSGFPlusParams()`
+plus the parsing semantics each one currently relies on. This is the
+foundation document for the Phase 1 picocli rewrite described in
+`parameter-modernization.md`. Total: 34 flags (27 visible + 7 hidden).
+Required: `-s`, `-d`.
+
+## Visible flags
+
+| Short | Canonical name | Type | Default | Bounds | Notes |
+|---|---|---|---|---|---|
+| `-conf` | `ConfigurationFile` | file | — | exists | Config file; CLI overrides config |
+| `-s` | `SpectrumFile` | file/dir | — | exists | **Required.** mzML/mzXML/mgf/ms2/pkl/_dta.txt or directory |
+| `-d` | `DatabaseFile` | file | — | exists | **Required.** *.fasta / *.fa / *.faa |
+| `-decoy` | `DecoyPrefix` | string | `DECOY_` | — | Decoy protein prefix |
+| `-o` | `OutputFile` | file | `<spec>.pin` | — | *.pin (default) or *.tsv |
+| `-t` | `PrecursorMassTolerance` | tolerance | `20ppm` | ≥0 | Symmetric (`20ppm`) or asymmetric (`0.5Da,2.5Da`); units must match |
+| `-ti` | `IsotopeErrorRange` | int range | `0,1` | ≥0, max-incl | Isotope-error window, both ends inclusive |
+| `-m` | `FragmentationMethodID` | dyn-enum | `ASWRITTEN` | — | 0=as-written, 1=CID, 2=ETD, 3=HCD |
+| `-inst` | `InstrumentID` | dyn-enum | `LOW_RES_LTQ` | registry | `InstrumentType` registry-driven |
+| `-e` | `EnzymeID` | dyn-enum | `TRYPSIN` | registry | `Enzyme` registry-driven |
+| `-protocol` | `ProtocolID` | dyn-enum | `AUTOMATIC` | registry | `Protocol` registry-driven |
+| `-ntt` | `NTT` | enum | `2` | 0..2 | Number of tolerable termini |
+| `-mod` | `ModificationFile` | file | built-in (C+57) | exists | Mod file; config-file path also accepts `StaticMod=`/`DynamicMod=`/`CustomAA=` |
+| `-minLength` | `MinPepLength` | int | `6` | ≥1 | |
+| `-maxLength` | `MaxPepLength` | int | `40` | ≥1 | |
+| `-minCharge` | `MinCharge` | int | `2` | ≥1 | |
+| `-maxCharge` | `MaxCharge` | int | `3` | ≥1 | |
+| `-n` | `NumMatchesPerSpec` | int | `1` | ≥1 | |
+| `-thread` | `NumThreads` | int | `Runtime.availableProcessors()` | ≥1 | |
+| `-tasks` | `NumTasks` | int | `0` (auto) | ≥-10 | 0=auto, >0=fixed, <0=N×threads |
+| `-minSpectraPerThread` | `MinSpectraPerThread` | int | `250` | ≥1 | |
+| `-verbose` | `Verbose` | enum | `0` | 0..1 | 0=total, 1=per-thread |
+| `-tda` | `TDA` | enum | `0` | 0..1 | 0=no decoy, 1=concat decoy search |
+| `-addFeatures` | `AddFeatures` | enum | `0` | 0..1 | Percolator extra features |
+| `-outputFormat` | `OutputFormat` | enum | `pin` | pin/tsv | mzIdentML removed |
+| `-precursorCal` | `PrecursorCal` | string | `auto` | auto/on/off | Case-insensitive |
+| `-ccm` | `ChargeCarrierMass` | double | `1.00727649` | >0.1 | Proton mass default |
+| `-maxMissedCleavages` | `MaxMissedCleavages` | int | `-1` | ≥-1 | -1 = unlimited |
+| `-numMods` | `NumMods` | int | `3` | ≥0 | Max dynamic mods per peptide |
+| `-allowDenseCentroidedPeaks` | `AllowDenseCentroidedPeaks` | enum | `0` | 0..1 | |
+| `-msLevel` | `MSLevel` | int range | `2,2` | ≥1, max-incl | `min,max` or single |
+| `-u` | `PrecursorMassToleranceUnits` | enum | `2` | 0..2 | **Hidden** — legacy; 0=Da, 1=ppm, 2=as-written |
+
+## Hidden flags
+
+| Short | Canonical name | Type | Default | Notes |
+|---|---|---|---|---|
+| `-dd` | `DBIndexDir` | dir | — | Database index dir |
+| `-index` | `SpecIndex` | int range | `1,INT_MAX-1` | Spectrum index range, both inclusive |
+| `-edgeScore` | `EdgeScore` | enum | `0` | 0=use, 1=skip |
+| `-minNumPeaks` | `MinNumPeaks` | int | `Constants.MIN_NUM_PEAKS_PER_SPECTRUM` | |
+| `-iso` | `NumIsoforms` | int | `Constants.NUM_VARIANTS_PER_PEPTIDE` | |
+| `-ignoreMetCleavage` | `IgnoreMetCleavage` | enum | `0` | 0=consider, 1=ignore |
+| `-minDeNovoScore` | `MinDeNovoScore` | int | `Constants.MIN_DE_NOVO_SCORE` | |
+
+## Sharp edges the picocli rewrite must preserve
+
+1. **Asymmetric tolerance.** `-t 0.5Da,2.5Da` → left tolerance (observed < theoretical) ≠ right tolerance. Both sides must use the same unit. Numeric-only value (e.g. `20`) defaults to Da. Trailing unit suffix is case-insensitive (`Da`/`ppm`/`Th`).
+2. **Range inclusivity is per-flag.** `IntRangeParameter` defaults to `min` inclusive / `max` exclusive, but `-ti`, `-index`, `-msLevel` flip max to inclusive via `.setMaxInclusive()`.
+3. **Dynamic enums.** `-inst`, `-e`, `-protocol`, `-m` are registry-driven (`InstrumentType`, `Enzyme`, `Protocol`, `ActivationMethod`). Numeric indices depend on registry load order; help text is generated at startup. Picocli converters must read from the same registries, not hardcode indices.
+4. **`OutputFormat` legacy mapping is gone.** Old `0=mzIdentML`, `2=both` are no longer accepted; only `pin` (0) and `tsv` (1) remain. Numeric indices are deprecated but still parse internally.
+5. **`-precursorCal` is a string, not an enum class.** Values: `auto` / `on` / `off` (case-insensitive, `.trim()`-ed). `auto` means "run pre-pass, apply only if ≥200 confident PSMs collected".
+6. **Trailing `!` on numbers.** `IntParameter` and `DoubleParameter` strip trailing `!` (legacy DMS config-file integration). Decide if Phase 1 keeps this quirk.
+7. **`-tasks` semantics.** `0` = auto, `>0` = fixed, `<0` = `N × threads`. Range allows down to `-10`.
+8. **Config-file-only entries.** `StaticMod=`, `DynamicMod=`, `CustomAA=` are not CLI flags. They're parsed from `-mod` file and `-conf` config file only. Repeated entries are *expected* (each line is a separate mod). Config parser preserves order.
+9. **Config-file aliases (canonical-name normalization in `ParamNameEnum.getParamNameFromLine()`).** Auto-renames at least 13 deprecated keys:
+   - `IsotopeError` → `IsotopeErrorRange`
+   - `TargetDecoyAnalysis` → `TDA`
+   - `FragmentationMethod` → `FragmentationMethodID`
+   - `Instrument` → `InstrumentID`
+   - `Enzyme` → `EnzymeID`
+   - `Protocol` → `ProtocolID`
+   - `NumTolerableTermini` → `NTT`
+   - `MinNumPeaks` → `MinNumPeaksPerSpectrum`
+   - `MaxNumMods` / `MaxNumModsPerPeptide` → `NumMods`
+   - `minLength` / `MinPeptideLength` → `MinPepLength`
+   - `maxLength` / `MaxPeptideLength` → `MaxPepLength`
+   - `PMTolerance` / `ParentMassTolerance` → `PrecursorMassTolerance`
+10. **File-format validation chain.** Order: directory-vs-file → format-suffix match → existence → no-reuse. Suffix matching is case-insensitive for `.pin`/`.tsv`/`.fasta`. Spec parameter auto-allows directories.
+11. **Defaults that depend on runtime.** `-thread` defaults to `Runtime.getRuntime().availableProcessors()` (includes hyperthreading; per CLAUDE.md, physical cores often give better wall-time).
+12. **Help-text drift.** Existing tests likely compare exact `--help` output. picocli's formatter is different. Decide: snapshot-update vs. custom renderer that mimics current format.
+
+## Out-of-scope reminders for Phase 1
+
+- `MSGFDB`, `MSGF`, `MSGFLib` entry points share `ParamManager`. Phase 1 only modernizes `MSGFPlus`; the other three keep using `ParamManager.parseParams()` until Phase 4.
+- Config-file parsing is Phase 2. Phase 1 covers CLI only.
+- The `Parameter` / `IntParameter` / `IntRangeParameter` / `ToleranceParameter` / etc. hierarchy is **not** removed in Phase 1. Removal is Phase 3.
+- `ParamManager` itself stays. Phase 1 adds an adapter that produces a populated `ParamManager` from the typed `MSGFPlusOptions`, so `SearchParams.parse(ParamManager)` is unchanged.

.claude/plans/parameter-modernization.md

@@ -0,0 +1,159 @@
+# Plan: modernize MS-GF+ parameter handling
+
+**Status: proposed**
+Branch: `perf/search-sync-cleanup` (worktree at
+`/Users/yperez/work/msgfplus-workspace/search-sync-cleanup`).
+
+## Why this exists
+
+The current parameter stack under `edu.ucsd.msjava.params` is doing
+several jobs at once:
+- command-line parsing
+- type conversion
+- validation
+- help/usage rendering
+- config-file alias handling
+- backward-compatibility shims
+
+That works, but it spreads option behavior across many small classes
+(`Parameter`, `NumberParameter`, `RangeParameter`, `ToleranceParameter`,
+`FileParameter`, enum wrappers, and `ParamManager`). The result is more
+code than we need for a solved problem and a higher risk of subtle
+parsing drift when new flags are added.
+
+## Goals
+
+- Reduce the amount of custom CLI parsing code.
+- Keep existing MS-GF+ command-line behavior stable where practical.
+- Preserve current config-file semantics in the first migration step.
+- Keep `SearchParams` as the internal domain model for search settings.
+- Improve help/usage generation and validation error consistency.
+
+## Non-goals
+
+- No search algorithm changes.
+- No performance claim for the search itself; parsing happens once at
+  startup and is not a runtime hotspot.
+- No forced removal of legacy config-file aliases in phase 1.
+- No broad package cleanup bundled into this effort.
+
+## Recommended direction
+
+Adopt `picocli` for command-line parsing and help generation, while
+keeping a thin MSGF+-specific compatibility layer for:
+- legacy option names and aliases
+- config-file parsing
+- repeated modification/custom-AA entries
+- conversion into `SearchParams`, `AminoAcidSet`, `Tolerance`, and
+  related domain objects
+
+## Proposed migration shape
+
+### Phase 1: introduce a typed CLI model beside `ParamManager`
+
+- Add a new options class for `MSGFPlus` under `edu.ucsd.msjava.cli`.
+- Represent flags as typed fields with defaults, required markers,
+  and descriptions.
+- Add custom `picocli` converters for:
+  - precursor mass tolerance
+  - integer and float ranges
+  - output format
+  - precursor calibration mode
+  - file/directory validation
+- Keep `ParamManager` intact during this phase.
+- Add an adapter that maps parsed CLI options into the current
+  `SearchParams` inputs.
+
+Success criteria:
+- `MSGFPlus` can parse its current CLI arguments through the new path.
+- Generated help text is complete and readable.
+- Existing tests for parameter behavior still pass or are updated
+  mechanically where output formatting differs.
+
+### Phase 2: preserve config-file compatibility explicitly
+
+- Keep `ParamParser` or replace it with a thinner reader that still
+  accepts the current `key=value` format.
+- Centralize legacy config-name alias resolution in one place instead
+  of scattering it through `ParamNameEnum`.
+- Support repeated config entries for:
+  - `DynamicMod`
+  - `StaticMod`
+  - `CustomAA`
+- Feed config values into the same typed options model used by CLI.
+
+Success criteria:
+- Existing example parameter files still load.
+- Duplicate-entry behavior for mods/custom amino acids is preserved.
+- Command-line values continue to override config-file values.
+
+### Phase 3: move validation out of the custom parameter hierarchy
+
+- Replace per-type `parse()` methods with:
+  - `picocli` conversion
+  - explicit validation methods on the typed options object
+  - targeted domain-level validation while building `SearchParams`
+- Collapse or remove custom classes that are no longer needed:
+  - `Parameter`
+  - `NumberParameter`
+  - `RangeParameter`
+  - `IntParameter`
+  - `FloatParameter`
+  - `DoubleParameter`
+  - `IntRangeParameter`
+  - `FloatRangeParameter`
+  - enum parameter wrappers
+
+Success criteria:
+- No user-visible behavior regressions on required flags, defaults,
+  range checks, or enum choices.
+- Validation failures still produce actionable messages.
+
+### Phase 4: reduce `ParamManager` to compatibility-only or retire it
+
+- If any remaining tools still depend on `ParamManager`, keep it only as
+  a compatibility facade over the new parser.
+- Otherwise remove `ParamManager` from the active CLI path.
+- Decide whether `MSGFDB` migrates in the same PR series or follows
+  after `MSGFPlus` is stable.
+
+## Main risks
+
+- Help text and error messages may change in ways that break tests or
+  documentation.
+- Config-file behavior is more important than it looks; it includes
+  legacy aliases and repeated entries that generic CLI libraries do not
+  model by default.
+- `MSGFDB` and `MSGFPlus` share parts of the current stack, so an
+  incomplete migration could increase duplication before it decreases.
+
+## Validation plan
+
+- Add focused tests for:
+  - required arguments
+  - default values
+  - bad range syntax
+  - enum parsing
+  - file existence checks
+  - config-file override precedence
+  - repeated modification/custom-AA entries
+- Keep existing `SearchParams` tests green.
+- Run at least one end-to-end `MSGFPlus` smoke test on a known fixture.
+- Compare old vs new parser outcomes for a representative set of real
+  command lines and config files.
+
+## Suggested implementation order
+
+1. Add `picocli` dependency.
+2. Build a typed `MSGFPlusOptions` class and converters.
+3. Parse CLI into the new options class without removing `ParamManager`.
+4. Add an adapter into the current `SearchParams` build path.
+5. Port config-file handling.
+6. Remove unused custom parameter classes.
+7. Migrate `MSGFDB` only after `MSGFPlus` is stable.
+
+## Recommendation on branch strategy
+
+Do this in a dedicated refactor branch, not as part of a performance
+cleanup PR. The expected win is maintainability and correctness, not
+search throughput, and the surface area touches the public CLI.

.claude/plans/search-sync-cleanup.md

@@ -0,0 +1,133 @@
+# Plan: search-path sync cleanup + per-task result buffers
+
+**Status: SHIPPED in PR #25** (https://github.com/bigbio/msgfplus/pull/25)
+Branch: `perf/search-sync-cleanup` (worktree at
+`/Users/yperez/work/msgfplus-workspace/search-sync-cleanup`).
+
+Successor to PR #24. Pure refactor + instrumentation — no scoring,
+parser, or `.pin` feature changes. Output bit-identical to dev's tip
+on every measurable axis.
+
+## What shipped (6 commits)
+
+1. **T1 — per-task wall stats + tail-imbalance summary**
+   `RunMSGFPlus` captures preprocess / db-search / compute-evalue /
+   total wall into a `TaskWallStats` accessor; `MSGFPlus.runMSGFPlus`
+   prints a one-line summary at end of search:
+   ```
+   Task wall summary (n=12): min=101.7s median=224.2s p95=246.4s
+     max=246.4s total=2356.7s tail_gap=22.2s (10% of median)
+   ```
+   On Astral the measured `tail_gap` is **10 % of median**, which means
+   T2 and T3 can't deliver substantial wins on this workload.
+
+2. **Drop dead `synchronized` wrappers in DBScanner + ScoredSpectraMap.**
+   Each instance is task-local (verified: no internal fork-out in
+   `dbSearch`, no shared instance across threads). Plain `HashMap` /
+   `TreeMap` replace the `Collections.synchronizedMap` /
+   `synchronizedSortedMap` wrappers; `synchronized` modifier dropped
+   from `addDBMatches`, `generateSpecIndexDBMatchMap`,
+   `addResultsToList`, `addDBSearchResults`. Memory-visibility safety
+   preserved via `awaitTermination`'s happens-before.
+
+3. **Per-task local result buffers + final merge.**
+   Replaced the global `Collections.synchronizedList<MSGFPlusMatch>`
+   with a per-task `ArrayList`. Each `RunMSGFPlus` owns its own buffer;
+   main thread drains all buffers after `awaitTermination`.
+   `RunMSGFPlus`'s constructor drops the `resultList` parameter; new
+   `getResults()` accessor.
+
+4. **T2 — `-Dmsgfplus.numTasksPerThread=N`** (default 3, unchanged).
+   Lets operators raise the multiplier on datasets where T1's
+   `tail_gap` shows real imbalance.
+
+5. **T3 — `-Dmsgfplus.useForkJoin=true`** (default false, unchanged).
+   Opt-in `ForkJoinPool` swap. Default keeps
+   `ThreadPoolExecutorWithExceptions` (which retains progress
+   reporting + exception-capture-via-afterExecute). FJP path uses
+   `Future.get()` for exception propagation.
+
+6. **Polish — tighter result-buffer merge + `drainResultsTo` + reused
+   null sink.** Static `NULL_PRINT_STREAM` cached instead of allocated
+   per `run()`; `drainResultsTo(dest)` clears per-task buffers
+   immediately after merge so heap is collectible; pre-size merged
+   `ArrayList` to `sum(t.getResultCount())` to avoid resize-and-copy;
+   `submittedTasks.clear()` after summary drops strong refs to all 12
+   task instances before the FDR / write phase.
+
+## Validation gate cleared (Astral 3-arm + Percolator)
+
+Astral 3-arm cold, 8 GB heap, 4 threads, default sysprops.
+**All 8 parity numbers bit-identical to dev's tip:**
+
+| Metric | dev | this branch |
+|---|---:|---:|
+| armB raw targets | 89,479 | 89,479 ✓ |
+| armB raw decoys | 46,792 | 46,792 ✓ |
+| armB 1 % FDR targets | 35,818 | 35,818 ✓ |
+| armB 5 % FDR targets | 40,408 | 40,408 ✓ |
+| armC raw targets | 89,360 | 89,360 ✓ |
+| armC raw decoys | 46,913 | 46,913 ✓ |
+| armC 1 % FDR targets | 35,767 | 35,767 ✓ |
+| armC 5 % FDR targets | 40,426 | 40,426 ✓ |
+
+Walltime delta vs master in the same run:
+- armB: 752.2s vs 848.8s = **−11.4 %**
+- armC: 798.2s vs 848.8s = **−5.9 %**
+
+(First run came in with armC at 6298s; root-caused to OS thrashing —
+load avg 5-8, ~120 MB free RAM, 165M page reclaims, Rancher VM eating
+1 GB. Re-ran after stopping Rancher; wall normalized. Not a code
+issue. Documented in PR #25 description.)
+
+## What we learned vs. expected wins
+
+The plan predicted:
+- Step 1 (sync removal): 0–2 % wall. Possibly negative if biased
+  locking was helping. Code clarity is the more reliable win.
+- Step 2 (per-task buffers): 2–8 % wall, scaling with PSM count.
+- T2 / T3: only worth doing if profiler shows real tail-imbalance.
+
+What we measured:
+- Combined wall improvement: **11.4 % on armB, 5.9 % on armC** —
+  better than the upper end of the per-step predictions, suggesting
+  the gains compound (less monitor traffic + cheaper drain phase).
+- T1's measured tail_gap on Astral: **10 % of median** — small enough
+  that T2/T3 default-on would give marginal wins. They ship as opt-in
+  knobs precisely so they don't gate the default behavior.
+
+## What this branch is NOT
+
+Not a fragment-index revival. Not a primitive mass-window port. Not
+a peak-storage refactor (`Peak` → `float[]`). Not a CLI / format
+change. Originated from a third-party review of PR #24.
+
+## Follow-ups (out of scope for this PR)
+
+- **Profile on TMT and a metaproteomic FASTA** with the new T1
+  summary. Astral's 10 % tail_gap might not represent uneven
+  workloads — homolog-rich DBs are the place T2/T3 should bite.
+- **`DatabaseMatch.indices` from `TreeSet<Integer>` to primitive
+  `int[]`** (M1 from the broader memory-roadmap discussion). Highest
+  expected impact for homolog-heavy databases (5-12× memory reduction
+  per match); needs a metaproteomic test fixture to validate.
+- **Parser cache stores raw `float[] mz, float[] intensity`** (M3),
+  with a fresh `Spectrum` built per `getSpectrumBySpecIndex`. Side
+  benefit: cache-layer immutability instead of cloneSpectrum.
+- **`Peak`/`Spectrum` storage refactor** (M2). Multi-PR. Big surface
+  area. Defer until M1 + M3 land.
+
+## Open questions resolved
+
+- **Did the custom `ThreadPoolExecutorWithExceptions` preserve
+  awaitTermination's happens-before on the exception path?** Yes —
+  observed bit-identical results in armB / armC across the 3-arm
+  benchmark, which would not be the case if visibility were broken.
+
+- **Was HotSpot already eliding the uncontended monitors?** Probably
+  partially. Step 2 (sync removal) on its own gives an unmeasured
+  delta; combined with steps 3–6 the total is 11.4 %. We can't
+  attribute that 11.4 % to any single commit without per-commit
+  benchmarks, but the polish commit (#6) likely contributes
+  meaningfully via the pre-sized `ArrayList` and immediate
+  per-task-buffer release.