Reduce memcpys in substruct search and improve benchmark autotuning - #240
Merged
scal444 merged 4 commits intoJul 31, 2026
Merged
Conversation
scal444
commented
Jul 30, 2026
scal444
commented
Collaborator
- We had been uploading query patterns for each minibatch, it's now done once up front
- Default to sanitize, no-sanitize is for preprocessed datasets and should be opt in
- Increased batch scan and made it log scale
- Increased default autotune cap, 2k was setup-bound and didnt give useful enough signal
The batchSize search range was (128, 1024, step=128), and tuning runs were pinning to 1024 -- the ceiling of the range, not an interior optimum. Raise the ceiling to 8192, matching what benchmarks/substruct_config.csv already uses, and switch to log spacing since the useful range spans two orders of magnitude. The step=128 bought nothing: the runtime derives targetsPerBatch = batchSize // numQueries, and that truncation destroys any 128 alignment before it reaches a kernel. Default calibration_max_size to None (uncapped) for tune_substructure. The 2000-target cap left too few mini-batches to fill every worker's executor ring, so pipeline fill and drain dominated the measurement and large batchSize values were systematically under-rewarded. auto_subsample and normalize_calibration_set now accept max_size=None; other tuners keep the 2000 default. Also default substruct_bench.py to --sanitize. It was the only benchmark defaulting to sanitize=False, while etkdg_bench and ff_optimize_bench both default to True. Move the flag pair into a shared bench_utils.cli helper so the mutually-exclusive group and the safe default stay consistent.
preprocessMiniBatch re-staged and re-uploaded the recursive SMARTS pattern entries on every pattern sub-batch, of every mini-batch, on every worker. The data is identical every time: MiniBatchPlanner::prepareRecursiveMiniBatch points plan.patternsAtDepth at leafSubpatterns_.allQueriesPatternsAtDepth, a table built once by buildAllPatterns and shared across all mini-batches. On a 200k-target x 158-query hasSubstructMatch run that was 133,333 uploads costing 4,979 ms of aggregate worker time -- 44% of all time the six worker threads spent launching work, and the single largest item in the profile. Most of it was not the 428-byte copy itself but the handshake around it: the pinned staging area is only double-buffered, so with four sub-batches per mini-batch acquireBufferIndex kept returning a buffer whose copy was still queued behind a full depth level of paint kernels, and waitForBuffer blocked the worker on cudaEventSynchronize. Those accounted for roughly 135k of the 168k blocking event syncs on worker threads; the actual result drain was only 221 ms. Upload the table once in LeafSubpatterns::syncToDevice, which already runs on the setup stream that callers synchronize before starting any worker. Each sub-batch is a contiguous slice of its depth's entries, so the kernels now take a pointer into the device table rather than a per-sub-batch staging buffer, and the whole acquire/wait/fill/copy/record sequence is gone. The non-pipeline preprocessRecursiveSmarts path builds its own local pattern vectors and is left unchanged.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
| Filename | Overview |
|---|---|
| src/substruct/recursive_preprocessor.cu | Moves immutable recursive pattern tables to device-resident storage shared across minibatches. |
| src/substruct/substruct_search.cu | Synchronizes secondary-device default-stream uploads before workers using independent streams are launched. |
| nvmolkit/autotune/tune_substructure.py | Expands batch-size and calibration sampling while using the existing supported log-uniform tuple format. |
| nvmolkit/tests/test_autotune.py | Updates the default search-space assertion to validate the log-uniform batch-size specification. |
| benchmarks/substruct_bench.py | Makes sanitization the benchmark default while retaining an explicit opt-out. |
Reviews (2): Last reviewed commit: "Fix substructure setup ordering and auto..." | Re-trigger Greptile
evasnow1992
approved these changes
Jul 30, 2026
evasnow1992
left a comment
evasnow1992
left a comment
Collaborator
There was a problem hiding this comment.
Thank for the fix and optimization. Changes look good to me.
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
2 participants
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.