Optimize wholemolecules #1045
Open
Optimize wholemolecules #1045
Conversation
This file contains 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
speedup 3%, but code is a mess
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/core/ActionWithValue.cpp
Outdated
Comment on lines
317
to
319
| // for(unsigned j=0; j<forcesForApply.size(); ++j) { | ||
| // forcesForApply[j]+=omp_f[j]; | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
src/core/ActionWithValue.cpp
Outdated
Comment on lines
305
to
306
| // if( nt>1 ) for(unsigned j=0; j<nder; ++j) omp_f[j] += ff*thisderiv[1+j]; | ||
| //else for(unsigned j=0; j<nder; ++j) forcesForApply[j] += ff*thisderiv[1+j]; |
Check notice
Code scanning / CodeQL
Commented-out code Note
src/core/ActionAtomistic.cpp
Outdated
Comment on lines
330
to
336
| // for(const auto & a : atom_value_ind) { | ||
| // plumed_dbg_massert( ind<forcesToApply.size(), "problem setting forces in " + getLabel() ); | ||
| // std::size_t nn = a.first, kk = a.second; | ||
| // xpos[nn]->inputForce[kk] += forcesToApply[ind]; ind++; | ||
| // ypos[nn]->inputForce[kk] += forcesToApply[ind]; ind++; | ||
| // zpos[nn]->inputForce[kk] += forcesToApply[ind]; ind++; | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
src/core/ActionAtomistic.cpp
Outdated
Comment on lines
293
to
301
| // for(const auto & a : atom_value_ind) { | ||
| // std::size_t nn = a.first, kk = a.second; | ||
| // positions[j][0] = xpos[nn]->data[kk]; | ||
| // positions[j][1] = ypos[nn]->data[kk]; | ||
| // positions[j][2] = zpos[nn]->data[kk]; | ||
| // charges[j] = chargev[nn]->data[kk]; | ||
| // masses[j] = masv[nn]->data[kk]; | ||
| // j++; | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
src/core/ActionWithVirtualAtom.cpp
Outdated
Comment on lines
75
to
80
| // for(const auto & a : atom_value_ind) { | ||
| // std::size_t nn = a.first, kk = a.second; | ||
| // xpos[nn]->inputForce[kk] += xf*xval->data[1+k] + yf*yval->data[1+k] + zf*zval->data[1+k]; k++; | ||
| // ypos[nn]->inputForce[kk] += xf*xval->data[1+k] + yf*yval->data[1+k] + zf*zval->data[1+k]; k++; | ||
| // zpos[nn]->inputForce[kk] += xf*xval->data[1+k] + yf*yval->data[1+k] + zf*zval->data[1+k]; k++; | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
src/core/ActionAtomistic.cpp
Outdated
| atom_value_ind_grouped.back().second.push_back(kk); | ||
| auto prev_nn=nn; | ||
| for(unsigned i=1; i<atom_value_ind.size(); i++) { | ||
| auto nn = atom_value_ind[i].first; |
Check notice
Code scanning / CodeQL
Declaration hides variable Note
src/core/ActionAtomistic.cpp
Outdated
| auto prev_nn=nn; | ||
| for(unsigned i=1; i<atom_value_ind.size(); i++) { | ||
| auto nn = atom_value_ind[i].first; | ||
| auto kk = atom_value_ind[i].second; |
Check notice
Code scanning / CodeQL
Declaration hides variable Note
Comment on lines
+286
to
+293
| // } else { | ||
| // for(unsigned j=1; j<p_groups[i].size(); ++j) { | ||
| // Vector first=getGlobalPosition(p_roots[i][j-1]); | ||
| // Vector second=getGlobalPosition(p_groups[i][j]); | ||
| // second=first+pbcDistance(first,second); | ||
| // setGlobalPosition(p_groups[i][j], second ); | ||
| // } | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code Note
Comment on lines
+263
to
+267
| // if(addref) { | ||
| // first = refs[i]+pbcDistance(refs[i],first); | ||
| // setGlobalPosition( p_groups[i][0], first ); | ||
| // } | ||
| // if(!doemst) { |
Check notice
Code scanning / CodeQL
Commented-out code Note
|
And here results for the intel compiler on my workstation. Reference is current master (just pulled), then I time master + this optimization of wholemolecules. My input: 28% -> 26% overhead Carlo's input: no measurable overhead in both cases (<1%) |
|
Hello @GiovanniBussi What you have done here seems sensible. I don't think I can do it better. |
|
@gtribello I really don't like the way it's done, because it's intrusive in what's supposed to be "user code" (wholemolecules), with modifications that are difficult to understand. I also want to repeat the timings, because there is an interplay between all the optimizations we are doing. This one might be not so relevant, so I would keep it on hold |
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.
@gtribello here I tried a trick similar to #1044 to optimize wholemolecules. Notice that I didn't tough the EMST stuff, so tests are expected to fail. Anyway, tests with simple wholemolecules should work.
On my usual input I get some further speedup, with overhead going from 13% to 10%.
On @carlocamilloni 's input see here it's even better. Before this commit, I get same performance as v2.9. After this commit I gain something like 5%.
Maybe @gtribello you want to have a look at this code and think if there's some other reasonable (and simpler) solution. Notice that these PR are all stacked onto each other, so you should only look at the last commit (but the performance is measured including the previous PRs)
With my input:
With Carlo's input: