-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimized getOutputQuantity to avoid string copies #1016
Conversation
This change leads to a 4% speedup on my laptop with the small benchmark (20 atoms, 4 of which biased)
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1016 +/- ##
==========================================
+ Coverage 84.15% 84.18% +0.02%
==========================================
Files 612 612
Lines 56496 56497 +1
==========================================
+ Hits 47547 47563 +16
+ Misses 8949 8934 -15 ☔ View full report in Codecov by Sentry. |
@GiovanniBussi this is interesting because it has an impact also on the large simulation (110,000 atoms, saxs on ~6,000 atoms), it recovers the 2.5% loss but not speeding up the waiting data that is unchanged but speeding up quite a bit the update section, while the impact on alaDP is tiny, ~1% |
Unexpected. Maybe you have many actions? My understanding is that this function, with a string argument, is called at each step over all actions to check if they contain a "work" or "bias" named value. |
I have many components, this is SAXS+Metainference so I have ~30 components from SAXS and BIASVALUE applied on one of these many |
So I think I can optimize this further. The search is done with a loop now, but I think we should store a unordered_map with all value names if we really want to retrieve them by name during the simulation |
I think this is OK. Could there be an easier (and possibly faster) way to do this as follows by always creating a combine action in PLUMED after all the actions are read in like this: total_bias: COMBINE ARG=*.bias PERIODIC=NO You then set the value of bias equal to the total bias. You could perhaps do the the first time getBias is called. If you did this then you can use a GET command to pass the final bias from plumed back to the MD code. Also if the MD code doesn't take back the bias then the total bias is not even calculated. The only problem with this would be if you have biases that are not calculated on every step. It would be easy to solve that one by adding a flag in ActionPilot that ensures that the GET action pilot doesn't activate its dependencies. For all the non-active bias values you would thus be adding zeros. |
@gtribello I am not sure what you propose is optimal because it would not allow adding new actions after the simulation is started. What about keeping track of which actions have such components by something similar to the Meanwhile I will merge this, that in any case speeds up the search for the proper component |
This change leads to a 4% speedup on my laptop with the small benchmark (20 atoms, 4 of which biased)
@gtribello can you confirm this is correct? Can I correctly assume that, if the
values[i]->name
is longer thangetLabe()
, than it starts withgetLabel()+'.'
? In this way I avoid creating a new std::string and directly compare the remaining part.If not I can add a separate check on the first part of the string as well. In any case this will allow me to save the allocation.