refactor: convert dfsBuildStack to DFSStackBuilder class with reusable state#7
Open
refactor: convert dfsBuildStack to DFSStackBuilder class with reusable state#7
Conversation
The seenFromHere bitset was being allocated on every recursive call. By converting to a class (DFSStackBuilder), we allocate it once and reset per call. Also reuse ringsClosed and possibles vectors.
== /workdir/tmp/bench_results/master.txt → /workdir/tmp/bench_results/opt-canon-dfs.txt == Benchmark Old (mean±sd) New (mean±sd) Δ% Speed Sig(p) --------------------------------------------------------------------------------------------------------------------------- bench_common::nth_random 0.546 ns ± 0.00258 ns 0.549 ns ± 0.00442 ns +0.5% 1.00× slower ns p=0.372 Chirality::findPotentialStereo 609 us ± 8.21 us 608 us ± 59.4 us -0.2% 1.00× faster ns p=0.972 CIPLabeler::assignCIPLabels 264 ms ± 1.5 ms 264 ms ± 1.79 ms +0.2% 1.00× slower ns p=0.709 Descriptors::calcNumBridgeheadAtoms 1.06 us ± 7.84 ns 1.02 us ± 31.9 ns -3.2% 1.03× faster ns p=0.0731 Descriptors::calcNumSpiroAtoms 1.29 us ± 16 ns 1.27 us ± 9.18 ns -1.6% 1.02× faster sig p=0.0458 InchiToInchiKey 35.3 us ± 5.69 us 36.4 us ± 5.65 us +3.1% 1.03× slower ns p=0.813 InchiToMol 4.23 ms ± 31.4 us 4.2 ms ± 39.5 us -0.9% 1.01× faster ns p=0.215 memory pressure test 11.3 us ± 3.09 us 10.7 us ± 688 ns -5.9% 1.06× faster ns p=0.715 MolOps::addHs 425 us ± 8.26 us 423 us ± 14.8 us -0.6% 1.01× faster ns p=0.81 MolOps::assignStereochemistry legacy=false 764 us ± 34.9 us 719 us ± 2.72 us -5.9% 1.06× faster sig p=0.0264 MolOps::assignStereochemistry legacy=true 465 us ± 20.9 us 452 us ± 11.5 us -2.8% 1.03× faster ns p=0.341 MolOps::FindSSR 173 us ± 20 us 172 us ± 23.7 us -0.9% 1.01× faster ns p=0.931 MolOps::getMolFrags 1.04 ms ± 40.6 us 1.1 ms ± 35 us +6.1% 1.06× slower sig p=0.0407 MolPickler::molFromPickle 172 us ± 13.7 us 171 us ± 18 us -0.7% 1.01× faster ns p=0.932 MolPickler::pickleMol 111 us ± 5.05 us 115 us ± 2.74 us +3.5% 1.04× slower ns p=0.238 MolToCXSmiles 1.26 ms ± 99.2 us 1.26 ms ± 92.3 us -0.1% 1.00× faster ns p=0.989 MolToInchi 2.09 ms ± 107 us 2.03 ms ± 19.9 us -2.5% 1.03× faster ns p=0.41 MolToSmiles 1.02 ms ± 36.4 us 973 us ± 55.2 us -4.4% 1.05× faster ns p=0.237 MorganFingerprints::getFingerprint 185 us ± 2.53 us 186 us ± 2.6 us +0.7% 1.01× slower ns p=0.54 ROMol copy constructor 43.6 us ± 900 ns 42.8 us ± 565 ns -1.7% 1.02× faster ns p=0.214 ROMol destructor 18.7 us ± 2.39 us 17.8 us ± 904 ns -4.9% 1.05× faster ns p=0.536 ROMol::getNumHeavyAtoms 106 ns ± 2.77 ns 112 ns ± 3.57 ns +5.4% 1.05× slower sig p=0.0284 ROMol::GetSubstructMatch 33.3 us ± 266 ns 34.8 us ± 4.28 us +4.8% 1.05× slower ns p=0.521 ROMol::GetSubstructMatch patty 1.15 ms ± 38.6 us 1.12 ms ± 8.13 us -2.6% 1.03× faster ns p=0.187 ROMol::GetSubstructMatch RLewis 7.13 ms ± 124 us 7.07 ms ± 84.2 us -0.8% 1.01× faster ns p=0.526 SmilesToMol 1.21 ms ± 37.8 us 1.23 ms ± 5.07 us +1.6% 1.02× slower ns p=0.374 Summary: sig=4, below=0 (stat-sig but < threshold), ns=22, missing=0, new_only=0
== tmp/bench_results/master.txt → tmp/bench_results/opt-canon-dfs.txt == Benchmark Old (mean±sd) New (mean±sd) Δ% Speed Sig(p) ----------------------------------------------------------------------------------------------------------------------- bench_common::nth_random 1.57 ns ± 0.517 ns 1.67 ns ± 0.595 ns +6.5% 1.06× slower ns p=0.773 Chirality::findPotentialStereo 1.91 ms ± 280 us 1.5 ms ± 89.5 us -21.5% 1.27× faster sig p=0.00173 CIPLabeler::assignCIPLabels 591 ms ± 3.49 ms 589 ms ± 6.32 ms -0.3% 1.00× faster ns p=0.587 Descriptors::calcNumBridgeheadAtoms 4.99 us ± 1.57 us 6.12 us ± 2.04 us +22.7% 1.23× slower ns p=0.324 Descriptors::calcNumSpiroAtoms 6.04 us ± 1.85 us 5.71 us ± 1.9 us -5.5% 1.06× faster ns p=0.778 InchiToInchiKey 64.3 us ± 22.7 us 71.8 us ± 19.7 us +11.6% 1.12× slower ns p=0.597 InchiToMol 12 ms ± 546 us 11.9 ms ± 720 us -0.5% 1.01× faster ns p=0.89 memory pressure test 21.6 us ± 6.57 us 21.5 us ± 6.84 us -0.4% 1.00× faster ns p=0.986 MolOps::addHs 874 us ± 154 us 881 us ± 159 us +0.8% 1.01× slower ns p=0.941 MolOps::assignStereochemistry legacy=false 1.88 ms ± 113 us 2.02 ms ± 361 us +7.3% 1.07× slower ns p=0.465 MolOps::assignStereochemistry legacy=true 1.3 ms ± 159 us 1.14 ms ± 81.8 us -12.2% 1.14× faster ns p=0.0538 MolOps::FindSSR 398 us ± 61.5 us 397 us ± 60 us -0.1% 1.00× faster ns p=0.988 MolOps::getMolFrags 2.57 ms ± 333 us 2.37 ms ± 335 us -7.5% 1.08× faster ns p=0.359 MolPickler::molFromPickle 368 us ± 59.3 us 300 us ± 56.1 us -18.5% 1.23× faster ns p=0.0617 MolPickler::pickleMol 310 us ± 70.9 us 339 us ± 74.7 us +9.2% 1.09× slower ns p=0.536 MolToCXSmiles 2.61 ms ± 348 us 2.89 ms ± 166 us +10.7% 1.11× slower ns p=0.105 MolToInchi 6.46 ms ± 218 us 6.41 ms ± 381 us -0.8% 1.01× faster ns p=0.8 MolToSmiles 2.17 ms ± 238 us 2.15 ms ± 230 us -1.2% 1.01× faster ns p=0.862 MorganFingerprints::getFingerprint 713 us ± 146 us 750 us ± 150 us +5.2% 1.05× slower ns p=0.692 ROMol copy constructor 168 us ± 37.1 us 193 us ± 33.1 us +15.1% 1.15× slower ns p=0.254 ROMol destructor 70 us ± 14.5 us 73.1 us ± 16.7 us +4.4% 1.04× slower ns p=0.753 ROMol::getNumHeavyAtoms 359 ns ± 70.6 ns 323 ns ± 4.72 ns -9.9% 1.11× faster ns p=0.261 ROMol::GetSubstructMatch 134 us ± 32.8 us 118 us ± 32.3 us -12.5% 1.14× faster ns p=0.442 ROMol::GetSubstructMatch patty 3.79 ms ± 602 us 3.5 ms ± 313 us -7.7% 1.08× faster ns p=0.35 ROMol::GetSubstructMatch RLewis 23.2 ms ± 1.51 ms 23.1 ms ± 720 us -0.6% 1.01× faster ns p=0.86 SmilesToMol 3.35 ms ± 339 us 3.28 ms ± 218 us -2.1% 1.02× faster ns p=0.69 Summary: sig=1, below=0 (stat-sig but < threshold), ns=25, missing=0, new_only=0
== /home/ubuntu/rdkits/tmp/bench_results/master.txt → /home/ubuntu/rdkits/tmp/bench_results/opt-canon-dfs.txt == Benchmark Old (mean±sd) New (mean±sd) Δ% CV Speed Sig(p) -------------------------------------------------------------------------------------------------------------------------- bench_common::nth_random 1.41 ns ± 0.336 ns 1.3 ns ± 0.115 ns -7.9% 23.9% 1.09× faster ns p=0.517 Chirality::findPotentialStereo 1.58 ms ± 60.9 us 1.6 ms ± 83.9 us +1.3% 5.2% 1.01× slower ns p=0.681 CIPLabeler::assignCIPLabels 529 ms ± 3.58 ms 526 ms ± 4.16 ms -0.6% 0.8% 1.01× faster ns p=0.216 Descriptors::calcNumBridgeheadAtoms 4.33 us ± 550 ns 3.93 us ± 27.6 ns -9.2% 12.7% 1.10× faster ns p=0.181 Descriptors::calcNumSpiroAtoms 4.55 us ± 168 ns 5.67 us ± 1.8 us +24.4% 31.8% 1.24× slower ns p=0.24 InchiToInchiKey 54.8 us ± 15.8 us 59.2 us ± 16.1 us +7.9% 28.8% 1.08× slower ns p=0.68 InchiToMol 10.4 ms ± 281 us 10.4 ms ± 382 us -0.7% 3.7% 1.01× faster ns p=0.748 memory pressure test 19.5 us ± 1.82 us 22.4 us ± 6.13 us +15.0% 27.3% 1.15× slower ns p=0.355 MolOps::addHs 778 us ± 29 us 785 us ± 42.7 us +1.0% 5.4% 1.01× slower ns p=0.752 MolOps::assignStereochemistry legacy=false 1.95 ms ± 124 us 1.99 ms ± 103 us +2.0% 6.4% 1.02× slower ns p=0.611 MolOps::assignStereochemistry legacy=true 1.08 ms ± 65.5 us 1.11 ms ± 48.9 us +2.3% 6.1% 1.02× slower ns p=0.51 MolOps::FindSSR 395 us ± 22.4 us 403 us ± 33.4 us +1.9% 8.3% 1.02× slower ns p=0.688 MolOps::getMolFrags 2.25 ms ± 101 us 2.23 ms ± 110 us -0.6% 4.9% 1.01× faster ns p=0.848 MolPickler::molFromPickle 309 us ± 23.6 us 325 us ± 31.3 us +5.2% 9.6% 1.05× slower ns p=0.385 MolPickler::pickleMol 275 us ± 25.3 us 304 us ± 34.2 us +10.8% 11.2% 1.11× slower ns p=0.162 MolToCXSmiles 2.55 ms ± 138 us 2.58 ms ± 87.5 us +0.9% 5.4% 1.01× slower ns p=0.763 MolToInchi 5.76 ms ± 198 us 5.77 ms ± 104 us +0.1% 3.4% 1.00× slower ns p=0.961 MolToSmiles 1.97 ms ± 64.8 us 2 ms ± 65.2 us +1.6% 3.3% 1.02× slower ns p=0.466 MorganFingerprints::getFingerprint 607 us ± 35.2 us 654 us ± 47.1 us +7.8% 7.2% 1.08× slower ns p=0.113 ROMol copy constructor 154 us ± 23.5 us 163 us ± 13.4 us +5.3% 15.2% 1.05× slower ns p=0.521 ROMol destructor 63.5 us ± 8.41 us 71.6 us ± 5.4 us +12.8% 13.2% 1.13× slower ns p=0.112 ROMol::getNumHeavyAtoms 343 ns ± 24.8 ns 349 ns ± 66.7 ns +1.7% 19.1% 1.02× slower ns p=0.859 ROMol::GetSubstructMatch 112 us ± 19 us 110 us ± 12.7 us -2.1% 17.0% 1.02× faster ns p=0.826 ROMol::GetSubstructMatch patty 3.45 ms ± 123 us 3.45 ms ± 182 us +0.1% 5.3% 1.00× slower ns p=0.975 ROMol::GetSubstructMatch RLewis 21.3 ms ± 365 us 21.1 ms ± 295 us -1.2% 1.7% 1.01× faster ns p=0.259 SmilesToMol 2.92 ms ± 123 us 3.01 ms ± 131 us +2.9% 4.3% 1.03× slower ns p=0.326 Summary: sig=0, below=0 (stat-sig but < threshold), ns=26, missing=0, new_only=0 Bonferroni-corrected α = 0.001923 (26 tests, family α = 0.05)
== /home/ubuntu/rdkits/tmp/bench_results/master.txt → /home/ubuntu/rdkits/tmp/bench_results/opt-canon-dfs.txt == Benchmark Old (mean±sd) New (mean±sd) Δ% CV Speed Sig(p) ---------------------------------------------------------------------------------------------------------------------------- bench_common::nth_random 1.31 ns ± 0.178 ns 1.31 ns ± 0.167 ns -0.5% 13.5% 1.00× faster ns p=0.958 Chirality::findPotentialStereo 1.64 ms ± 137 us 1.65 ms ± 99.2 us +0.6% 8.4% 1.01× slower ns p=0.899 CIPLabeler::assignCIPLabels 576 ms ± 37.8 ms 575 ms ± 38.5 ms -0.3% 6.7% 1.00× faster ns p=0.949 Descriptors::calcNumBridgeheadAtoms 3.92 us ± 45.4 ns 4.06 us ± 141 ns +3.4% 3.5% 1.03× slower ns p=0.102 Descriptors::calcNumSpiroAtoms 5.15 us ± 1.49 us 5.42 us ± 1.43 us +5.1% 28.9% 1.05× slower ns p=0.782 InchiToInchiKey 57.9 us ± 12.7 us 52.2 us ± 1.62 us -9.8% 21.9% 1.11× faster ns p=0.374 InchiToMol 10.8 ms ± 191 us 10.7 ms ± 215 us -0.8% 2.0% 1.01× faster ns p=0.508 memory pressure test 17.6 us ± 3.93 us 19.3 us ± 3.87 us +10.0% 22.4% 1.10× slower ns p=0.497 MolOps::addHs 777 us ± 34.3 us 810 us ± 67.6 us +4.2% 8.3% 1.04× slower ns p=0.371 MolOps::assignStereochemistry legacy=false 1.9 ms ± 99.3 us 2.02 ms ± 116 us +6.2% 5.8% 1.06× slower ns p=0.121 MolOps::assignStereochemistry legacy=true 1.16 ms ± 33.4 us 1.12 ms ± 65.3 us -3.1% 5.8% 1.03× faster ns p=0.319 MolOps::FindSSR 405 us ± 24.6 us 404 us ± 51.5 us -0.5% 12.8% 1.00× faster ns p=0.942 MolOps::getMolFrags 2.28 ms ± 146 us 2.33 ms ± 177 us +2.2% 7.6% 1.02× slower ns p=0.635 MolPickler::molFromPickle 316 us ± 37.1 us 310 us ± 28.5 us -1.9% 11.7% 1.02× faster ns p=0.777 MolPickler::pickleMol 308 us ± 29.3 us 304 us ± 43.7 us -1.3% 14.4% 1.01× faster ns p=0.874 MolToCXSmiles 2.62 ms ± 125 us 2.61 ms ± 145 us -0.4% 5.6% 1.00× faster ns p=0.902 MolToInchi 6.18 ms ± 238 us 6.08 ms ± 226 us -1.6% 3.9% 1.02× faster ns p=0.53 MolToSmiles 1.97 ms ± 62 us 2.02 ms ± 82.7 us +2.7% 4.1% 1.03× slower ns p=0.286 MorganFingerprints::getFingerprint 649 us ± 47.3 us 599 us ± 40.4 us -7.6% 7.3% 1.08× faster ns p=0.114 ROMol copy constructor 153 us ± 20.9 us 157 us ± 24.1 us +2.8% 15.3% 1.03× slower ns p=0.771 ROMol destructor 60.7 us ± 5.22 us 67.8 us ± 6.03 us +11.6% 8.9% 1.12× slower ns p=0.0836 ROMol::getNumHeavyAtoms 362 ns ± 74.2 ns 340 ns ± 23.5 ns -5.9% 20.5% 1.06× faster ns p=0.565 ROMol::GetSubstructMatch 109 us ± 11.5 us 110 us ± 8.36 us +0.4% 10.6% 1.00× slower ns p=0.953 ROMol::GetSubstructMatch patty 3.5 ms ± 110 us 3.52 ms ± 167 us +0.6% 4.7% 1.01× slower ns p=0.83 ROMol::GetSubstructMatch RLewis 21.9 ms ± 573 us 22.3 ms ± 132 us +1.9% 2.6% 1.02× slower ns p=0.183 SmilesToMol 2.99 ms ± 203 us 3.03 ms ± 226 us +1.3% 7.5% 1.01× slower ns p=0.787 Summary: sig=0, below=0 (stat-sig but < threshold), ns=26, missing=0, new_only=0 Bonferroni-corrected α = 0.001923 (26 tests, family α = 0.05)
== /home/ubuntu/rdkits/tmp/bench_results/master.txt → /home/ubuntu/rdkits/tmp/bench_results/opt-canon-dfs.txt == Benchmark Old (mean±sd) New (mean±sd) Δ% CV Speed Sig(p) ---------------------------------------------------------------------------------------------------------------------------- bench_common::nth_random 1.38 ns ± 0.152 ns 1.43 ns ± 0.139 ns +3.6% 11.0% 1.04× slower ns p=0.345 Chirality::findPotentialStereo 1.57 ms ± 84.5 us 1.58 ms ± 80.3 us +0.9% 5.4% 1.01× slower ns p=0.626 CIPLabeler::assignCIPLabels 580 ms ± 37.4 ms 572 ms ± 39.2 ms -1.3% 6.9% 1.01× faster ns p=0.585 Descriptors::calcNumBridgeheadAtoms 4.41 us ± 491 ns 4.36 us ± 533 ns -1.2% 12.2% 1.01× faster ns p=0.78 Descriptors::calcNumSpiroAtoms 4.74 us ± 363 ns 4.9 us ± 589 ns +3.3% 12.0% 1.03× slower ns p=0.372 InchiToInchiKey 50 us ± 4.76 us 49.1 us ± 4.17 us -1.8% 9.5% 1.02× faster ns p=0.564 InchiToMol 10.5 ms ± 303 us 10.5 ms ± 297 us -0.3% 2.9% 1.00× faster ns p=0.776 memory pressure test 17.5 us ± 3.84 us 17.2 us ± 3.79 us -1.7% 22.0% 1.02× faster ns p=0.831 MolOps::addHs 790 us ± 38.9 us 778 us ± 43.7 us -1.6% 5.6% 1.02× faster ns p=0.405 MolOps::assignStereochemistry legacy=false 1.92 ms ± 59.6 us 1.94 ms ± 90 us +0.8% 4.7% 1.01× slower ns p=0.557 MolOps::assignStereochemistry legacy=true 1.13 ms ± 60.4 us 1.12 ms ± 63.4 us -1.0% 5.7% 1.01× faster ns p=0.616 MolOps::FindSSR 369 us ± 20.3 us 383 us ± 17.2 us +3.9% 5.5% 1.04× slower ns p=0.0408 MolOps::getMolFrags 2.26 ms ± 89 us 2.3 ms ± 126 us +1.7% 5.5% 1.02× slower ns p=0.328 MolPickler::molFromPickle 297 us ± 16.9 us 297 us ± 16.4 us +0.2% 5.7% 1.00× slower ns p=0.905 MolPickler::pickleMol 278 us ± 18.5 us 276 us ± 15.2 us -0.6% 6.7% 1.01× faster ns p=0.779 MolToCXSmiles 2.55 ms ± 109 us 2.57 ms ± 162 us +0.7% 6.3% 1.01× slower ns p=0.736 MolToInchi 6.09 ms ± 154 us 6.01 ms ± 146 us -1.3% 2.5% 1.01× faster ns p=0.145 MolToSmiles 1.99 ms ± 87.9 us 1.97 ms ± 122 us -0.6% 6.2% 1.01× faster ns p=0.743 MorganFingerprints::getFingerprint 626 us ± 16.1 us 615 us ± 22.4 us -1.8% 3.6% 1.02× faster ns p=0.118 ROMol copy constructor 155 us ± 9.8 us 145 us ± 9.35 us -6.4% 6.5% 1.07× faster ns p=0.00624 ROMol destructor 62.4 us ± 3.75 us 63.8 us ± 3.95 us +2.2% 6.2% 1.02× slower ns p=0.318 ROMol::getNumHeavyAtoms 351 ns ± 31.5 ns 347 ns ± 30.3 ns -1.0% 9.0% 1.01× faster ns p=0.749 ROMol::GetSubstructMatch 112 us ± 10.3 us 112 us ± 8.48 us -0.5% 9.2% 1.00× faster ns p=0.872 ROMol::GetSubstructMatch patty 3.61 ms ± 72.3 us 3.59 ms ± 95.7 us -0.4% 2.7% 1.00× faster ns p=0.631 ROMol::GetSubstructMatch RLewis 22 ms ± 447 us 21.9 ms ± 443 us -0.6% 2.0% 1.01× faster ns p=0.437 SmilesToMol 2.96 ms ± 111 us 2.95 ms ± 121 us -0.3% 4.1% 1.00× faster ns p=0.847 Summary: sig=0, below=0 (stat-sig but < threshold), ns=26, missing=0, new_only=0 Bonferroni-corrected α = 0.001923 (26 tests, family α = 0.05)
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
1 participant
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.
summary by opus-4.6 via opencode:
Refactor
dfsBuildStackfrom a free function into aDFSStackBuilderclass, allowing reuse of temporary vectors across calls to avoid repeated heap allocations.Benchmark (16 rounds × 800 samples, interleaved, Bonferroni-corrected α=0.002):
No significant results (0/26).
Benchmark Results (vs master)
Summary: sig=0, below=0 (stat-sig but < threshold), ns=26, missing=0, new_only=0
Bonferroni-corrected α = 0.001923 (26 tests, family α = 0.05)