From f0b71c75a8b2cd9b811cff9ac2dac99d1191306b Mon Sep 17 00:00:00 2001 From: Glenn Hickey Date: Mon, 24 Aug 2026 18:05:56 -0400 Subject: [PATCH 1/2] Iterate hashes in insertion order stHash and stSet iteration walked the bucket array, so the order entries came back in was a function of the numeric value of the keys. Most hashes in cactus are keyed on pointers, which makes that order a function of the heap layout: it shifts between runs of the same program on the same input, and everything downstream of the iteration shifts with it. Chain every entry into a doubly-linked list in insertion order and iterate that instead. Insertion order is under the caller's control, so a hash built from a deterministic sequence of inserts now yields its entries the same way on every run. Costs 16 bytes per live entry (32 -> 48 on x86-64). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017E9w7KrbFEfqNRtkam4u1F --- C/impl/hashTableC.c | 4 ++ C/impl/hashTableC_itr.c | 82 ++++++++++----------------------------- C/inc/hashTablePrivateC.h | 39 +++++++++++++++++++ 3 files changed, 63 insertions(+), 62 deletions(-) diff --git a/C/impl/hashTableC.c b/C/impl/hashTableC.c index 0755547..02d98fa 100644 --- a/C/impl/hashTableC.c +++ b/C/impl/hashTableC.c @@ -62,6 +62,8 @@ create_hashtable(uint64_t minsize, uint64_t(*hashf)(const void*), int(*eqf)( //my additions h->keyFree = keyFree; h->valueFree = valueFree; + h->insertFirst = NULL; + h->insertLast = NULL; return h; } @@ -164,6 +166,7 @@ int64_t hashtable_insert(struct hashtable *h, void *k, void *v) { e->v = v; e->next = h->table[index]; h->table[index] = e; + hashtable_insertListAppend(h, e); return -1; } @@ -203,6 +206,7 @@ hashtable_remove(struct hashtable *h, void *k, int64_t freeKey) { /* Check hash value to short circuit heavier comparison */ if ((hashvalue == e->h) && (h->eqfn(k, e->k))) { *pE = e->next; + hashtable_insertListRemove(h, e); h->entrycount--; v = e->v; if (freeKey) { diff --git a/C/impl/hashTableC_itr.c b/C/impl/hashTableC_itr.c index e3c5d2c..b195c0c 100644 --- a/C/impl/hashTableC_itr.c +++ b/C/impl/hashTableC_itr.c @@ -16,29 +16,18 @@ indexFor(uint64_t tablelength, uint64_t hashvalue) { /*****************************************************************************/ /* hashtable_iterator - iterator constructor */ +/* Walks the insertion-order list, not the buckets, so that the order does not + * depend on the numeric value of the keys. See hashTablePrivateC.h. */ struct hashtable_itr * hashtable_iterator(struct hashtable *h) { - unsigned int i, tablelength; struct hashtable_itr *itr = (struct hashtable_itr *) st_malloc(sizeof(struct hashtable_itr)); if (NULL == itr) return NULL; itr->h = h; - itr->e = NULL; + itr->e = h->insertFirst; itr->parent = NULL; - tablelength = h->tablelength; - itr->index = tablelength; - if (0 == h->entrycount) return itr; - - for (i = 0; i < tablelength; i++) - { - if (NULL != h->table[i]) - { - itr->e = h->table[i]; - itr->index = i; - break; - } - } + itr->index = 0; return itr; } @@ -49,38 +38,9 @@ hashtable_iterator(struct hashtable *h) int hashtable_iterator_advance(struct hashtable_itr *itr) { - unsigned int j,tablelength; - struct entry **table; - struct entry *next; if (NULL == itr->e) return 0; /* stupidity check */ - - next = itr->e->next; - if (NULL != next) - { - itr->parent = itr->e; - itr->e = next; - return -1; - } - tablelength = itr->h->tablelength; - itr->parent = NULL; - if (tablelength <= (j = ++(itr->index))) - { - itr->e = NULL; - return 0; - } - table = itr->h->table; - while (NULL == (next = table[j])) - { - if (++j >= tablelength) - { - itr->index = tablelength; - itr->e = NULL; - return 0; - } - } - itr->index = j; - itr->e = next; - return -1; + itr->e = itr->e->insertNext; + return (NULL == itr->e) ? 0 : -1; } /*****************************************************************************/ @@ -94,27 +54,25 @@ hashtable_iterator_advance(struct hashtable_itr *itr) int hashtable_iterator_remove(struct hashtable_itr *itr) { - struct entry *remember_e, *remember_parent; + struct entry *remember_e, **pE; int ret; - /* Do the removal */ - if (NULL == (itr->parent)) - { - /* element is head of a chain */ - itr->h->table[itr->index] = itr->e->next; - } else { - /* element is mid-chain */ - itr->parent->next = itr->e->next; - } - /* itr->e is now outside the hashtable */ remember_e = itr->e; - itr->h->entrycount--; - freekey(remember_e->k); - /* Advance the iterator, correcting the parent */ - remember_parent = itr->parent; + /* Unlink from the bucket chain. The iterator walks the insertion list, so + * the chain predecessor has to be looked up rather than remembered. */ + pE = &(itr->h->table[indexFor(itr->h->tablelength, remember_e->h)]); + while (*pE != remember_e) { + pE = &((*pE)->next); + } + *pE = remember_e->next; + + /* Advance before unlinking, so the successor is still reachable */ ret = hashtable_iterator_advance(itr); - if (itr->parent == remember_e) { itr->parent = remember_parent; } + + hashtable_insertListRemove(itr->h, remember_e); + itr->h->entrycount--; + freekey(remember_e->k); free(remember_e); return ret; } diff --git a/C/inc/hashTablePrivateC.h b/C/inc/hashTablePrivateC.h index cce7597..1866356 100644 --- a/C/inc/hashTablePrivateC.h +++ b/C/inc/hashTablePrivateC.h @@ -3,6 +3,7 @@ #ifndef __HASHTABLE_PRIVATE_CWC22_H__ #define __HASHTABLE_PRIVATE_CWC22_H__ +#include #include "hashTableC.h" #ifdef __cplusplus @@ -11,11 +12,22 @@ extern "C" { /*****************************************************************************/ +/* + * Entries are on two lists: "next" chains the entries that share a bucket, + * while insertNext/insertPrev chain every entry in the table in the order it + * was inserted. Iteration walks the insertion list rather than the buckets, + * so that the order a hash yields its entries does not depend on the numeric + * value of the keys. That matters because most hashes here are keyed on + * pointers: walking the buckets hands entries back in an order that shifts + * with the heap layout, which makes the output of anything downstream of the + * iteration differ from run to run. + */ struct entry { void *k, *v; uint64_t h; struct entry *next; + struct entry *insertNext, *insertPrev; }; struct hashtable { @@ -28,8 +40,35 @@ struct hashtable { int (*eqfn) (const void *k1, const void *k2); void (*keyFree)(void *); void (*valueFree)(void *); + struct entry *insertFirst, *insertLast; /* insertion-order list, for iteration */ }; +/*****************************************************************************/ +/* Insertion-order list maintenance, used by hashTableC.c and hashTableC_itr.c */ +static inline void hashtable_insertListAppend(struct hashtable *h, struct entry *e) { + e->insertNext = NULL; + e->insertPrev = h->insertLast; + if (h->insertLast != NULL) { + h->insertLast->insertNext = e; + } else { + h->insertFirst = e; + } + h->insertLast = e; +} + +static inline void hashtable_insertListRemove(struct hashtable *h, struct entry *e) { + if (e->insertPrev != NULL) { + e->insertPrev->insertNext = e->insertNext; + } else { + h->insertFirst = e->insertNext; + } + if (e->insertNext != NULL) { + e->insertNext->insertPrev = e->insertPrev; + } else { + h->insertLast = e->insertPrev; + } +} + /*****************************************************************************/ uint64_t hashP(struct hashtable *h, void *k); From cbb2285a54f4091af984a5cf888606c3f6dd4e37 Mon Sep 17 00:00:00 2001 From: Glenn Hickey Date: Mon, 24 Aug 2026 18:06:06 -0400 Subject: [PATCH 2/2] Give the random number generator per-thread state st_random drew from rand(), one sequence shared by the whole process. Two threads drawing from it interleave differently on every run, so anything built from those draws -- cactus's reference ordering and its ancestral base calls, both of which break ties randomly inside an OpenMP loop -- came out different each time. Replace it with splitmix64 in thread-local state. st_randomSeed now seeds the calling thread, which lets a parallel loop seed each iteration from the work item and get the same draws however the threads are scheduled. st_random keeps its old 31 bits of resolution: callers were written against values of that granularity, and at least one test depends on them being exactly representable. Unseeded single-threaded use is still reproducible, as it was before (rand() was never seeded from the clock), though the sequence itself is different. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017E9w7KrbFEfqNRtkam4u1F --- C/impl/sonLibRandom.c | 35 ++++++++++++++++++++++++++++++++--- C/inc/sonLibRandom.h | 5 +++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/C/impl/sonLibRandom.c b/C/impl/sonLibRandom.c index f835b3e..6a501d9 100644 --- a/C/impl/sonLibRandom.c +++ b/C/impl/sonLibRandom.c @@ -8,8 +8,36 @@ const char *RANDOM_EXCEPTION_ID = "RANDOM_EXCEPTION"; +/* + * The generator state is per-thread rather than per-process. rand() draws from + * one shared sequence, so threads racing for it get a different interleaving on + * every run, and anything built from those draws differs run to run. With the + * state thread-local, a parallel loop gets reproducible draws by seeding each + * iteration itself, from the work item rather than from the thread -- see the + * st_randomSeed calls in cactus's parallel loops. + * + * Every thread starts from the same default seed, so a single-threaded program + * that never calls st_randomSeed sees the same sequence it always did (a fixed + * one: rand() was never seeded from the clock either). + */ +#if defined(__GNUC__) || defined(__clang__) +#define ST_THREAD_LOCAL __thread +#else +#define ST_THREAD_LOCAL +#endif + +static ST_THREAD_LOCAL uint64_t st_randomState = 1; + +/* splitmix64 - small, fast, and passes the usual statistical batteries */ +static uint64_t st_randomNext(void) { + uint64_t z = (st_randomState += UINT64_C(0x9E3779B97F4A7C15)); + z = (z ^ (z >> 30)) * UINT64_C(0xBF58476D1CE4E5B9); + z = (z ^ (z >> 27)) * UINT64_C(0x94D049BB133111EB); + return z ^ (z >> 31); +} + void st_randomSeed(int64_t seed) { - srand(seed); + st_randomState = (uint64_t) seed; } int64_t st_randomInt64(int64_t min, int64_t max) { @@ -37,8 +65,9 @@ int64_t st_randomInt(int64_t min, int64_t max) { } double st_random(void) { - static const double i = RAND_MAX+1.0; - double d = rand()/i; + // 31 bits, the resolution this returned when it was rand()/(RAND_MAX+1.0). + // Callers have been written against values of that granularity, so keep it. + double d = (double) (st_randomNext() >> 33) / 2147483648.0; // 2^31 return d >= 1.0 ? 0.9999 : (d < 0.0 ? 0.0 : d); } diff --git a/C/inc/sonLibRandom.h b/C/inc/sonLibRandom.h index 01e8484..a26a4f6 100644 --- a/C/inc/sonLibRandom.h +++ b/C/inc/sonLibRandom.h @@ -27,6 +27,11 @@ extern const char *RANDOM_EXCEPTION_ID; /* * Seed the random number generator. + * + * The generator state is thread-local, so this seeds the calling thread only. + * Code that draws random numbers from a parallel loop and wants a reproducible + * result should seed at the top of each iteration with a value derived from the + * work item (not from the thread id, which is not stable across runs). */ void st_randomSeed(int64_t seed);