[Store] Add shared thread-local random helpers#2954
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes random number generation by introducing a shared thread-local random utility in include/random.h and refactoring various allocation strategies, services, and utilities to use it. Feedback on the changes points out that std::uniform_int_distribution is only defined for standard integer types (excluding smaller types like int8_t or char), which can lead to undefined behavior or compilation failures. A suggestion is provided to promote smaller integral types to int or unsigned int within randomUniform to ensure safety across all platforms.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| template <typename Integer, typename Generator> | ||
| Integer randomUniform(Integer lower_bound, Integer upper_bound, | ||
| Generator& generator) { | ||
| static_assert(std::is_integral_v<Integer>, | ||
| "randomUniform requires an integral result type"); | ||
| if (lower_bound > upper_bound) { | ||
| throw std::invalid_argument( | ||
| "randomUniform lower bound must not exceed upper bound"); | ||
| } | ||
| std::uniform_int_distribution<Integer> distribution(lower_bound, | ||
| upper_bound); | ||
| return distribution(generator); | ||
| } |
There was a problem hiding this comment.
The C++ standard specifies that std::uniform_int_distribution is only defined for short, int, long, long long, unsigned short, unsigned int, unsigned long, and unsigned long long. Instantiating it with other integral types (such as char, int8_t, uint8_t, or bool) results in undefined behavior and can cause compilation failures on some standard library implementations (e.g., libc++ on macOS).
To prevent this, we can use std::conditional_t to promote types smaller than int to int or unsigned int for the distribution, and then cast the result back to the requested Integer type.
template <typename Integer, typename Generator>
Integer randomUniform(Integer lower_bound, Integer upper_bound,
Generator& generator) {
static_assert(std::is_integral_v<Integer>,
"randomUniform requires an integral result type");
if (lower_bound > upper_bound) {
throw std::invalid_argument(
"randomUniform lower bound must not exceed upper bound");
}
using DistType = std::conditional_t<
sizeof(Integer) < sizeof(int),
std::conditional_t<std::is_signed_v<Integer>, int, unsigned int>,
Integer>;
std::uniform_int_distribution<DistType> distribution(
static_cast<DistType>(lower_bound),
static_cast<DistType>(upper_bound));
return static_cast<Integer>(distribution(generator));
}
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
he-yufeng
left a comment
he-yufeng
left a comment
There was a problem hiding this comment.
Nice consolidation — reads well and the correctness-sensitive parts are right. A few things I checked:
- The sampling is unbiased because it goes through
std::uniform_int_distributionrather thanengine() % n, andrandomIndex/randomUniformreject invalid bounds (upper_bound == 0,lower > upper) instead of silently wrapping. That also quietly fixes the old call sites:uniform_int_distribution<size_t>(0, names.size() - 1)on an emptynameswould have underflowed toSIZE_MAXand indexed out of bounds;randomIndex(names.size())throws instead (and the callers already guard non-empty upstream, so it isn't reachable in practice). - Seeding is an improvement over what it replaces: the per-thread engine is a
mt19937_64seeded from aseed_seqof eightrandom_devicedraws, versus the oldstd::mt19937 generator(std::random_device{}())that fed a single 32-bit value into the engine's state. More entropy into the seed is the right call for a 64-bit engine. - Thread-local storage keeps it race-free by construction, the explicit-engine overloads make the samplers deterministically testable, and the
AGENTS.mdnote plus thecachelib_memory_allocatorcarve-out set the convention clearly.
One forward-looking nit, not a blocker: randomUniform's static_assert(std::is_integral_v<Integer>) is looser than what std::uniform_int_distribution actually permits — the standard only allows short/int/long/long long and their unsigned forms, so instantiating with char, signed/unsigned char, bool, or int8_t compiles but is UB. Every current caller uses int, so nothing is wrong today, but since random.h is now the sanctioned shared entry point, it'd be worth tightening the assert to the permitted set (or documenting the constraint) so a future caller following the AGENTS.md rule can't reach for a narrow type and land in UB.
Approving — correctness, the seeding upgrade, and the test coverage all look good.
3 participants
Description
Add a shared thread-local random facility for Mooncake Store and migrate Store-owned production call sites to use it.
std::mt19937_64engine per thread.randomIndex()andrandomUniform()helpers, including explicit-engine overloads for deterministic tests.AutoPortBinder.AGENTS.mdguidance so future Store code extends the shared API instead of creating local random sources.src/cachelib_memory_allocator/unchanged.No matching open issue or pull request was found.
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
Test results:
The six focused random tests passed, the allocation strategy suite passed, and
mooncake_masterbuilt successfully.Checklist
pre-commit run --all-filesand all hooks passThe repository-wide format hook rewrites unrelated baseline files; those rewrites were reverted. All remaining pre-commit hooks passed on the touched files.
AI Assistance Disclosure
Codex implemented the shared random facility, migrated call sites, added tests, and ran the validation commands. Before marking this PR ready, the human submitter must review every changed line and be able to defend the change end-to-end.