Skip to content

Convert train-movement event detection to kDisco waitCrossing state events (Issue #750, step 1)#756

Open
bedaHovorka with Copilot wants to merge 6 commits into
toKdisco0.6.1from
copilot/fix-zero-crossing-detection
Open

Convert train-movement event detection to kDisco waitCrossing state events (Issue #750, step 1)#756
bedaHovorka with Copilot wants to merge 6 commits into
toKdisco0.6.1from
copilot/fix-zero-crossing-detection

Conversation

Copilot AI commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

fast-sim spends 60–70% of its wall time in RKF45 continuous integration because Generator hardcodes dtMax=1e-3, capping every step at 1 ms (~1.85M derivative evals/run). That tiny dtMax is load-bearing: block-boundary crossings are detected purely by step granularity, so raising it breaks the simulation. This converts the two train-movement event detections to kDisco's new waitCrossing { guard } root-finding — step 1 of the issue's proposed direction.

Depends on kDisco PR #68 (waitCrossing); interlockSim CI cannot compile until that is merged and published.

Changes (sim/Train.kt)

  • Block-boundary crossing (Site.actions): waitUntil { position.state + dtMin >= nextLength } → root-found state event. The dtMin slack is retained to avoid an asymptotic-stop hang when a train decelerates to a STOP semaphore sitting on the boundary and settles ~1e-8 short.
  • Tail-entry crossing (Train.actions): waitUntil { front.totalDistance >= length } → root-found state event with the same dtMin slack.
  • Re-added import cz.ksimulantenbande.kdisco.dtMin.
// before
waitUntil { position.state + dtMin >= nextLength }

// after — crossing time located by root-finding, independent of step size
val boundaryThreshold = nextLength - dtMin
if (position.state < boundaryThreshold) {
    waitCrossing { boundaryThreshold - position.state }
}

Generator.kt

  • dtMax left at 1e-3. See below.

Not yet addressed — dtMax stays at 1e-3

Raising dtMax still collapses the run (5 → 2 trains), so the perf win is deferred. Two root causes were isolated; both need a Motor redesign, golden re-baselining, and expert sign-off under the conservative sim/ rules:

  • Asymptotic Motor stop — deceleration a = -v²/2s stops via a velocity-clamp, undershooting the boundary by ~O(dtMax²) (1.1e-8 @ 1e-3, 2.2e-5 @ 1e-2), so a fixed tolerance cannot absorb it as dtMax grows. Fix direction: waitCrossing { distanceToSemaphore() } to stop exactly at s=0.
  • Front/tail velocity-clamp driftMotor.derivatives() mutates the shared velocity.state in place; front and tail then integrate against different velocities, drifting |front − tail − length| to 0.0246 @ dtMax=1.0 (> maxAbsError=1e-2, LengthChecker FATAL). Fix direction: an order-independent clamp.

Validation notes

  • Behavior is identical to the golden baseline at dtMax=1e-3 (5 trains, 75 TRAIN_EVENTS lines, 213.1 s sim) — the conversion introduces no drift.
  • Scope is limited to Train.kt and Generator.kt; Motor, velocity-clamp, and LengthChecker are unchanged from the original.
  • At dtMax=1e-3 this is groundwork with no runtime speedup on its own; the win lands once the Motor state-event work above is complete. :core:heavyTest and re-baselining still pending.

…lity (~9% fast-sim win) (#754)

* fix(#749): gate findNextReservationTarget on reservation-eligibility

ShuntingLoop.toBlockInputObservation resolved BlockInputObservation.toSeparatorName
by calling PathReservationService.findNextReservationTarget for every block input on
every tick, including inputs that provably cannot take a forward reservation.

The call is a BFS (findNextSemaphoresVia) plus a full topological-path enumeration per
candidate (isPathAvailable -> findAllTopologicalPaths). Measured on vyhybna: 906 calls
per `example shuntingLoop 300`, of which 892 (98.5%) are discarded unread by
RuleBasedDispatcher.checkInput, which already returns null for FREE, not-approaching
and already-extended inputs. Cost: 30.3 ms of 342 ms wall (~9%).

Gate the search on eligibility. The three facts it needs (isApproachingThisInput,
pathSetUpTowardThisInput, pathAlreadyExtendedBeyond) are already computed in the same
function, so the gate is free.

This narrows the sensor-port contract: toSeparatorName is now populated only where a
forward reservation is possible, and is null elsewhere without the search having run.
RuleBasedDispatcher behaviour is unchanged by construction. Documented on
BlockInputObservation so a future LLM dispatcher does not read null as "track ahead is
occupied".

Native linuxX64 release, example shuntingLoop 300, median of 7:
  before  341 ms
  after   318 ms
Golden output byte-for-byte identical (90/90 event lines, 5 trains, 211.1 s sim time).

ShuntingLoopReservationTargetLazinessTest locks the contract and fails 2/2 without the
gate (a FREE input resolves k1->doA1=A).

Gates: core:jvmTest 2194, core:integrationTest 725, core:heavyTest 1001,
dispatcher-agent:test 51, desktop-ui:test 650, desktop-ui:integrationTest 247 — all
green. purity/detekt/ktlint clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Extract reservation-target helper

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix excessive derivative evaluations in zero-crossing detection Convert train-movement event detection to kDisco waitCrossing state events (Issue #750, step 1) Jul 11, 2026
Copilot AI requested a review from bedaHovorka July 11, 2026 20:35
@bedaHovorka bedaHovorka marked this pull request as ready for review July 12, 2026 03:29
@bedaHovorka

bedaHovorka commented Jul 12, 2026

Copy link
Copy Markdown
Owner

After publishing the new kDisco snapshot, do only this in PR #756:

  1. Bump kDisco dependency in interlockSim to that new snapshot version.
  2. Refresh dependencies and verify waitCrossing resolves in Train.kt.
  3. Push the dependency bump commit to the PR branch.
  4. Re-run CI pipeline (same failing workflow/job).
  5. If green, continue with next checks

bedaHovorka and others added 5 commits July 12, 2026 06:27
… JMH

`:core:integrationTest` failed on run 29165254178 (725 tests, 1 failed).
The failure was DefaultSimulationContextControllerTest.controlledLoopOverheadIsNegligible,
a wall-clock microbenchmark living inside the CI correctness gate: it timed one
300s ShuntingLoop run per arm and asserted the ratio was < 5%. It measured 5.33%.

It could not have held up. One sample per arm, no JIT warmup (the baseline arm ran
first and absorbed it, biasing the ratio negative, so it passed for a reason unrelated
to the property it claimed to test), and a 5% threshold on a shared runner whose
CPU-steal variance far exceeds 5%.

Note TwoTrainConcurrencyTest passed 150/150 in that run and is not implicated; the
simulation is a single-threaded kDisco discrete-event loop with a seeded RNG, so there
is no race here to serialize.

- Delete the wall-clock assertion. The class keeps its four deterministic tests
  (throttle invoked per event, non-negative deltas, StopSimulation propagates,
  awaitIfPaused called), which is what a correctness gate should assert.

- Add ControlledLoopOverheadBenchmark to the existing :desktop-ui JMH harness, with
  two independently reported arms instead of one fragile ratio. Only ctx.run(controller)
  is timed; context construction sits in @setup(Level.Invocation). Mode is SingleShotTime:
  the payload is a single-use, millisecond-scale run, and under AverageTime the resulting
  context churn collides Koin scope ids, since DefaultSimulationContext derives its scope
  id from a (non-unique) identity hash code.

  Measured: baseline 0.699 +/- 0.103 ms/op, with-controller 0.685 +/- 0.062 ms/op.
  Indistinguishable -- the hook overhead is below the noise floor, so the original
  claim was true; only the instrument was broken.

- Fix a latent order-dependent bug in TwoTrainConcurrencyTest found en route. It matched
  train names by substring, and Train.countValue is a process-global counter that is never
  reset, so once names reach two digits "Train #1" matches "Train #12". Now compares
  extracted whole tokens and indexes by identity rather than List.indexOf.

Verified: :core:integrationTest 724/724 (was 725/1 failed), :core:jvmTest 2194/2194,
:desktop-ui:test + integrationTest green, detekt + ktlintCheck clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…de.kdisco)

kDisco PR #54 renamed its Maven group and Kotlin package from the
maintainer's personal namespace to the neutral cz.ksimulantenbande.kdisco
and added public-domain license headers. Update all consumer-side
references (dependency coordinates, imports, checkKdisco path, docs) to
match.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…itCrossing state events; keep dtMax=1e-3 pending Motor redesign
…ect comment, add regression tests

Verifies and closes out the waitCrossing conversion from 16fb70c now that
kDisco 0.6.1-SNAPSHOT (with waitCrossing) is locally publishable and the
build works again:

- Generator.kt: correct a comment that overclaimed velocity-target events
  are root-found — only block-boundary and tail-entry were converted;
  Motor's AccelerationStopCondition still uses plain waitUntil. The
  Motor/dtMax follow-up is now tracked separately in issue #760.

- SimpleTestProcess.kt: fix a real regression exposed by the conversion.
  This test coordinator never runs Generator, so it silently relied on
  kDisco's own raw defaults (dtMin=1e-5, maxAbsError=1e-5 — nearly equal
  in magnitude). The waitCrossing guards intentionally leave a structural
  gap of exactly `dtMin` at the moment a threshold is reached (see the
  existing Train.kt comments on why that slack is necessary), which is
  negligible against the intended maxAbsError=1e-2 but trips
  LengthChecker's invariant when left at kDisco's near-equal raw
  defaults. Confirmed via A/B test against 74cd41b (same kDisco version,
  pre-conversion Train.kt) that this is new, not pre-existing. Fixed by
  configuring the same tolerances Generator.startAction() sets.

- SimpleTestProcessTest.kt: relax an assertion that assumed `position`
  stays >= 0 immediately after a block-boundary crossing. The
  waitCrossing guard deliberately fires `dtMin` short of the boundary so
  it reliably crosses zero instead of asymptoting forever; this leaves a
  bounded, harmless negative transient that was never a documented
  invariant.

- SimpleLinearTrackTestProcessTest.kt: add an explicit regression test
  proving both converted call sites fire correctly end-to-end.

Verified locally: kDisco published to mavenLocal from
copilot/add-zero-crossing-detection (feaa058), full build/test/
integrationTest/heavyTest all green (0 failures across all JUnit reports).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@bedaHovorka bedaHovorka force-pushed the copilot/fix-zero-crossing-detection branch from fe07131 to 9a7ce9e Compare July 12, 2026 06:24
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.

Zero-crossing event detection: dtMax=1e-3 forces 1.85M derivative evals/run (60-70% of fast-sim wall)

2 participants