DualArray::resize leaves the grown device tail undefined (pinned build), silently NaN-poisoning contact forces - #74
Open
DanNegrut wants to merge 2 commits into
Open
Conversation
…variant) In the pinned-memory DualArray, resize(n, val) filled only the host vector: resizeHost(n, val) applies the fill, then resizeDevice(n) either reallocates and copies forward pre-existing elements only, or returns early when capacity already suffices. Either way the device range [old_size, n) was never written, while all 104 call sites pass a fill value in the reasonable belief that it defines the new elements. The same asymmetry existed in the no-value flavor, whose host side is value-initialized by std::vector::resize. Consequence in practice: contactEventArraysResize grows the per-contact arrays mid-simulation once the true contact count exceeds the initial one-slot-per-sphere sizing. Candidate pairs demoted to NOT_A_CONTACT in calculateContactForces never have their contactPointGeometryA/B written (the demotion is local to the kernel by design, so slots below the live count legitimately go unwritten in a step), and forceToAcc consumed uninitialized device memory for those slots: cross(garbage, 0) is 0 for finite garbage but NaN when the bits decode to NaN or Inf, and the NaN enters angular acceleration via atomicAdd. The kT-side velocity health check cannot see it because a CUB max reduction discards NaN operands (IEEE comparisons against NaN are false), so affected runs corrupted silently and then stalled: 6 of 40 async bed-settling runs blew up, with 60-190 s control steps and GB-scale memory growth. Evidence: compute-sanitizer --tool initcheck on a settling-bed repro attributes the reads to forceToAcc by name. Stock totals are large and run-dependent (two async runs: 23,357 and 15,226 total errors; the run counted by class had 14,893 device-side uninitialized reads). With this fix, and DEME_INIT_CNT_MULTIPLIER kept at 1, device-side uninitialized reads are 0 in all three configurations measured (async, synchronous with a fixed contact margin, synchronous without); each reports 20 total errors, all belonging to a separate pre-existing host-side defect in DEMSolverScratchData::allocateScratchSpace that this change neither causes nor closes. memcheck reports no invalid access attributable to this change. It did surface, once in five runs of the fixed build, an out-of-bounds read in rearrangeContactWildcards; that array is dispensed by DeviceVectorPool (built on DeviceArray) while this change touches DualArray only, and the defect is reported upstream separately. Two independent checks corroborate the mechanism and the neutrality of the fix. Avoiding growth altogether via an 8x initial contact buffer drove failures from 6/40 to 0/40 (Fisher exact p = 0.026), which is the mechanism confirmed by a route that does not touch this code. And settled-bed physics is unchanged within measurement resolution: 41 interleaved runs, solid fraction difference 1.2 sigma against a ~0.3% single-run scatter. The fix pushes the freshly filled host tail to the device after resizeDevice, in both resize flavors, using the existing partial toDevice(start, count). Only the grown tail is copied: device data in [0, old_size) remains authoritative and untouched. The managed-memory variant needs no code change (one allocation serves both sides); its copy-pasted "fills host values only" comment is corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment-only. States the two invariants the tail-push contract relies on, raised in code review: (1) resize() assumes device [0, old_size) is already defined and does not repair a host-only prefix created by growing through resizeHost() directly (no in-tree caller does; verified by grep); (2) shrink and same-size calls return without touching device memory, so capacity >= logical size is the invariant and bytes past the new logical size are dead. Co-Authored-By: Claude Opus 5 (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
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.
Related to #71, and complementary to @Ward-Vandepitte's #72, which identified this same root cause
three weeks earlier and which I had not seen when I prepared this. #72 has the better account of
how the corruption becomes a stall: a NaN quaternion makes every bin-range comparison in
getNumberOfBinsEachSphereTouchesevaluate false, so the sphere is assigned the entire bin gridand contact detection degenerates into an all-pairs sweep. That is the link I was missing.
This PR is one change at a different level of the stack, and touches no file that #72 touches.
#72 fixes the two symptomatic call sites, with an explicit
cudaMemsetofcontactPointGeometryA/Bafter eachDEME_DUAL_ARRAY_RESIZE, plus two defensive layers (a loudabort on non-finite bin coordinates, and a zero-force early-out in
forceToAngAccthat alsoavoids the MOI and quaternion loads for the majority of entries). Its own comment states the
container contract that forces the workaround: "DualArray::resize(n, val) fills host values only".
This PR fixes that contract instead. In the pinned-memory build (
DEME_USE_MANAGED_ARRAYSoff),
resizeDevicenever initialises grown elements: on reallocation it copies forward only thepre-existing data, and when capacity already suffices it returns without touching memory. So
device elements
[old_size, n)are undefined on both growth paths, in bothresizeflavors. Thefix captures the old size and pushes exactly the grown range host-to-device through the class's
own
toDevice(start, n). Device data in[0, old_size)is never overwritten, so arrays whosedevice copy is authoritative mid-simulation are unaffected. The managed variant needs no code
change; only its copy-pasted comment is corrected.
The reason to do both: 104 call sites pass a fill value in the reasonable belief that it defines
the new elements. #72 makes that true for the two arrays that were observed to bite. Any other
DualArraythat grows and is read on device before being written keeps the defect, and the nextoccurrence would present as a fresh mystery rather than as this known one. If #72 lands first its
memsets become redundant but remain harmless, and its two defensive layers keep their independent
value.
Measurements, on a settling bed of 3,615 spheres, one RTX 5090, CUDA 12.8, commit
0fdc6de1:0fdc6de1async0fdc6de1synchronous + user expand factorStock counts vary run to run because how much the contact arrays grow mid-run depends on
scheduling; the reproducible part is the contrast with zero. Blocking growth by a different route,
an 8x initial contact buffer, independently drove failures from 6/40 to 0/40 (Fisher exact
p = 0.026), which confirms the mechanism without touching this code. Settled-bed physics is
unchanged within resolution: 41 interleaved runs, solid fraction difference 1.2 sigma against a
~0.3% single-run scatter.
Two disclosures. The residual 20 errors in every fixed configuration are a separate pre-existing
host-side defect,
DEMSolverScratchData::allocateScratchSpacehandingcudaMemcpya partlywritten pinned buffer; it appears in configurations that never fail, so it is not this mechanism,
and this change neither causes nor closes it. And memcheck surfaced, once in ten completed runs of
the fixed build and never in eleven of stock, an out-of-bounds read in
rearrangeContactWildcardsreached through
migrateEnduringContacts. Fisher exact on 1/10 against 0/11 is p = 0.48, so thatcomparison is inconclusive rather than exonerating. I attribute it elsewhere on source grounds
rather than statistical ones: that array is dispensed by
DeviceVectorPool, built onDeviceArray, while this change touchesDualArrayonly, and where it does touch the wildcardarrays it makes their grown tails defined rather than undefined, which cannot manufacture an
out-of-range index. Reported separately rather than buried here.
Full mechanism, evidence and the preconditions the tail-push relies on are in the commit bodies.