From 3d13df6b482a59be3bdced70913aaee46dcb5e9e Mon Sep 17 00:00:00 2001 From: Ciaran Ryan-Anderson Date: Mon, 28 Jul 2025 23:23:06 -0600 Subject: [PATCH] Make PCG thread safe --- Cargo.lock | 14 -- crates/pecos-clib-pcg/Cargo.toml | 1 - crates/pecos-clib-pcg/build.rs | 68 ++-------- crates/pecos-clib-pcg/c_src/rng_pcg.c | 92 +++++++++++++ crates/pecos-clib-pcg/c_src/rng_pcg.h | 51 ++++++++ crates/pecos-clib-pcg/src/lib.rs | 70 ++++++++-- crates/pecos-clib-pcg/tests/pcg_tests.rs | 158 +++++++++++++++++++++++ 7 files changed, 376 insertions(+), 78 deletions(-) create mode 100644 crates/pecos-clib-pcg/c_src/rng_pcg.c create mode 100644 crates/pecos-clib-pcg/c_src/rng_pcg.h diff --git a/Cargo.lock b/Cargo.lock index c111a59dc..8c66d65d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1796,7 +1796,6 @@ name = "pecos-clib-pcg" version = "0.1.1" dependencies = [ "cc", - "ureq", ] [[package]] @@ -2993,19 +2992,6 @@ version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7264e107f553ccae879d21fbea1d6724ac785e8c3bfc762137959b5802826ef3" -[[package]] -name = "ureq" -version = "2.12.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "02d1a66277ed75f640d608235660df48c8e3c19f3b4edb6a263315626cc3c01d" -dependencies = [ - "base64", - "log", - "native-tls", - "once_cell", - "url", -] - [[package]] name = "url" version = "2.5.4" diff --git a/crates/pecos-clib-pcg/Cargo.toml b/crates/pecos-clib-pcg/Cargo.toml index 259fd8bbc..efa86a6b1 100644 --- a/crates/pecos-clib-pcg/Cargo.toml +++ b/crates/pecos-clib-pcg/Cargo.toml @@ -15,7 +15,6 @@ readme = "README.md" [build-dependencies] cc.workspace = true -ureq = { version = "2.9", default-features = false, features = ["native-tls"] } [lints] workspace = true diff --git a/crates/pecos-clib-pcg/build.rs b/crates/pecos-clib-pcg/build.rs index 7bf049bd1..00903a527 100644 --- a/crates/pecos-clib-pcg/build.rs +++ b/crates/pecos-clib-pcg/build.rs @@ -1,63 +1,23 @@ use cc::Build; use std::env; -use std::fs; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; -// TODO: Should probably just vendor the C code into the Rust crate... fn main() { - let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap()); let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); - - // Try local path first (for development) - let local_clib_path = manifest_dir - .parent() - .unwrap() - .parent() - .unwrap() - .join("clib") - .join("pecos-rng"); - - let src_path = if local_clib_path.exists() { - // Development: use local files - let src = local_clib_path.join("src"); - println!("cargo:rerun-if-changed={}", src.join("rng_pcg.c").display()); - println!("cargo:rerun-if-changed={}", src.join("rng_pcg.h").display()); - src - } else { - // Published crate: download from GitHub - let pcg_dir = out_dir.join("pcg"); - fs::create_dir_all(&pcg_dir).unwrap(); - - let commit = "95a6ddbdf85ad7bcf8b9133aa2552f3f1ae7da84"; - let base_url = format!( - "https://raw.githubusercontent.com/PECOS-packages/PECOS/{commit}/clib/pecos-rng/src" - ); - - // Download files if they don't exist - download_if_needed(&pcg_dir.join("rng_pcg.c"), &format!("{base_url}/rng_pcg.c")); - download_if_needed(&pcg_dir.join("rng_pcg.h"), &format!("{base_url}/rng_pcg.h")); - - pcg_dir - }; + let c_src_dir = manifest_dir.join("c_src"); + + // Build our local C code with thread-safe functions exposed + println!( + "cargo:rerun-if-changed={}", + c_src_dir.join("rng_pcg.c").display() + ); + println!( + "cargo:rerun-if-changed={}", + c_src_dir.join("rng_pcg.h").display() + ); Build::new() - .file(src_path.join("rng_pcg.c")) - .include(&src_path) + .file(c_src_dir.join("rng_pcg.c")) + .include(&c_src_dir) .compile("pecos_pcg"); } - -fn download_if_needed(path: &Path, url: &str) { - if !path.exists() { - println!("cargo:warning=Downloading {} to {}", url, path.display()); - - let response = ureq::get(url) - .call() - .unwrap_or_else(|e| panic!("Failed to download {url}: {e}")); - - let mut file = fs::File::create(path) - .unwrap_or_else(|e| panic!("Failed to create {}: {}", path.display(), e)); - - std::io::copy(&mut response.into_reader(), &mut file) - .unwrap_or_else(|e| panic!("Failed to write {}: {}", path.display(), e)); - } -} diff --git a/crates/pecos-clib-pcg/c_src/rng_pcg.c b/crates/pecos-clib-pcg/c_src/rng_pcg.c new file mode 100644 index 000000000..23f2385c8 --- /dev/null +++ b/crates/pecos-clib-pcg/c_src/rng_pcg.c @@ -0,0 +1,92 @@ +/* + * PCG Random Number Generation for C. + * + * Copyright 2014 Melissa O'Neill + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * For additional information about the PCG random number generation scheme, + * including its license and other licensing options, visit + * + * http://www.pcg-random.org + */ + +#include +#include "rng_pcg.h" + +// RNG state structure is now defined in the header + +// global RNG state +static pcg32_random_t pcg32_global = { + 0x853c49e6748fea9bULL, + 0xda3e39cb94b95bdbULL +}; + +// default multi[plier] +#define PCG_DEFAULT_MULTIPLIER_64 6364136223846793005ULL + +// helper functions +static inline uint32_t pcg_rotr_32(uint32_t value, unsigned int urot) { + int rot = (int)urot; + return (value >> rot) | (value << ((-rot) & 31)); +} + +static inline void pcg_setseq_64_step_r(pcg32_random_t* rng) { + rng->state = rng->state * PCG_DEFAULT_MULTIPLIER_64 + rng->inc; +} + +static inline uint32_t pcg_output_xsh_rr_64_32(uint64_t state) { + return pcg_rotr_32(((state >> 18u) ^ state) >> 27u, state >> 59u); +} + +uint32_t pcg32_random_r(pcg32_random_t* rng) { + const uint64_t oldstate = rng->state; + pcg_setseq_64_step_r(rng); + return pcg_output_xsh_rr_64_32(oldstate); +} + +uint32_t pcg32_boundedrand_r(pcg32_random_t* rng, uint32_t ubound) { + int32_t bound = (int32_t)ubound; + uint32_t threshold = -bound % bound; + for (;;) { + const uint32_t r = pcg32_random_r(rng); + if (r >= threshold) + return r % bound; + } +} + +void pcg32_srandom_r(pcg32_random_t* rng, uint64_t initstate, uint64_t initseq) { + rng->state = 0U; + rng->inc = (initseq << 1u) | 1u; + pcg_setseq_64_step_r(rng); + rng->state += initstate; + pcg_setseq_64_step_r(rng); +} + +// public interface to RNG + +uint32_t pcg32_random() { + return pcg32_random_r(&pcg32_global); +} + +uint32_t pcg32_boundedrand(uint32_t bound) { + return pcg32_boundedrand_r(&pcg32_global, bound); +} + +double pcg32_frandom() { + return ldexp(pcg32_random(), -32); +} + +void pcg32_srandom(uint64_t seq) { + pcg32_srandom_r(&pcg32_global, 42u, seq); +} diff --git a/crates/pecos-clib-pcg/c_src/rng_pcg.h b/crates/pecos-clib-pcg/c_src/rng_pcg.h new file mode 100644 index 000000000..718c52703 --- /dev/null +++ b/crates/pecos-clib-pcg/c_src/rng_pcg.h @@ -0,0 +1,51 @@ +/* + * PCG Random Number Generation for C. + * + * Copyright 2014 Melissa O'Neill + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * For additional information about the PCG random number generation scheme, + * including its license and other licensing options, visit + * + * http://www.pcg-random.org + */ + +#pragma once + +#include + +#if __cplusplus +extern "C" { +#endif + +// Thread-safe RNG state structure +typedef struct pcg_state_setseq_64 { + uint64_t state; + uint64_t inc; +} pcg32_random_t; + +// Thread-safe versions that take explicit state +uint32_t pcg32_random_r(pcg32_random_t* rng); +uint32_t pcg32_boundedrand_r(pcg32_random_t* rng, uint32_t bound); +void pcg32_srandom_r(pcg32_random_t* rng, uint64_t initstate, uint64_t initseq); + +// Global state versions (for compatibility) +uint32_t pcg32_random(); +uint32_t pcg32_boundedrand(uint32_t bound); +double pcg32_frandom(); +void pcg32_srandom(uint64_t seq); + +#if __cplusplus +} +#endif diff --git a/crates/pecos-clib-pcg/src/lib.rs b/crates/pecos-clib-pcg/src/lib.rs index f44696de9..fadbc8c6c 100644 --- a/crates/pecos-clib-pcg/src/lib.rs +++ b/crates/pecos-clib-pcg/src/lib.rs @@ -1,29 +1,81 @@ -// FFI bindings to the C PCG library +use std::cell::RefCell; + +// Mirror the C struct pcg32_random_t +#[repr(C)] +struct PcgState { + state: u64, + inc: u64, +} + +// FFI bindings to the C PCG library - now with thread-safe versions! unsafe extern "C" { - fn pcg32_random() -> u32; - fn pcg32_boundedrand(bound: u32) -> u32; - fn pcg32_frandom() -> f64; - fn pcg32_srandom(seq: u64); + fn pcg32_random_r(rng: *mut PcgState) -> u32; + fn pcg32_boundedrand_r(rng: *mut PcgState, bound: u32) -> u32; + fn pcg32_srandom_r(rng: *mut PcgState, initstate: u64, initseq: u64); +} + +// Thread-local state: each thread has its own PCG state +thread_local! { + static THREAD_STATE: RefCell = RefCell::new(ThreadRngState::new()); +} + +struct ThreadRngState { + pcg: PcgState, + seed: u64, +} + +impl ThreadRngState { + fn new() -> Self { + let mut state = Self { + pcg: PcgState { state: 0, inc: 0 }, + seed: 0, + }; + // Initialize with default seed + state.reseed(42); + state + } + + fn reseed(&mut self, seed: u64) { + self.seed = seed; + unsafe { + // For PCG: initstate affects starting position, initseq selects sequence + // Using seed for initseq (sequence selection) and a fixed initstate + // matches the original behavior while allowing different sequences per thread + pcg32_srandom_r(&raw mut self.pcg, 42, seed); + } + } } // Rust wrapper functions with safe interfaces #[must_use] pub fn random() -> u32 { - unsafe { pcg32_random() } + THREAD_STATE.with(|state| { + let mut state = state.borrow_mut(); + unsafe { pcg32_random_r(&raw mut state.pcg) } + }) } #[must_use] pub fn boundedrand(bound: u32) -> u32 { - unsafe { pcg32_boundedrand(bound) } + THREAD_STATE.with(|state| { + let mut state = state.borrow_mut(); + unsafe { pcg32_boundedrand_r(&raw mut state.pcg, bound) } + }) } #[must_use] pub fn frandom() -> f64 { - unsafe { pcg32_frandom() } + // The C code implements this as ldexp(pcg32_random(), -32) + // which is equivalent to dividing by 2^32 + THREAD_STATE.with(|state| { + let mut state = state.borrow_mut(); + let val = unsafe { pcg32_random_r(&raw mut state.pcg) }; + f64::from(val) * 2.0_f64.powi(-32) + }) } pub fn srandom(seq: u64) { - unsafe { pcg32_srandom(seq) } + THREAD_STATE.with(|state| state.borrow_mut().reseed(seq)); } #[cfg(test)] diff --git a/crates/pecos-clib-pcg/tests/pcg_tests.rs b/crates/pecos-clib-pcg/tests/pcg_tests.rs index f687ef5b3..fe255ba85 100644 --- a/crates/pecos-clib-pcg/tests/pcg_tests.rs +++ b/crates/pecos-clib-pcg/tests/pcg_tests.rs @@ -1,4 +1,6 @@ use pecos_clib_pcg::{boundedrand, frandom, random, srandom}; +use std::sync::Arc; +use std::thread; #[test] fn test_pcg_random_generates_values() { @@ -87,3 +89,159 @@ fn test_pcg_deterministic_behavior() { "First value after seeding should be deterministic" ); } + +#[test] +fn test_pcg_shared_state_interference() { + // This test is more likely to fail when run in parallel with other tests + // because they all share the same global RNG state + + const ITERATIONS: usize = 100; + let mut results = Vec::new(); + + for i in 0..ITERATIONS { + srandom(42); + // Add some delay to increase chance of interference + std::thread::yield_now(); + + let val = random(); + results.push(val); + + assert!( + !(i > 0 && results[i] != results[0]), + "Iteration {}: Expected deterministic value {} but got {} (shared state interference detected!)", + i, + results[0], + val + ); + } +} + +#[test] +fn test_pcg_rapid_reseeding() { + // Rapidly reseed and check values to increase chance of race conditions + let expected_values: Vec = (0..10) + .map(|i| { + srandom(i); + random() + }) + .collect(); + + // Now verify multiple times + for round in 0..50 { + for (i, &expected) in expected_values.iter().enumerate() { + srandom(i as u64); + let actual = random(); + assert_eq!( + actual, expected, + "Round {round}, seed {i}: Expected {expected} but got {actual} (state corruption detected!)" + ); + } + } +} + +#[test] +fn test_pcg_concurrent_access() { + // This test verifies that threads maintain independent sequences even when running concurrently + + let num_threads = 10; + let iterations_per_thread = 100; + let barrier = Arc::new(std::sync::Barrier::new(num_threads)); + + // First, generate expected sequences for each thread + let expected_sequences: Vec> = (0..num_threads) + .map(|thread_id| { + srandom(thread_id as u64); + (0..iterations_per_thread).map(|_| random()).collect() + }) + .collect(); + + let handles: Vec<_> = (0..num_threads) + .map(|thread_id| { + let barrier = Arc::clone(&barrier); + let expected_seq = expected_sequences[thread_id].clone(); + + thread::spawn(move || { + // Wait for all threads to be ready + barrier.wait(); + + // Each thread uses its own seed + srandom(thread_id as u64); + + let mut results = Vec::new(); + #[allow(clippy::needless_range_loop)] + for i in 0..iterations_per_thread { + let val = random(); + results.push(val); + + // Verify we're getting the expected value + assert_eq!( + val, expected_seq[i], + "Thread {thread_id} iteration {i}: expected {} but got {val}", + expected_seq[i] + ); + + // Yield to increase chance of interleaving + if i % 10 == 0 { + thread::yield_now(); + } + } + + results + }) + }) + .collect(); + + // Collect all results and verify + for (thread_id, handle) in handles.into_iter().enumerate() { + let results = handle.join().unwrap(); + assert_eq!( + results, expected_sequences[thread_id], + "Thread {thread_id} produced unexpected sequence" + ); + } +} + +#[test] +fn test_pcg_thread_independence() { + // Verify that threads with different seeds maintain independent sequences + + // First, get expected sequences + srandom(100); + let expected_seq1: Vec = (0..5).map(|_| random()).collect(); + + srandom(200); + let expected_seq2: Vec = (0..5).map(|_| random()).collect(); + + // Run threads concurrently + let handle1 = thread::spawn(move || { + srandom(100); + let mut results = vec![]; + for _ in 0..5 { + results.push(random()); + thread::yield_now(); // Encourage interleaving + } + results + }); + + let handle2 = thread::spawn(move || { + srandom(200); + let mut results = vec![]; + for _ in 0..5 { + results.push(random()); + thread::yield_now(); // Encourage interleaving + } + results + }); + + let thread1_results = handle1.join().unwrap(); + let thread2_results = handle2.join().unwrap(); + + assert_eq!( + thread1_results, expected_seq1, + "Thread 1 maintains independent sequence" + ); + assert_eq!( + thread2_results, expected_seq2, + "Thread 2 maintains independent sequence" + ); +}