switch random number generators used by sampling - #110
Merged
Merged
Conversation
leoparente
added a commit
that referenced
this pull request
Jun 23, 2026
…700) Removes the vendored 3rd/rng/jsf.h (178 lines, Bob Jenkins Small Fast) in favor of a ~6-line inline SplitMix64 in AbstractMetricsManager.h. This is the deep- sampling gate's PRNG (roll 0-99, deep-sample when the roll is under deep_sample_rate), used in exactly one place; sampling RATE is unchanged (only which events get deep-sampled differs, and nothing depends on the specific set). Why SplitMix64 over the issue's suggested pcg-cpp conan package: it removes the maintained code (the whole 3rd/rng dir) without ADDING a third-party dependency, keeps an 8-byte state (vs jsf's 16, vs std::mt19937's ~2.5KB), and has BigCrush-grade quality. CRITICAL invariant preserved (issue #109 "RDRAND failure on Arista switch"): the generator is seeded from a fixed constant and never from std::random_device / hardware entropy. The pre-JSF randutils-based RNG auto-seeded via random_device -> RDRAND, which faulted and threw on startup on some hardware; PR #110 fixed that by switching to a fixed-seeded PRNG. A prominent comment documents this so nobody "improves" it into entropy seeding (a particular footgun with pcg-cpp's idiomatic seed_seq_from<random_device>). Build wiring: - Delete 3rd/rng/ and its add_subdirectory + the `rng` link in visor-core. - Fix a latent copy-paste bug in 3rd/timer/CMakeLists.txt: it called target_include_directories(rng ...) instead of (timer ...), so timer.hpp's include dir was being attached to the rng target and only reached visor-core transitively through it. Removing rng would have broken <timer.hpp>; pointing it at timer makes the timer target self-contained. - Update the NOMINMAX comment in CMakeLists.txt (it cited jsf.h's static min()/max()); keep the define for general Windows hygiene. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
leoparente
added a commit
that referenced
this pull request
Jun 24, 2026
…700) (#786) Removes the vendored 3rd/rng/jsf.h (178 lines, Bob Jenkins Small Fast) in favor of a ~6-line inline SplitMix64 in AbstractMetricsManager.h. This is the deep- sampling gate's PRNG (roll 0-99, deep-sample when the roll is under deep_sample_rate), used in exactly one place; sampling RATE is unchanged (only which events get deep-sampled differs, and nothing depends on the specific set). Why SplitMix64 over the issue's suggested pcg-cpp conan package: it removes the maintained code (the whole 3rd/rng dir) without ADDING a third-party dependency, keeps an 8-byte state (vs jsf's 16, vs std::mt19937's ~2.5KB), and has BigCrush-grade quality. CRITICAL invariant preserved (issue #109 "RDRAND failure on Arista switch"): the generator is seeded from a fixed constant and never from std::random_device / hardware entropy. The pre-JSF randutils-based RNG auto-seeded via random_device -> RDRAND, which faulted and threw on startup on some hardware; PR #110 fixed that by switching to a fixed-seeded PRNG. A prominent comment documents this so nobody "improves" it into entropy seeding (a particular footgun with pcg-cpp's idiomatic seed_seq_from<random_device>). Build wiring: - Delete 3rd/rng/ and its add_subdirectory + the `rng` link in visor-core. - Fix a latent copy-paste bug in 3rd/timer/CMakeLists.txt: it called target_include_directories(rng ...) instead of (timer ...), so timer.hpp's include dir was being attached to the rng target and only reached visor-core transitively through it. Removing rng would have broken <timer.hpp>; pointing it at timer makes the timer target self-contained. - Update the NOMINMAX comment in CMakeLists.txt (it cited jsf.h's static min()/max()); keep the define for general Windows hygiene. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.