Skip to content

Widen expected-MI cache keys to the full integer range - #141

Merged
ms609 merged 3 commits into
cpp-searchfrom
feature/mi-key-cache-aliasing
Aug 7, 2026
Merged

Widen expected-MI cache keys to the full integer range#141
ms609 merged 3 commits into
cpp-searchfrom
feature/mi-key-cache-aliasing

Conversation

@ms609

@ms609 ms609 commented Aug 5, 2026

Copy link
Copy Markdown

Fixes #132

mi_key() narrowed every block size to uint16_t before hex-encoding it, so
two partitions whose block sizes differed by a multiple of 65536 received the
same .ExpectedMICache key, and the second was served the first one's expected
mutual information.

TreeSearch:::mi_key(c(3L, 65597L), c(30L, 31L))  # "0003003d001e001f"
TreeSearch:::mi_key(c(3L, 61L),    c(30L, 31L))  # "0003003d001e001f"

TreeSearch:::.ExpectedMI(c(3L, 61L),    c(30L, 31L))  # 0.01537134  correct
TreeSearch:::.ExpectedMI(c(3L, 65597L), c(30L, 31L))  # 0.01537134  cached
expected_mi(c(3L, 65597L), c(30L, 31L))               # 4.044254e-07  true

38 000x wrong, with no diagnostic. ClusteringConcordance(normalize = TRUE)
would have subtracted that as its chance correction.

The fix

uint32_t spans the whole of int, and the int -> uint32_t conversion is
injective across that whole range, so distinct block sizes now always give
distinct keys — the collision is impossible rather than merely improbable.
Values are encoded in 8 hex characters instead of 4; the key roughly doubles in
length, which costs nothing in a hashed environment.

mi_key()'s signature is unchanged, so Rcpp::compileAttributes() on this tree
leaves src/RcppExports.cpp and R/RcppExports.R byte-identical (verified via
.claude/tools/compile-attrs.R) and src/ts_rcpp.cpp and
src/TreeSearch-init.c are untouched. No collision with #122 or #56.

The two sort invariances are sound, and stay

mi_key() canonicalises by sorting ni and nj, which is only legitimate if
expected_mi() is invariant under both. It is. Over 2000 random partitions with
N in 6..400 and 2-5 blocks:

canonicalisation max relative difference
swapping the two blocks of ni 2.8e-13
permuting nj 1.5e-15

Both are floating-point noise, so sorting is not a second, size-independent
aliasing bug. That is worth stating explicitly, because had either invariance
failed the key would have been wrong at every tree size rather than only above
65535.

How reachable was it?

Remote, and worth saying so plainly rather than overselling the fix.

A block of 65536 requires a character scored across at least 65536 tips, so it
needs a tree of at least that many tips. ClusteringConcordance() — the only
caller of the cache — first evaluates as.logical(as.Splits(tree)), which at
65536 tips is a 65533 x 65536 logical matrix: 16 GB, which apply() then
copies. Measured scaling on this build (5 characters) is close to quadratic:

tips time splits matrix
100 0.018 s 0.05 MB
200 0.044 s 0.17 MB
400 0.102 s 0.65 MB
800 0.333 s 2.53 MB

which extrapolates to roughly 40 minutes and ~32 GB at the threshold. So it is
not unreachable — a large-memory machine could do it — but nobody is hitting it
today, and no published result is affected. The case for fixing it is that the
failure is silent and unbounded while the fix is confined to the encoder.

mi_key() is internal (not in NAMESPACE); expected_mi() is exported but
uncached, so a direct call was never affected.

Tests

Three assertions added to tests/testthat/test-expected-mi.R, all Tier 1
arithmetic. Verified against a build of cpp-search at cd991cb, i.e. this
branch's parent:

assertion pre-fix
keys distinct across 60, 61, 65596, 65597, 131133 FAIL — 3 duplicates
.ExpectedMI() correct after a colliding key is cached FAIL — 0.015 vs 4.0e-07
the ni / nj sort invariances hold passes pre-fix; it documents why sorting is allowed, and is not a regression test

Local: test-expected-mi.R and test-Concordance.R 108/108 pass (2 CRAN
skips). spelling::spell_check_package(vignettes = TRUE) clean.

mi_key() narrowed each block size to uint16_t, so partitions whose block
sizes differed by a multiple of 65536 shared a .ExpectedMICache entry and
the second was served the first one's expected mutual information.

uint32_t spans the whole of int, so the encoding is now injective and the
collision is impossible rather than unlikely. The two sort invariances the
key relies on hold: expected_mi() agrees to 2.8e-13 when the blocks of ni
are swapped and to 1.5e-15 when nj is permuted, over 2000 random partitions.

Fixes #132

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Doubling the hex width made mi_key() measurably slower in isolation: at
2e6 calls per arm, appending to a reserved string costs 250 ns against
the 16-bit original's 215 ns at three blocks, rising to 318 vs 239 at
eight. Sizing the string once and writing through a pointer drops the
per-character capacity check, and is 1-4% faster than the 16-bit
original at every block count from 2 to 16 despite emitting twice the
characters.

The emitted key is unchanged; a new assertion pins it to fixed-width hex
of the sorted values, using 0x12345678 so a mis-shifted nibble fails.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ms609

ms609 commented Aug 7, 2026

Copy link
Copy Markdown
Author

Performance of the wider key

Review raised that the hand-unrolled hex emit looked profile-tuned, and that
an end-to-end ClusteringConcordance() A/B might be too coarse to see a
regression in it. Both points were right, so here are the isolated numbers.

Timed in C++ (std::chrono, 2e6 calls per arm, median of 7–11 runs, same
translation unit and flags), so the ~1.4 µs .Call boundary does not mask the
difference:

nj blocks 16-bit, unrolled (original) 32-bit, shift loop 32-bit, unrolled 32-bit, sized once
2 212.6 ns 244.7 234.6 210.6
3 216.8 248.3 237.9 211.6
4 223.8 264.3 253.8 219.4
6 236.1 300.3 283.3 233.0
8 246.2 317.7 298.3 235.3
16 307.9 469.7 412.9 294.9

The shift loop first pushed was 15–53% slower than the original. Unrolling it
recovered part of that but not all. Sizing the string once and writing through
a pointer — dropping the per-character capacity check that += performs even
on a reserved string — is 1–4.4% faster than the 16-bit original at every
block count, despite emitting twice as many characters. That is what is now on
the branch.

The emitted key is byte-identical across all three 32-bit variants; a new
assertion pins it to fixed-width hex of the sorted values, using 0x12345678
so that a mis-shifted nibble fails rather than merely staying injective.

Why this was invisible end-to-end

mi_key() is roughly 10% of a cached .ExpectedMI() call, which is in turn
8–12% of ClusteringConcordance() — so about 1% of the workflow. The
53%-worst regression above would have moved end-to-end wall by ~0.5%, below
the 10 ms clock granularity. The e2e A/B (88 tips: 0.31 s both arms; 400 tips:
warm medians 1.93/1.77 vs 1.84/1.78, sign split 1–1) was not wrong, just
unable to resolve it.

There is no hotter caller to measure instead: nothing in src/ calls
expected_mi, and ClusteringConcordance() is reached only from
ConcordanceTable(), PaintCharacters() and the Shiny consensus module, each
of which calls it once before plotting.

@ms609
ms609 enabled auto-merge August 7, 2026 13:45
@ms609
ms609 disabled auto-merge August 7, 2026 13:46
@ms609
ms609 enabled auto-merge August 7, 2026 13:47
@ms609
ms609 merged commit 2aa25c6 into cpp-search Aug 7, 2026
15 checks passed
@ms609
ms609 deleted the feature/mi-key-cache-aliasing branch August 7, 2026 15:56
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.

mi_key() narrows block sizes to uint16_t, so cache keys alias above 65,535

1 participant