Skip to content

Fix catastrophic hash collisions in FreqTable::make_hash and vecHasher - #24

Merged
gvegayon merged 5 commits into
masterfrom
fix/freqtable-hash-collisions
Jul 15, 2026
Merged

gvegayon merged 5 commits into
masterfrom
fix/freqtable-hash-collisions

Conversation

@gvegayon

@gvegayon gvegayon commented Jul 13, 2026 •

Copy link
Copy Markdown
Member

Summary

DEFM::init() appeared to hang for models with many response columns (reported with a 16-column, Markov order-1 panel model built through the defm R package). Profiling showed 99.9% of the time inside a single unordered_map emplace called from FreqTable::add.

Root cause: std::hash of arithmetic types is typically the identity (a bit-cast for doubles), so hashing statistics that are small integers (0.0, 1.0, ...) yields values whose low ~50 bits are all zero. The boost-style combine in FreqTable::make_hash/vecHasher only propagates entropy toward the low bits ~2 bits per element, so for arrays with up to ~20 columns the low 16+ bits of the final hash are identical across all keys. libc++'s unordered_map masks the hash with a power-of-two bucket count — exactly those bits — so every key lands in one bucket (verified: all 65,536 distinct stat vectors of a 16-column support produced a single bucket with a 65,536-long chain). Each insert scans the whole chain, making Support::calc O(4^ncol) instead of O(2^ncol).

Fix: apply a splitmix64 finalizer to each element hash before combining, in both FreqTable::make_hash (freqtable.hpp) and vecHasher (typedefs.hpp — it backs keys2support in Model::add_array, which had the same degeneration). The top-level barry.hpp amalgam is regenerated.

Measurements

Benchmark Before After
Insert all 2^16 stat vectors of one 16-col support into FreqTable 2.37 s 0.015 s
defm::DEFM::init(), 16 cols, 1 unique Markov state 14.4 s 0.05 s
defm::DEFM::init(), 16 cols, 20 unique Markov states >5 min (timed out) ~1.4 s

Collision hardening (third commit)

FreqTable::index keyed rows by the 64-bit hash alone, so two distinct stat vectors hashing to the same value would silently merge into one support row (birthday-bound probability, ~1e-10 for a 65,536-row support, but silent if it ever fired). add now verifies the stored row against the incoming vector on a hash hit — the stats are already in data, so no extra storage — and sends true collisions to a per-hash overflow chain that stays empty otherwise. size() returns the true row count n (with chains, index.size() would undercount); get_index() has no consumers, and the h_precomp path in Support is currently unreachable (hashes_initialized is never set to true), so no caller is affected.

Complexity is unchanged — the verify compare is O(k), which make_hash already costs, and misses (the common case) pay nothing new. Measured overhead ~5% on the worst-case benchmark above. A new unit test (tests/19-freqtable-collisions.cpp) forces collisions through the precomputed-hash argument and checks rows stay separate and clear() resets the chains.

Also in this PR

The second commit regenerates the stale top-level defm.hpp amalgam: running barry-hpp.R showed it was missing the multi-group formula parser changes (#19) already merged in include/barry/models/defm/. Purely mechanical output of the regeneration script — happy to drop it from this PR if you prefer it separate.

Testing

  • make test-portable: all 122 assertions in 21 test cases pass
  • Downstream check with the R stack: patched headers in barryr, rebuilt defm; its full tinytest suite (25 results) passes, and a synthetic 16-outcome panel model (mirroring the report) initializes and fits instead of hanging

🤖 Generated with Claude Code

gvegayon and others added 2 commits July 13, 2026 13:00
std::hash of arithmetic types is typically the identity (a bit-cast for
doubles), so hashing statistics that are small integers (0.0, 1.0, ...)
yields values whose low ~50 bits are all zero. The boost-style combine
only propagates entropy toward the low bits ~2 bits per element, so for
arrays with up to ~20 columns the low 16+ bits of the final hash are
identical across all keys. libc++'s unordered_map masks the hash with a
power-of-two bucket count, which uses exactly those bits: every key
lands in a single bucket and each insert scans the whole chain, making
Support::calc O(4^ncol) instead of O(2^ncol).

In practice this made DEFM::init() appear to hang for models with many
response columns: a 16-column, order-1 model took 14+ seconds per unique
Markov state (all 65,536 stat vectors chained in one bucket); with the
fix it takes ~0.05s. Inserting the 2^16 stat vectors of one support into
FreqTable drops from 2.37s to 0.015s.

The fix applies a splitmix64 finalizer to each element hash before
combining, in both FreqTable::make_hash and vecHasher (the latter backs
keys2support in Model::add_array, which had the same degeneration).

All 108 assertions in 20 test cases pass (make test-portable).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Running barry-hpp.R to propagate the hash fix showed defm.hpp was out of
date with include/barry/models/defm/: it was missing the multi-group
formula parser changes (#19) already merged in the source headers. This
commit is purely the mechanical regeneration output.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.04%. Comparing base (57b12fe) to head (fdaaf1a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   88.88%   89.04%   +0.16%     
==========================================
  Files          46       46              
  Lines        3166     3204      +38     
==========================================
+ Hits         2814     2853      +39     
+ Misses        352      351       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

FreqTable::index maps make_hash(x) to the row position, keying rows by
the hash value alone: two distinct stat vectors hashing to the same
64-bit value would silently merge into one support row with summed
weights. The probability is birthday-bound (~1e-10 for the 65,536 rows
of a 16-column support), but the failure would be silent.

Now a hash hit verifies the stored row against the incoming vector
(the stats are already in -data-, so no extra storage). On a mismatch
- a true collision - the row goes to a per-hash overflow chain that
stays empty otherwise. size() returns the true row count n instead of
index.size(), which would undercount chained rows; get_index() has no
consumers, and the h_precomp path in Support is currently unreachable
(hashes_initialized is never set to true), so no caller is affected.

Complexity is unchanged: the verify compare is O(k), which make_hash
already costs per add; misses (the common case in support enumeration)
pay nothing new. Measured overhead is ~5% on the worst-case benchmark
(2^16-row supports) and the collision chain is only entered on true
64-bit collisions.

Adds a unit test that forces collisions through the precomputed-hash
argument and checks rows stay separate, weights accumulate per distinct
vector, and clear() resets the chains. All 122 assertions in 21 test
cases pass; downstream defm tinytest suite (25 results) passes.

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

This comment was marked as resolved.

Portability (Copilot review on #24): the splitmix64 finalizer used
64-bit constants and shifts on a size_t accumulator, which silently
changes the avalanche (and warns) where size_t is 32-bit. The mixing
now runs in an explicit std::uint64_t and casts back to size_t at the
end, in both FreqTable::make_hash and vecHasher. On 64-bit targets the
produced hashes are byte-identical to before (<cstdint> already comes
in via barry.hpp).

Robustness: make_hash and vecHasher::operator() read element [0]
unconditionally, which is UB for an empty vector (a valid key type for
MapVec_type). Both now return a fixed value for empty input; a new
assertion covers make_hash on an empty vector.

Also fixes a "lenght" -> "length" typo in the FreqTable length_error.

make test-portable: 123 assertions in 21 test cases pass. Downstream
defm rebuilt against these headers passes its tinytest suite (25).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gvegayon

Copy link
Copy Markdown
Member Author

Thanks @copilot — addressed all five points in fdaaf1a:

  1. 32-bit portability (both mix64 sites). The finalizer now operates on an explicit std::uint64_t and casts back to size_t only at the end, so the avalanche is well-defined and identical regardless of whether size_t is 32- or 64-bit. On 64-bit targets the produced hashes are byte-identical to the previous commit (verified: benchmark and all existing assertions unchanged). <cstdint> is already pulled in via barry.hpp.

  2. Empty-vector UB in make_hash and vecHasher::operator(). Both now return a fixed value for empty input instead of reading element [0]. Added an assertion exercising make_hash on an empty vector.

  3. lenght → length typo fixed in the FreqTable length_error message.

make test-portable now passes 123 assertions in 21 test cases, and defm rebuilt against these headers still passes its full tinytest suite (25 results).

gvegayon added a commit to USCbiostats/barryr that referenced this pull request Jul 15, 2026
…ards

Follows USCbiostats/barry#24 review: the splitmix64 finalizer now mixes
in an explicit uint64_t (identical avalanche on 32- and 64-bit targets),
make_hash/vecHasher guard against empty input, and a length_error typo
is fixed. Keeps inst/include/barry byte-identical to barry master.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gvegayon
gvegayon merged commit de83fec into master Jul 15, 2026
9 of 13 checks passed
@gvegayon
gvegayon deleted the fix/freqtable-hash-collisions branch July 15, 2026 18:34
gvegayon added a commit to USCbiostats/barryr that referenced this pull request Jul 15, 2026
#8)

* Sync headers from barry 0.2.2: fix hash collisions in FreqTable/vecHasher

Syncs inst/include/barry/ with USCbiostats/barry#24, which fixes a
catastrophic hash-collision bug: std::hash of small-integer doubles has
no entropy in its low bits, and the boost-style combine never propagated
any down, so unordered_map (which masks the hash with a power-of-two
bucket count) placed all keys in a single bucket. Support computation
degenerated to O(4^ncol) and models with many response columns (e.g.,
the 16-outcome DEFM that surfaced this) appeared to hang in init.

Both hash sites now apply a splitmix64 finalizer per element, and
FreqTable::add() verifies the stored row on a hash hit, chaining true
64-bit collisions instead of silently merging distinct statistics.

Downstream check: defm rebuilt against these headers passes its full
tinytest suite (25 results); a 16-column model init drops from >5
minutes (never observed to finish on real data) to ~1.4s.

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

* Sync review fixes from barry #24: 32-bit-safe mixing, empty-vector guards

Follows USCbiostats/barry#24 review: the splitmix64 finalizer now mixes
in an explicit uint64_t (identical avalanche on 32- and 64-bit targets),
make_hash/vecHasher guard against empty input, and a length_error typo
is fixed. Keeps inst/include/barry byte-identical to barry master.

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
gvegayon added a commit to UofUEpiBio/defm that referenced this pull request Jul 15, 2026
…uptible (#8)

* Require barry >= 0.2.2 and make long computations interruptible

barry 0.2.2 (USCbiostats/barry#24) fixes a catastrophic hash-collision
bug that made init_defm() appear to hang for models with many outcome
columns: all support statistics landed in a single unordered_map bucket,
degrading support enumeration to O(4^ncol). A 16-outcome, order-1 model
that previously never finished initializing now takes seconds. The
LinkingTo requirement ensures defm is compiled against fixed headers.

This also defines BARRY_USER_INTERRUPT as Rcpp::checkUserInterrupt() in
every translation unit that includes barry, enabling barry's built-in
interrupt checkpoint (every 1000 support iterations). Long-running calls
such as init_defm() can now be stopped with Ctrl-C from an interactive
session instead of requiring a force-kill. Verified via setTimeLimit(),
which uses the same R_CheckUserInterrupt checkpoint: a ~20s init aborts
at the 3s limit with a clean R error and no crash. A model interrupted
mid-initialization should be rebuilt before further use (same caveat as
any error thrown during init).

All 25 tinytest results pass against barry 0.2.2.

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

* Adding the remotes field in the description (will remove once the package lands on CRAN)

* ci: install barry 0.2.2 from barryr PR branch until it reaches CRAN

CI could not resolve `barry (>= 0.2.2)`: CRAN only has 0.2.1, and pak's
`deps::.` (used by setup-r-dependencies) does not consume the DESCRIPTION
`Remotes:` field when a satisfying-named package exists on CRAN, so the
GitHub source for the newer barry was never considered.

Add the barryr PR branch as an explicit `extra-packages` entry in every
workflow that installs dependencies. The action passes extra-packages
straight to `pak::lockfile_create()`, which does honor an explicit
remote spec; verified locally with pak 0.10.0 in a clean library that
the graph now solves with barry -> 0.2.2 from the branch.

All three entries are temporary and must be removed once barry 0.2.2 is
on CRAN (tracked via USCbiostats/barryr#8); each is commented as such.

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

* ci: point barry remote at barryr main now that #8 is merged

USCbiostats/barryr#8 is merged, so barryr's main branch is barry 0.2.2.
Drop the PR-branch ref from the extra-packages remotes; plain
USCbiostats/barryr now resolves to 0.2.2 and won't break when the branch
is deleted. Verified with pak 0.10.0 in a clean library: the graph
solves with barry -> 0.2.2 from barryr main (commit fdd41d5).

Still temporary: remove these extra-packages entries entirely once
barry 0.2.2 reaches CRAN.

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

2 participants