refactor(sampling): replace std::random_device with the existing xorshift64 - #794
Conversation
…hift64 Sampling code carried two random sources: xorshift64, written out in PoissonSampler together with the rationale for preferring it, and std::mt19937 seeded from std::random_device in three other places. The reasons given for the first apply to all of them. std::random_device and the distribution wrappers may allocate and may throw, and LivenessTracker::track runs on allocation callbacks on arbitrary application threads, where neither is safe. std::random_device may also block or return low-entropy values depending on the host, and which it does is settled by the libstdc++ the library is compiled against rather than by anything in this source. xorshift.h now holds the generator and the draws built on it, and every copy of the recurrence calls it: PoissonSampler, ProfiledThread's fault-injection state, faultInjection's global fallback, and the three converted sites. The Knuth constant references common.h instead of being spelled out a fifth time. LivenessTracker loses the most: two thread-local heap allocations per thread (~2.5 KB of mt19937 among them), four create/free functions, and a subsample decision that is now an integer compare against a threshold computed once in initialize(). Seeding no longer draws on the OS. A stream comes from an identity mixed with a sequence number -- an object address with an epoch, a thread id with a tick -- giving independent streams per thread and per recording with no entropy source. Sampling requires independence rather than unpredictability, so this is a deliberate trade: two processes started on the same tick with ASLR disabled would correlate. ReservoirSampler takes its stream as a constructor argument. Seeding from the instance address alone made every sampler built at one call site replay a single stream, so consecutive recordings sampled identically. The wall-clock interval dither keeps its normal distribution, now drawn by Box-Muller rather than std::normal_distribution. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 4 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 7237b42 | Docs | View more details | Give us feedback! |
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5364e99f9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
The new threshold helper converts NaN to u64. The liveness option parser accepts NaN, so profiler setup has undefined behavior.
🤖 Datadog Autotest · Commit 5364e99 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
CI Test ResultsRun: #35107293522 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 31 | Failed: 0 | Cancelled: 1 Updated: 2026-09-16 17:19:04 UTC |
…sholds Pre-review follow-ups, three of them comments that claimed properties the code does not have. reservoirSampler.h lost its transitive <cassert> when <random> went, so the header no longer compiled standalone; it built only because wallClock.h includes <cassert> first and the gtest gets assert from gtest. MIN_UNIFORM's comment said the floor was not about finiteness. At _size == 1, reachable with wall_threads_per_tick=1, weight equals the draw, and toUnitDouble's smallest draw makes 1 - weight round to exactly 1.0, log(1 - weight) exactly 0.0 and the division -inf, whose cast to int is undefined. The floor is what prevents that, so the comment was inviting a change that would introduce undefined behaviour. It now says so, along with the ~1e-16 engagement probability that makes the case untestable. xorshift::threshold() compared p against 0 in a way NaN passes, leaving an unordered value to reach the cast -- undefined, and a trap under -fsanitize=undefined. A "nan" sampling ratio survives the min/max clamp in arguments.cpp and reaches it. The guard is inverted and covered by a test. The two LivenessTracker comments described the pre-change design: one argued about pthread-key destructors leaking heap allocations that no longer exist (both ThreadLocals now hold plain values and register no destructor), the other promised a per-recording reseed that does not happen, since the slot is cleared only at thread end or JNI detach and not by stop/start. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…e type The ratio and the threshold derived from it were two independent fields kept in step by convention. Nothing stopped a future writer of the ratio from leaving the threshold behind, and the result would have been silent: allocations sampled at the old rate while Recording reports the new ratio into the JFR chunk, with no assertion or counter disagreeing. SubsampleRate holds both, and its only constructor derives the threshold from the ratio, so one cannot be set without the other; initialize() replaces the pair in a single assignment. The invariant is now enforced by construction rather than remembered. Tested against observed behaviour instead of against xorshift::threshold, so the tests still fail if the derivation stops honouring the advertised rate: the keep-rate over 200k draws matches the ratio for four rates, reassignment carries both halves, and the degenerate ratios (1, 0, NaN) all yield defined thresholds. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
zhengyu123
left a comment
There was a problem hiding this comment.
LGTM, just a few nits.
…ariant Review follow-ups, all three of them places the first pass left behind. ProfiledThread had two _fi_rng seeding sites -- claimAcquire and the constructor -- and only the first was converted, so a hand-rolled copy of the address-XOR-Knuth-hashed-tid seed survived in threadLocalData.h. Both now call xorshift::seed, which is what the earlier commit claimed of every copy. faultInjection.cpp kept its own KNUTH constant after xorshift.h started referencing common.h's, leaving two declarations of one value; it now uses xorshift::KNUTH at both of its sites. next() documented requiring a non-zero state without checking it. It now asserts on entry, and again after stepping: the recurrence is a bijection on the non-zero states, so a valid state never reaches the fixed point, and saying so in an assertion documents the property rather than only guarding the input. Release builds compile with -DNDEBUG, so neither check reaches the sampling path. The non-zero rule itself had three implementations -- inside seed(), in setFiRng's `seed ? seed : 1`, and in faultinj::nextRandom's `if (x == 0)`. xorshift::nonZero() is now the only one, and seed() is expressed in terms of it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
#794 removed the std::random_device uses, so libstdc++'s getentropy reference no longer enters the link and the last symbol above 2.17 is gone. With expf already pinned per architecture, the artifact requires nothing newer than the glibc that EL7 ships. Measured on release builds in the AlmaLinux 8 image on both architectures: no references above 2.17 at all, and the floor check passes at 2.17 where it previously named getentropy@GLIBC_2.25. Both artifacts also load on Oracle Linux 7.9 (glibc 2.17) with no unresolved symbol versions, which tests the dynamic linker rather than the symbol table. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… EOL Debian (#790) * ci: drop the archived buster base default, bump the amd64 build image The x64-2.17 image's Dockerfile defaulted to openjdk:11-slim-buster. Debian 10 buster left even LTS support in June 2024 and its repositories are archived, so `apt-get update` against them now fails with "does not have a Release file" -- and rebuild-images.sh builds without --build-arg BASE_IMAGE whenever BASE_IMAGE_LIBC_2_17 is unset, which reaches that default. The body of the file is yum-based, so a Debian base cannot work there at all; the default now names what CI actually passes (centos:7), with the glibc 2.17 rationale for staying on an EOL distro recorded next to the vault mirror URLs it already needs. The amd64 build image moves to Debian 13 trixie, the current stable release. It compiles no shipped native code -- build:x64 uses the glibc 2.17 image -- so its glibc is not a customer-facing floor. The arm64 image stays on bullseye: it is where build:arm64 compiles the shipped linux-arm64 libjavaProfiler.so, so that glibc (2.31) is the runtime floor for arm64 customers and trixie would raise it to 2.41. Both variables now say which of the two they are and why. Also drop the `|| true` from the base image's package install, and with it the dead `apk` branch -- Alpine has its own Dockerfile.musl. As written, a total install failure produced a "successful" image whose tools were simply missing, surfacing later as an unrelated job failure. Verified against the real archives: buster's apt repositories fail as described, bullseye's still resolve, and the full package list installs on trixie with hexdump present (bsdmainutils is still a real package there). Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: build the shipped binaries on AlmaLinux 8 with gcc-toolset-14 Release builds link -static-libstdc++ -static-libgcc, so the C++ runtime and unwinder in the shipped libjavaProfiler.so come out of the build image's toolchain. That makes the build image's support status a property of the artifact, not just of CI: on CentOS 7 those objects can never receive another security rebuild, and no gcc newer than devtoolset-10 was ever built for aarch64 there (the dts-11 collection ships binutils, elfutils and its own metapackage for aarch64, but no compiler). AlmaLinux 8 with gcc-toolset-14 is supported until 2029 and publishes the toolset for x86_64 and aarch64 alike, so one image definition now covers both architectures with one compiler version, replacing the hand-rolled CentOS 7 image and its vault/Rocky mirror repositories. almalinux:8 is a multi-arch manifest, so a single BASE_IMAGE_GLIBC serves both. Measured on the real artifact, built in this image for both architectures: max required symbol version GLIBC_2.27 (both x86_64 and aarch64) symbols above 2.17 expf (2.27), getentropy (2.25) dynamic dependencies libc, libdl, libm, libpthread, librt GLIBCXX/CXXABI references none So the runtime floor moves 2.17 -> 2.27, not to the base image's 2.28: symbol versions are per-function. 2.27 keeps Ubuntu 18.04 (glibc 2.27) in support and drops RHEL 7 (2.17), Amazon Linux 2 (2.26) and SLES 15 SP1 (2.26). Both symbols are addressable with a .symver pin if a lower floor is needed later. With the shipped arm64 build moved off the Debian image, that image no longer sets a customer-facing floor, so it moves to trixie alongside amd64 -- which also restores apt on the arm64 sanitizer jobs. Names now describe the purpose rather than a version that has changed twice: BUILD_IMAGE_{X64,ARM64}_GLIBC and BASE_IMAGE_GLIBC, with tag suffixes {x64,arm64}-glibc-base. The variables still hold the current images until rebuild-images.sh publishes the new tags. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: move the C++ test toolchain into the build image The sanitizer jobs installed cmake, libgtest-dev, libgmock-dev, binutils, libc6-dbg and llvm in a before_script, which made every run depend on the distro archive still serving them, and which does not survive the move to trixie: sanitizer/asan_interface.h is no longer part of the llvm package there, so compiling linearAllocator.cpp fails with fatal error: 'sanitizer/asan_interface.h' file not found The header comes from libclang-rt-<n>-dev. The packages now live in the image instead, using the unversioned libclang-rt-dev metapackage so the right clang runtime is picked up on whatever release the image is built from. Dropping the before_script override also stops these jobs bypassing the pipeline default, so they get the CANCELLED guard and the maven proxy export that every other job already had. Verified on a locally built trixie image (clang 19.1.7, gtest 1.16.0), aarch64: buildGtestAsan and buildGtestTsan both compile, and all 62 asan and 62 tsan binaries pass. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: repin the build images onto the rebuilt bases Pins from the rebuild in pipeline 137281788, which built every image from the definitions on this branch. Verified by pulling each digest: x64-glibc-base AlmaLinux 8.10, glibc 2.28, gcc 14.2.1 arm64-glibc-base AlmaLinux 8.10, glibc 2.28, gcc 14.2.1 x64-base Debian 13 trixie, glibc 2.41, clang 19.1.7 arm64-base Debian 13 trixie The musl, datadog-ci and benchmark images come from the same rebuild; their definitions did not change on this branch. These pins belong here rather than in the image-update PR the rebuild opened against main: main has no BUILD_IMAGE_*_GLIBC variables, so the two shipped-binary pins silently did nothing there, while BUILD_IMAGE_ARM64 -- which main still uses for build:arm64 and stresstest:arm64 -- would have moved the shipped arm64 binary onto a trixie image and its glibc 2.41 floor. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: point the base image's default at the release CI actually uses The ARG default was still the bullseye digest while both OPENJDK_BASE_IMAGE variables moved to trixie, and the comment justified it as "the more conservative of the two" for an arm64 glibc floor that no longer exists here: the shipped binaries come from .gitlab/base/el8/Dockerfile, so neither arch of this image sets a customer floor. It also stopped being a harmless default. With the package install no longer ending in `|| true`, a direct `docker build` of this file -- or rebuild-images.sh reaching its documented unset-variable fallback -- hit bullseye's pruned security pool and failed on exactly the 404s this PR exists to fix. Verified: building with no BASE_IMAGE argument now produces a trixie image. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: assert the shipped library's ABI floor in the build job The oldest host the shipped library can load on is decided entirely by the build image: the highest versioned glibc symbol it references sets the glibc floor, and the static-link flags decide whether it needs a C++ runtime from the host at all. Nothing checked either, so both could move without a source change and without a failing job. check-abi-floor.sh asserts both against SHIPPED_GLIBC_FLOOR, declared in config.env next to the image pin that determines it, and build.sh runs it on each glibc artifact before the job ends. musl targets carry no glibc symbol versions and are skipped. Failures name the offending symbols or libraries, since the point of the check is to say what needs fixing. Three cases fail loudly rather than reporting a pass: an artifact with no versioned glibc symbols, one with no NEEDED entries, and a sort that cannot order versions. Each would otherwise leave the check quietly inoperative, which is worse than not having it. The checker reads the artifact through $OBJDUMP, so its tests drive it with canned output and need no compiler or real shared object; they run in a new ci-script-tests job alongside shellcheck. Every guard was mutation-checked: disabling it individually turns a named assertion red. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: pin expf to its GLIBC_2.17 interface and tighten the floor to 2.25 glibc 2.27 added a faster expf under a new symbol version and made it the default, so building on AlmaLinux 8 made the shipped library require expf@GLIBC_2.27 from source that has not changed. glibcCompat.h selects the GLIBC_2.17 interface explicitly, which is still exported and still maintained; .symver picks which of the versions already in the host's libc is called and bundles nothing, so a host on a newer glibc still runs its own implementation. Measured on the AlmaLinux 8 + gcc-toolset-14 build: expf moves from GLIBC_2.27 to GLIBC_2.17 and the artifact's requirement drops from 2.27 to 2.25, so the floor follows to 2.25. The remaining above-2.17 symbol is getentropy, which libstdc++ references from its own std::random_device and which glibc exports in only one version -- no pin can reach it, so 2.17 waits on removing the std::random_device uses. The pin is guarded to glibc: musl has no symbol versioning and a .symver naming a GLIBC_* version fails to link there, which would have broken both musl targets. Verified by linking and running a TU that reaches expf through poissonSampler.h under musl, where it emits no GLIBC_ references at all. The floor check could not read the artifact the pin produces. objdump parenthesises a binding to a non-default version, which is exactly what .symver creates, and the parser matched only the bare form -- so it found no versioned references on a pinned library. The empty-result guard turned that into a loud failure rather than the silent pass it would otherwise have been, which is what it exists for. The parser now accepts both forms, with fixtures for a pinned artifact at and above the floor. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: pin expf per architecture and run both script suites through bash The expf pin named a version that does not exist on x86_64, so every link against it failed there: nativeSocketSampler.o: undefined reference to `expf@GLIBC_2.17' no symbol version section for versioned symbol `expf@GLIBC_2.17' A symbol's oldest version is the baseline of the glibc release that introduced the port, which differs per architecture: x86_64 has expf@GLIBC_2.2.5 and aarch64 expf@GLIBC_2.17, both alongside the GLIBC_2.27 default added in 2.27. Naming a version the architecture does not export is a link error rather than a fallback, so the pin now selects per architecture and an unlisted one gets no pin at all, leaving the ABI floor check to report what it requires. This took out every x86_64 job that links the library: the two sanitizer gtest builds, and the CodeQL and Analyze jobs, which build linkRelease. Verified by linking and running a translation unit that reaches expf through poissonSampler.h under clang on Debian trixie -- the toolchain the sanitizer jobs use -- on both architectures, and by building the release artifact on AlmaLinux 8 for both: expf binds to each architecture's oldest version, getentropy@GLIBC_2.25 is the only remaining reference above 2.17, and the floor check passes at the declared 2.25 on both. Separately, ci-script-tests invoked the test suites directly. includes_test.sh is not executable and documents being run through bash, so the job failed with exit 126 after its own assertions had all passed. Both suites now run through bash, which leaves the existing file's mode alone. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * ci: lower the shipped floor to 2.17 now that getentropy is gone #794 removed the std::random_device uses, so libstdc++'s getentropy reference no longer enters the link and the last symbol above 2.17 is gone. With expf already pinned per architecture, the artifact requires nothing newer than the glibc that EL7 ships. Measured on release builds in the AlmaLinux 8 image on both architectures: no references above 2.17 at all, and the floor check passes at 2.17 where it previously named getentropy@GLIBC_2.25. Both artifacts also load on Oracle Linux 7.9 (glibc 2.17) with no unresolved symbol versions, which tests the dynamic linker rather than the symbol table. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
What does this PR do?:
Gives the profiler one random source instead of two.
xorshift.hholds thegenerator and the draws built on it; the four places that had their own copy of
the recurrence now call it, and the three that used
std::mt19937seeded fromstd::random_deviceare converted.PoissonSamplerxorshift::ProfiledThread::_fi_rngxorshift::faultInjectionfallbackxorshift::LivenessTrackermt19937+uniform_real_distribution, per thread, heapThreadLocal<u64>ReservoirSamplermt19937+ two distributionsxorshift::BaseWallClockmt19937+normal_distributionxorshift::nextNormal<random>is gone from the library.Motivation:
poissonSampler.halready argued the case, and the argument was never specificto that one sampler:
std::random_deviceand the distributionwrappers may allocate and may throw.
LivenessTracker::trackruns onallocation callbacks on arbitrary application threads, where neither is safe.
std::random_devicemay block or return low-entropy valuesdepending on the host, and which it does is decided by the libstdc++ the
library was compiled against, not by anything in this source. That made a
sampling-relevant behaviour a property of the build image.
std::mt19937carries ~2.5 KB of state.LivenessTrackerheld oneper thread, on the heap, plus a distribution object — and the lazy-create /
explicit-free machinery that existed only because they were heap-allocated.
Side effect that matters elsewhere.
std::random_deviceis also why theshipped library referenced
getentropy@GLIBC_2.25. That reference is not in ourcode: libstdc++ decides at its build time how to seed
std::random_device, andfrom GCC 12 on it calls
getentropywhere the glibc it was configured againsthas it — so
-static-libstdc++copiesrandom.oand that undefined referenceinto
libjavaProfiler.so. glibc has only one version ofgetentropy, so thereis no older symbol to pin it to. Removing the three
std::random_deviceuseskeeps
random.oout of the link entirely, which is what lets #790 hold theglibc 2.17 floor while building on a supported EL8 toolchain — the remaining
above-2.17 reference there is
expf, which is in our own code and can be.symver-pinned. This PR stands on the reasons above regardless of thatdecision; the floor is a beneficiary, not the motivation.
Additional Notes:
Seeding is no longer OS-backed. A stream is an identity mixed with a sequence
number — an object address with an epoch, a thread id with a tick. That yields
independent streams per thread and per recording without an entropy source.
Sampling needs independence, not unpredictability, so this is a deliberate
trade rather than an oversight: two processes started on the same tick with
ASLR disabled would correlate. Nothing here is used for anything security-
relevant.
ReservoirSamplernow takes its stream as a constructor argument. Seedingfrom the instance address alone meant every sampler built at one call site got
the same stream, so consecutive recordings sampled identically. Making it a
required argument means a caller cannot silently inherit a fixed stream; the
wall-clock loop passes
TSC::ticks().The wall-clock dither keeps its shape. It was
normal_distribution(interval, interval/10)and still is, drawn by Box-Muller. The alternatives (a flatdither, sum-of-uniforms) would have changed observable behaviour for no reason.
LivenessTracker's subsample decision is now an integer compare against athreshold computed once in
initialize(), the same devicefaultInjectionalready used for its probability tiers.
How to test the change?:
ddprof-lib/src/test/cpp/xorshift_ut.cpp— 23 assertions, picked upautomatically by the gtest plugin:
including when the two inputs cancel exactly; distinct identities give
distinct streams; small sequence numbers (0, 1, 2 …) are spread rather than
differing in a few low bits
and over 10k draws, with
log()finite throughout; roughly uniform by decileand mean
double; monotonic; fires at the requested rate (0.01 … 0.9)
and balances the range; does not favour low indices for a non-power-of-two n,
which a modulo mapping would
returns the mean for stddev 0
when it does not; writes every slot; distinct streams give distinct samples
and the same stream gives the same samples; empty input; reuse across calls
Two defects in the first draft were found by these tests and fixed:
toUnitDouble(UINT64_MAX)returned exactly 1.0 (1 - 2^-65rounds up), whichwould make
log(1 - u)infinite for a later caller; and everyReservoirSamplerat one call site produced byte-identical output. Both have tests pinned to the
specific failure.
Also run: the full
gtestDebugsuite, andgtestDebug_faultInjection_utunder-PenableFaultInjection— the three tier-rate tests there exercise theconsolidated generator statistically, and the fault-injection paths are not
compiled by the default build.
For Datadog employees:
🤖 Generated with Claude Code