Skip to content

Major performance improvements: Ensembles, Volumes, Splitting - #455

Merged
jhprinz merged 5 commits into
openpathsampling:masterfrom
dwhswenson:ensemble_speed
Mar 28, 2016
Merged

jhprinz merged 5 commits into
openpathsampling:masterfrom
dwhswenson:ensemble_speed

Conversation

@dwhswenson

Copy link
Copy Markdown
Member

Performance improvements. Five-fold speedup in the DNA analysis I was doing; probably less (but still significant) in our examples.

  • Move expensive debug logging into logger.isEnabledFor(logging.DEBUG) scopes
  • Add short-circuit logic to volumes
  • Fix problem that .split is using an algorithm that does not scale linearly (not using trusted=True)
  • Switch ensembles to be better for short-circuit (Length before Volume) in flux calculation

Timings (in order of my implementations, so these are cumulative improvements) to use SingleTrajectoryAnalysis.analyze_flux() on a DNA trajectory of 10k frames. Inexact, since I just ran the test once on my laptop, but when you see a calculation drop from ~5 minutes to ~1 minute, that’s well beyond the margin of error (even for my sample of $N=1$).

  • Current master branch: 264.98s user 1.95s system 89% cpu 4:56.82 total
  • Rescope debugging: 127.29s user 1.64s system 89% cpu 2:24.61 total
  • Short-circuit volumes: 96.98s user 1.45s system 90% cpu 1:49.07 total
  • Fix split algorithm: 62.83s user 1.35s system 80% cpu 1:19.98 total
  • Fix short-circuit order in flux: 57.13s user 1.20s system 85% cpu 1:08.24 total

Of course, getting such improvement from the short-circuit volumes requires defining your volumes to take the most advantage of short-circuit logic. But that was easy to do with this particular system.

There’s some room to improve SequentialEnsemble.can_append, especially in its use of _find_subtraj_final. This should give us a significant speed boost. I’ll do that at a later date.

Other notes for future speedups:

  • Looks like calculating the unit cells (after loading trajectory) is one of the major bottlenecks for this (25% of the total time). But I think that is in MDTraj?
  • chaindict is always going to be inner-loop stuff. It looks like a good target for cythonization -- it does a lot of python function calls deep in inner loops, and Cython can massively decrease the overhead cost of those function calls.
  • There are a number of algorithmic speed-ups that can happen once we start including results from path ensemble theory. That should be after 1.0, though.

@dwhswenson

Copy link
Copy Markdown
Member Author

Call graphs, generated by gprof2dot for master and for ensemble_speed branch. First, master:
master

Now ensemble_speed:
ensemble_speed

Note the difference in the number of times volume calls chain_dict: it still seems a bit much at > 550k, but that's a big improvement over 12.3M.

@dwhswenson

Copy link
Copy Markdown
Member Author

Passed tests. Ready for review.

@jhprinz

jhprinz commented Mar 28, 2016

Copy link
Copy Markdown
Contributor

Very nice! Really. This is how I like improvements... I think with all the changes in storage lately we should change the chaindict approach also a little. But that is different PR. Maybe cython will do some tricks. I think we might remove some convenience and trade it for speed. Especially the flexibility in storing and caching etc. is possible not necessary.

Looks good and will merge.

@jhprinz
jhprinz merged commit f2f008c into openpathsampling:master Mar 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants