logaritmisk/trueskill-tt
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Activity

Decide whether rayon becomes a default-on feature #5

New Issue
Closed
opened 2026-08-04 18:54:20 +00:00 by logaritmisk · 1 comment
logaritmisk commented 2026-08-04 18:54:20 +00:00
Owner
Copy Link
Copy Source

Deferred from #2 (T3). Spec: docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md §6, "Rayon as default-on feature".

Why

The spec calls for rayon default-on, with default-features = false falling back to sequential iterators behind cfg(feature = "rayon"). T3 deliberately shipped it opt-in instead, pending production confidence.

Cargo.toml today:

[features]
approx = ["dep:approx"]
rayon = ["dep:rayon"]

There is no default = [...] key, so the parallel paths are off unless a downstream crate asks for them.

The case against flipping it now

T3's measured speedups are 1.0× on realistic workloads and 1.3× on a pathological one (see #4). Default-on would hand every downstream user a rayon dependency, a thread pool, and an unsafe concurrent-write path through SkillStore in exchange for approximately no speedup on the shapes people actually run.

The case for flipping it later

Once #4 lands, the parallel story changes and the cost/benefit is worth re-measuring.

Scope

This is a decision issue, not an implementation one. Resolve it as either:

  1. Keep opt-in — update spec §6 to record the deviation and why, and close this.
  2. Flip to default-on — add default = ["rayon"], verify --no-default-features still builds and passes the full suite, and confirm the determinism test (tests/determinism.rs) covers both configurations in CI.

Blocked on

#4 — re-measure after cross-slice parallelism lands. Deciding before that means deciding on numbers we already know are unrepresentative.

Deferred from #2 (T3). Spec: `docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md` §6, "Rayon as default-on feature". ## Why The spec calls for `rayon` default-on, with `default-features = false` falling back to sequential iterators behind `cfg(feature = "rayon")`. T3 deliberately shipped it opt-in instead, pending production confidence. `Cargo.toml` today: ```toml [features] approx = ["dep:approx"] rayon = ["dep:rayon"] ``` There is no `default = [...]` key, so the parallel paths are off unless a downstream crate asks for them. ## The case against flipping it now T3's measured speedups are 1.0× on realistic workloads and 1.3× on a pathological one (see #4). Default-on would hand every downstream user a rayon dependency, a thread pool, and an `unsafe` concurrent-write path through `SkillStore` in exchange for approximately no speedup on the shapes people actually run. ## The case for flipping it later Once #4 lands, the parallel story changes and the cost/benefit is worth re-measuring. ## Scope This is a decision issue, not an implementation one. Resolve it as either: 1. **Keep opt-in** — update spec §6 to record the deviation and why, and close this. 2. **Flip to default-on** — add `default = ["rayon"]`, verify `--no-default-features` still builds and passes the full suite, and confirm the determinism test (`tests/determinism.rs`) covers both configurations in CI. ## Blocked on #4 — re-measure after cross-slice parallelism lands. Deciding before that means deciding on numbers we already know are unrepresentative.
logaritmisk added the blockeddecisionperformance labels 2026-09-07 13:52:00 +00:00
logaritmisk commented 2026-09-08 13:55:03 +00:00
Author
Owner
Copy Link
Copy Source

Resolved as option 1: keep opt-in. Spec §6 records the deviation in 7c6965c; Cargo.toml is unchanged.

Why now rather than after the blocker

This was blocked on #4 — "re-measure after cross-slice parallelism lands. Deciding before that means deciding on numbers we already know are unrepresentative."

That measurement is not coming. #4 was closed on 2026-08-27 by removing the inert ConvergenceReport::slices_skipped field rather than by implementing dirty-bit skipping. So the condition this was waiting on cannot be met, and waiting longer only leaves a decided question open.

The numbers this issue already has therefore stand as the numbers: 1.0x on realistic workloads, 1.3x on a pathological one. Typical slices hold too few events to amortize rayon's task-spawn overhead. Default-on would hand every downstream user a thread pool and a dependency for approximately nothing.

A correction to this issue's own reasoning

The case against flipping cites, alongside the measurements, "an unsafe concurrent-write path through SkillStore". That cost does not exist. The crate is #![forbid(unsafe_code)], and the compute/apply split on the internal Event — compute() reads skills immutably, apply() folds the result back — is precisely what lets a color group run in parallel without it.

So the conclusion is unchanged but the argument is narrower than written: opt-in rests on the measurements alone, not on a safety concern. Both the deviation note and the superseded "Trade-offs" bullet in §6 now say so, since a spec that argues from a non-existent hazard would mislead whoever revisits this.

If cross-slice parallelism is ever implemented, this is worth reopening — the note names that explicitly so the trade is re-examined rather than treated as settled forever.

Resolved as **option 1: keep opt-in**. Spec §6 records the deviation in `7c6965c`; `Cargo.toml` is unchanged. ## Why now rather than after the blocker This was blocked on #4 — "re-measure after cross-slice parallelism lands. Deciding before that means deciding on numbers we already know are unrepresentative." That measurement is not coming. #4 was closed on 2026-08-27 by **removing** the inert `ConvergenceReport::slices_skipped` field rather than by implementing dirty-bit skipping. So the condition this was waiting on cannot be met, and waiting longer only leaves a decided question open. The numbers this issue already has therefore stand as the numbers: 1.0x on realistic workloads, 1.3x on a pathological one. Typical slices hold too few events to amortize rayon's task-spawn overhead. Default-on would hand every downstream user a thread pool and a dependency for approximately nothing. ## A correction to this issue's own reasoning The case against flipping cites, alongside the measurements, "an `unsafe` concurrent-write path through `SkillStore`". **That cost does not exist.** The crate is `#![forbid(unsafe_code)]`, and the compute/apply split on the internal `Event` — `compute()` reads skills immutably, `apply()` folds the result back — is precisely what lets a color group run in parallel without it. So the conclusion is unchanged but the argument is narrower than written: opt-in rests on the measurements alone, not on a safety concern. Both the deviation note and the superseded "Trade-offs" bullet in §6 now say so, since a spec that argues from a non-existent hazard would mislead whoever revisits this. If cross-slice parallelism is ever implemented, this is worth reopening — the note names that explicitly so the trade is re-examined rather than treated as settled forever.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
api
Shape or ergonomics of the public surface
 blocked
Waiting on another issue or an external decision
 breaking
Resolving it changes the public API; wants a major bump
 bug
Behaviour is wrong, including silently wrong
 decision
Needs a call to be made, not code to be written
 docs
Documentation, including doc comments and the README
 enhancement
New capability the crate does not have
 infra
CI, release process, packaging
 numerics
Floating-point accuracy, cancellation, convergence maths
 performance
Speed or memory
 refactor
Internal cleanup with no intended behaviour change
 testing
Test coverage and test infrastructure
No labels blocked decision performance
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logaritmisk
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#5