search

openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
309 stars 90 forks source link

Optimize code for speed during system creation #253

Closed j-wags closed 2 years ago

j-wags commented 5 years ago

Note: @SimonBoothroyd did some code profiling and submitted significant speedups in #308

SimonBoothroyd commented 5 years ago

When upgrading my codebase from the 0.0.4 version of the toolkit to the 0.3.0 version I noticed degraded performance in a number of cases. After doing a bit of digging I came across a couple of bottlenecks in the code, namely:

While the latter PR goes someway to speed things up, I think there are still more gains to be had (although with probably a lot of effort!). In particular, the Topology object is currently designed to never directly store all of the indices of atoms in the system, nor the indices of valence terms in the full system, but rather store just enough information to generate these values on the fly when needed (such as by calling TopologyAtom.topology_atom_index).

The great thing about this design is that it has only a small memory footprint, but the downside is that every time you want information about the full system (as opposed to the reference molecules) you have to re-compute a lot of information. These computations add up rapidly inside of tight loops, and are still the biggest performance contributor when creating systems.

It would be great to start thinking about how Topology may be refactored to get around these issues. I think in particular more caching needs to be done, but this will require a large amount of bookkeeping to make sure when a change is made to the topology (i.e a new molecule is added / removed or a molecule is altered in some way), all of the cached information is properly invalidated and recomputed. While definitely do-able, to do this cleanly and efficiently will probably require a large amount of refactoring and possibly re-design of some of the TopologyXXX classes.

SimonBoothroyd commented 5 years ago

In addition to the above, we do (as we definitely should!) a lot of checks to ensure the systems being created are 'scientifically' correct - i.e. molecules have the bonds we expect them to have, all valence terms have been assigned parameters etc. At the moment these checks are typically applied to full systems, even though really we only care the reference molecules have been correctly assigned / validated, and then we clone these up to the full system. It sounds like the object proposed in #310 would definitely help alleviate some of this overhead!

jchodera commented 5 years ago

All of these are great suggestions!

Lazy caching is a great way to speed things up. We would just have to be diligent about invalidating the cache when necessary.

mattwthompson commented 2 years ago

Two steps forward, one step back. The openmm.System creation code has been moved over to Interchange, but has not really been optimized for performance (https://github.com/openforcefield/openff-interchange/issues/516).

I'm going to close this as it's years old now and significant changes have happened, but this was great information to read up on!