refactor: Range based pair algorithms - #2202
Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 3abf23c | Previous: 9005f62 | Ratio |
|---|---|---|---|
BM_Box_MinimumImage<OrthorhombicBox> |
14.007764158714778 ns/iter |
5.911377415380199 ns/iter |
2.37 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @disorderedmaterials/dissolve-devs
trisyoungs
left a comment
There was a problem hiding this comment.
Excellent. One question and one suggestion for further clean-up of the calls to potentially consider before merging.
|
|
||
| dissolve::for_each_pair( | ||
| ParallelPolicies::par, 0, nTypes, | ||
| ParallelPolicies::par, std::views::iota(0, nTypes), |
There was a problem hiding this comment.
Just a thought - should we have an overload of for_each_pair specific to 0 -> N-1 ranges, which only takes the upper N value as a single argument, and which calls for_each_pair with the appropriate std::iota range?
| dissolve::for_each_pair(ParallelPolicies::seq, std::span(sp->atoms().begin(), sp->nAtoms()), pairwiseForceOperator); | ||
| else | ||
| dissolve::for_each_pair(ParallelPolicies::par, sp->atoms().begin(), sp->atoms().end(), pairwiseForceOperator); | ||
| dissolve::for_each_pair(ParallelPolicies::par, std::span(sp->atoms().begin(), sp->nAtoms()), pairwiseForceOperator); |
There was a problem hiding this comment.
Curious - why the use of std::span here and nowhere else?
There was a problem hiding this comment.
To zeroth order: I don't know.
To first order: if I pass just pass in sp->atoms(), I get a compiler error about SpeciesAtom missing a copy constructor. It seems that the specific std::vector implementation of ranges in GCC used the copy constructor for some optimizations. Thankfully, the standard does have std::span which just takes two iterators and doesn't make any assumptions, so we can use it as a wrapper.
76e315a to
c4ae8aa
Compare
c4ae8aa to
3abf23c
Compare
This PR takes advantage of the range concept to simplify the calling procedure for
for_each_pairandfor_each_pair_early. They now just take the entire range as a single argument instead of requiring individual beginning and ending iterators. In other words, code that wasfor_each_pair(vector.begin(), vector.end(), doSomething);is now
for_each_pair(vector, doSomething);Also, each function was originally overloaded with one version for iterators and another for flat integers. Now, instead, there is a single method which does the right thing for either instance. The one price for this is that the integer parameters now need to be wrapped in an iota view. For example, code that was
is now
This should not be merged before #2201, as I think as that PR adds some calls to
for_each_pairthat I will need to convert to the range format