Repository navigation
Cache last occluding atom in Vacc_atomSurf - #332
Conversation
Consecutive points on an atom's reference sphere are spatially adjacent and are usually buried by the same neighboring atom. Cache a stack-local pointer to the previous occluding Vatom in Vacc_atomSurf and test it before looking up the Vclist cell, preserving accessibility results and OpenMP thread safety.
sobolevnrm
left a comment
There was a problem hiding this comment.
This is a well-scoped optimization, and the reported performance improvements are encouraging.
The stack-local cache avoids introducing shared mutable state, and the cached overlap test preserves the existing distance comparison and self-exclusion for the current caller.
I’d request one compatibility fix before merging.
In src/generic/vacc.c:102–111, the cached-neighbor check returns before Vclist_getCell.
For a manually bounded grid (CLIST_MANUAL_DOMAIN), a surface point can lie outside the lookup domain while still overlapping a previously cached neighbor.
The original implementation classifies that point as accessible because the cell lookup returns NULL; the new implementation can classify it as buried.
I reproduced this difference using consecutive surface samples.
Both current production construction sites use CLIST_AUTO_DOMAIN, so this is a library-API compatibility issue rather than a demonstrated regression in ordinary APBS calculations.
Please restrict caching to automatic domains, or otherwise preserve the existing out-of-domain behavior without losing the optimization.
Please also add an automated regression test covering cached hits, fallback after a miss, changes in the occluding neighbor, and the manual-domain boundary case.
With that compatibility issue addressed and the remaining CI checks passing, I would be comfortable merging this.
Under CLIST_MANUAL_DOMAIN, a surface point can fall outside the manual lookup bounds (where Vclist_getCell returns VNULL and classifies the point as accessible) while still overlapping the previously cached neighbor. Disable last_hit caching unless thee->clist->mode is CLIST_AUTO_DOMAIN, and add a CTest unit regression test covering consecutive cached hits, fallback after a cache miss, changes in the occluding neighbor, and out-of-domain manual-grid samples.
|
Thanks for the review! I updated |
|
Wonderful; I will merge. Thanks! |
Summary
When building an atom's solvent-accessibility surface,
Vacc_atomSurf(src/generic/vacc.c#L866-L902) tests every point on the atom's reference sphere withivdwAccExclus(src/generic/vacc.c#L78-L128), which looks up the probe point'sVclisthash cell and scans the cell's atom list from index 0 until an overlapping neighbor is found. Because consecutive points on the reference sphere are spatially adjacent, neighboring buried points are usually occluded by the same atom, repeating the cell lookup and list scan many times per atom.This change caches a stack-local pointer (
Vatom *last_hit) inVacc_atomSurfto the last atom that buried a probe point and tests that atom first before queryingVclistwhenthee->clistuses an automatic domain (CLIST_AUTO_DOMAIN).Behavior and outputs are unchanged:
ivdwAccExclusCachehas already verifiedatom->id != atomIDand overlap (dist2 < (atom->radius + radius)^2), and the fast path applies the identical distance check.Vclistdomains (CLIST_AUTO_DOMAIN), where any point overlapping a cached neighbor inthee->alistwithradius <= Vclist_maxRadius(thee->clist)lies inside the inflatedVclistlookup grid. Manual domains (CLIST_MANUAL_DOMAIN) leavelast_hitdisabled so out-of-domain sample points continue to hitVclist_getCell(...) == VNULLand return accessible (1).last_hitlives on the stack ofVacc_atomSurf, adding no shared mutable state.Changes
ivdwAccExclusCache(..., Vatom **last_hit)insrc/generic/vacc.cthat gateslast_hitonthee->clist->mode == CLIST_AUTO_DOMAIN, checks*last_hitbeforeVclist_getCell, and updates*last_hitwhen a cell atom buries the probe point.ivdwAccExclusas a thin wrapper passingVNULLfor its other callers (ivdwAccandVacc_ivdwAcc), and callivdwAccExclusCachefromVacc_atomSurfwith stack-local&last_hit.vacc_surf_test(tests/vacc_surf_test.c, registered intests/CMakeLists.txt) covering consecutive cached hits, fallback after a cache miss, changes in the occluding neighbor, and out-of-domainCLIST_MANUAL_DOMAINsurface samples.Validation
-DENABLE_iAPBS=ON -DBUILD_TOOLS=ON, and-DENABLE_OPENMP=ONconfigurations with zero new compiler warnings (-Wall -Wextra -Wunused -Wdeclaration-after-statement, and zero total warnings ontests/vacc_surf_test.c). Verified thatvacc_surf_testreproduces theCLIST_MANUAL_DOMAINmismatch on34244afand passes after the fix. Passedctest(14/14targets) andapbs_tester.py --ocd(1e-12relative tolerance onborn,FKBP,hca-bind,ion-protein,pka-lig, andsolv), with identical printed energies and apolar SASA/SAV outputs across all benchmarked runs (0.00e+00diff) and identical 4-thread vs 1-thread OpenMP energies onexamples/actin-dimer/apbs-smol-auto.in.gcc -O2, 1 core on AMD EPYC 7B13, median of 5 interleaved runs againstmain327ddc6):srfm smol,sdens 10.0)Setup(main-> PR)main)examples/pka-lig/apbs-smol-surf.in4.08 -> 2.33 s(43.1% less)6.24 s4.49 sexamples/hca-bind/apbs-smol.in4.30 -> 2.23 s(48.2% less)5.25 s3.19 sexamples/actin-dimer/apbs-smol-auto.in14.60 -> 6.43 s(55.9% less)40.97 s32.78 s1EBZMM/PBSA (3 frames,129x97x97+apolar*)15.17 -> 10.21 s(32.7% less)27.91 s22.34 s*
ApolarSASA/SAV time on1EBZMM/PBSA also drops by 51.3% (1.29 -> 0.63 s).Prepared with AI assistance.